From 90b72ba7a899cea3beccc7b3b795501fbdda14d7 Mon Sep 17 00:00:00 2001 From: David van Moolenbroek Date: Sat, 3 Mar 2012 19:25:57 +0100 Subject: [PATCH] Kernel: pass FPU restore exception to user process Previously, user processes could cause a kernel panic upon FPU state restore, by passing bogus FPU state to the kernel (through e.g. sigreturn). With this patch, the process is now sent a SIGFPE signal instead. --- kernel/arch/i386/arch_system.c | 12 +++-- kernel/arch/i386/exception.c | 11 ++++ kernel/arch/i386/include/arch_proto.h | 7 ++- kernel/arch/i386/klib.S | 16 ++++-- kernel/arch/i386/mpx.S | 4 +- kernel/proc.c | 10 +++- kernel/proto.h | 2 +- test/Makefile | 2 + test/run | 2 +- test/test62.c | 72 +++++++++++++++++++++++++++ 10 files changed, 125 insertions(+), 13 deletions(-) create mode 100644 test/test62.c diff --git a/kernel/arch/i386/arch_system.c b/kernel/arch/i386/arch_system.c index 439e84f8b..505b50f53 100644 --- a/kernel/arch/i386/arch_system.c +++ b/kernel/arch/i386/arch_system.c @@ -322,18 +322,24 @@ PUBLIC void save_fpu(struct proc *pr) #endif } -PUBLIC void restore_fpu(struct proc *pr) +PUBLIC int restore_fpu(struct proc *pr) { + int failed; + if(!proc_used_fpu(pr)) { fninit(); pr->p_misc_flags |= MF_FPU_INITIALIZED; } else { if(osfxsr_feature) { - fxrstor(pr->p_fpu_state.fpu_save_area_p); + failed = fxrstor(pr->p_fpu_state.fpu_save_area_p); } else { - frstor(pr->p_fpu_state.fpu_save_area_p); + failed = frstor(pr->p_fpu_state.fpu_save_area_p); } + + if (failed) return EINVAL; } + + return OK; } PUBLIC void cpu_identify(void) diff --git a/kernel/arch/i386/exception.c b/kernel/arch/i386/exception.c index fd0748f77..99c169814 100644 --- a/kernel/arch/i386/exception.c +++ b/kernel/arch/i386/exception.c @@ -224,6 +224,17 @@ PUBLIC void exception_handler(int is_nested, struct exception_frame * frame) panic("Copy involving a user pointer failed unexpectedly!"); } } + + /* Pass any error resulting from restoring FPU state, as a FPU + * exception to the process. + */ + if (((void*)frame->eip >= (void*)fxrstor && + (void *)frame->eip <= (void*)__fxrstor_end) || + ((void*)frame->eip >= (void*)frstor && + (void *)frame->eip <= (void*)__frstor_end)) { + frame->eip = (reg_t) __frstor_failure; + return; + } } if(frame->vector == PAGE_FAULT_VECTOR) { diff --git a/kernel/arch/i386/include/arch_proto.h b/kernel/arch/i386/include/arch_proto.h index f97466fa3..e7af99cf6 100644 --- a/kernel/arch/i386/include/arch_proto.h +++ b/kernel/arch/i386/include/arch_proto.h @@ -99,8 +99,11 @@ _PROTOTYPE( void fninit, (void)); _PROTOTYPE( void clts, (void)); _PROTOTYPE( void fxsave, (void *)); _PROTOTYPE( void fnsave, (void *)); -_PROTOTYPE( void fxrstor, (void *)); -_PROTOTYPE( void frstor, (void *)); +_PROTOTYPE( int fxrstor, (void *)); +_PROTOTYPE( int __fxrstor_end, (void *)); +_PROTOTYPE( int frstor, (void *)); +_PROTOTYPE( int __frstor_end, (void *)); +_PROTOTYPE( int __frstor_failure, (void *)); _PROTOTYPE( unsigned short fnstsw, (void)); _PROTOTYPE( void fnstcw, (unsigned short* cw)); diff --git a/kernel/arch/i386/klib.S b/kernel/arch/i386/klib.S index a46f9e99d..5c0c4f679 100644 --- a/kernel/arch/i386/klib.S +++ b/kernel/arch/i386/klib.S @@ -608,21 +608,29 @@ ENTRY(fnsave) ret /*===========================================================================*/ -/* fxrstor */ +/* fxrstor */ /*===========================================================================*/ ENTRY(fxrstor) mov 4(%esp), %eax - fxrstor (%eax) /* Do not change the operand! (gas2ack) */ + fxrstor (%eax) +ENTRY(__fxrstor_end) + xor %eax, %eax ret /*===========================================================================*/ -/* frstor */ +/* frstor */ /*===========================================================================*/ ENTRY(frstor) mov 4(%esp), %eax - frstor (%eax) /* Do not change the operand! (gas2ack) */ + frstor (%eax) +ENTRY(__frstor_end) + xor %eax, %eax ret +/* Shared exception handler for both fxrstor and frstor. */ +ENTRY(__frstor_failure) + mov $1, %eax + ret /*===========================================================================*/ /* read_cr0 */ diff --git a/kernel/arch/i386/mpx.S b/kernel/arch/i386/mpx.S index 8663db3a6..43b77926b 100644 --- a/kernel/arch/i386/mpx.S +++ b/kernel/arch/i386/mpx.S @@ -548,7 +548,9 @@ LABEL(copr_not_available) push %ebp mov $0, %ebp call _C_LABEL(context_stop) - jmp _C_LABEL(copr_not_available_handler) + call _C_LABEL(copr_not_available_handler) + /* reached upon failure only */ + jmp _C_LABEL(switch_to_user) copr_not_available_in_kernel: pushl $0 diff --git a/kernel/proc.c b/kernel/proc.c index c83dc291a..d385e4349 100644 --- a/kernel/proc.c +++ b/kernel/proc.c @@ -1885,7 +1885,15 @@ PUBLIC void copr_not_available_handler(void) * restore the current process' state and let it run again, do not * schedule! */ - restore_fpu(p); + if (restore_fpu(p) != OK) { + /* Restoring FPU state failed. This is always the process's own + * fault. Send a signal, and schedule another process instead. + */ + *local_fpu_owner = NULL; + cause_sig(proc_nr(p), SIGFPE); + return; + } + *local_fpu_owner = p; context_stop(proc_addr(KERNEL)); restore_user_context(p); diff --git a/kernel/proto.h b/kernel/proto.h index abe52df3d..e3f3a9f09 100644 --- a/kernel/proto.h +++ b/kernel/proto.h @@ -32,7 +32,7 @@ _PROTOTYPE( void cycles_accounting_init, (void) ); _PROTOTYPE( void context_stop, (struct proc * p) ); /* this is a wrapper to make calling it from assembly easier */ _PROTOTYPE( void context_stop_idle, (void) ); -_PROTOTYPE( void restore_fpu, (struct proc *) ); +_PROTOTYPE( int restore_fpu, (struct proc *) ); _PROTOTYPE( void save_fpu, (struct proc *) ); _PROTOTYPE( void save_local_fpu, (struct proc *) ); _PROTOTYPE( void fpu_sigcontext, (struct proc *, struct sigframe *fr, struct sigcontext *sc) ); diff --git a/test/Makefile b/test/Makefile index 23d9ad2b4..97fc98c1b 100644 --- a/test/Makefile +++ b/test/Makefile @@ -10,6 +10,7 @@ OBJ= test1 test2 test3 test4 test5 test6 test7 test8 test9 \ test30 test31 test32 test34 test35 test36 test37 test38 test39 \ test40 test41 test42 test45 test47 test48 test49 \ test50 test53 test54 test55 test58 \ + test62 \ t10a t11a t11b t40a t40b t40c t40d t40e t40f t60a t60b \ ROOTOBJ= test11 test33 test43 test44 test46 test56 test60 test61 @@ -113,3 +114,4 @@ test60: test60.c t60a: t60a.c t60b: t60b.c test61: test61.c +test62: test62.c diff --git a/test/run b/test/run index 6b70767a5..ebd8f2407 100755 --- a/test/run +++ b/test/run @@ -14,7 +14,7 @@ badones= # list of tests that failed tests=" 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 \ 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 \ 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 \ - 61 \ + 61 62 \ sh1.sh sh2.sh interp.sh" tests_no=`expr 0` diff --git a/test/test62.c b/test/test62.c new file mode 100644 index 000000000..f98ddaf33 --- /dev/null +++ b/test/test62.c @@ -0,0 +1,72 @@ +/* FPU state corruption test. This used to be able to crash the kernel. */ +#include +#include +#include +#include +#include +#include + +#define MAX_ERROR 1 +#include "common.c" + +double state = 2.0; +static int count; + +static void use_fpu(int n) +{ + state += (double) n * 0.5; +} + +static void crashed(int sig) +{ + exit(EXIT_SUCCESS); +} + +static void handler(int sig, int code, struct sigcontext *sc) +{ + memset(&sc->sc_fpu_state, count, sizeof(sc->sc_fpu_state)); +} + +int main(void) +{ + int status; + + start(62); + subtest = 0; + + signal(SIGUSR1, (void (*)(int)) handler); + + /* Initialize the FPU state. This state is inherited, too. */ + use_fpu(-1); + + for (count = 0; count <= 255; count++) { + switch (fork()) { + case -1: + e(1); + + break; + + case 0: + signal(SIGFPE, crashed); + + /* Load bad state into the kernel. */ + if (kill(getpid(), SIGUSR1)) e(2); + + /* Let the kernel restore the state. */ + use_fpu(count); + + exit(EXIT_SUCCESS); + + default: + /* We cannot tell exactly whether what happened is correct or + * not -- certainly not in a platform-independent way. However, + * if the whole system keeps running, that's good enough. + */ + (void) wait(&status); + } + } + + if (state <= 1.4 || state >= 1.6) e(3); + + quit(); +} -- 2.44.0