]> Zhao Yanbai Git Server - minix.git/commitdiff
Kernel: pass FPU restore exception to user process
authorDavid van Moolenbroek <david@minix3.org>
Sat, 3 Mar 2012 18:25:57 +0000 (19:25 +0100)
committerBen Gras <ben@minix3.org>
Tue, 6 Mar 2012 15:12:58 +0000 (16:12 +0100)
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
kernel/arch/i386/exception.c
kernel/arch/i386/include/arch_proto.h
kernel/arch/i386/klib.S
kernel/arch/i386/mpx.S
kernel/proc.c
kernel/proto.h
test/Makefile
test/run
test/test62.c [new file with mode: 0644]

index 439e84f8b71ea2a7202338bb4b699c75651ef541..505b50f53cbc4f8c9ebb4809eef63045456bcdaa 100644 (file)
@@ -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)
index fd0748f7785043cc1a5eebd863085c1c5c51e8e6..99c16981474dd2abcf3f7b38c974a70a9b73c6e2 100644 (file)
@@ -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) {
index f97466fa34eec5d3fb0ca32281af1b5c9c1da9fc..e7af99cf6a4043386974271514b0619c41173f24 100644 (file)
@@ -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));
 
index a46f9e99d6af682f897cc98ac8b8a107fca73dd1..5c0c4f67979a90057ef2afaeb16212d34c5a3306 100644 (file)
@@ -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                                       */
index 8663db3a6cabc3d5428a12f06ce207c37e15d426..43b77926be48cde8ee6a0ae0185453cbd42f4c79 100644 (file)
@@ -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
index c83dc291aa6fdb6a7f7a7def1a39352a50c5b635..d385e4349ad95ae4c848195a92bff13b44dc2ad9 100644 (file)
@@ -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);
index abe52df3d240bdd4ea51874ec1a8a3dc425572ba..e3f3a9f09479425b496bfeb5331f2b4e887d6ab5 100644 (file)
@@ -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)   );
index 23d9ad2b4dbe3cda8c541d62f18c67a9de8752fa..97fc98c1bbbec167fc8a9c7507c8909a9daf057d 100644 (file)
@@ -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
index 6b70767a590419e8d05c971fb6645eb65ccc0717..ebd8f24074ab4edcce2d62b7f5ec32a44ca96475 100755 (executable)
--- 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 (file)
index 0000000..f98ddaf
--- /dev/null
@@ -0,0 +1,72 @@
+/* FPU state corruption test. This used to be able to crash the kernel. */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <signal.h>
+#include <sys/wait.h>
+#include <machine/fpu.h>
+
+#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();
+}