Fixes sysenter stack pointer bug
authorBarret Rhoden <brho@cs.berkeley.edu>
Sat, 24 Oct 2009 22:52:17 +0000 (15:52 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Mon, 26 Oct 2009 08:29:25 +0000 (09:29 +0100)
Sysenter needs to have interrupts turned on before calling sysexit,
during which it needs to have a good stack pointer in case an interrupt
comes in.  If this fails, the interrupt mechanism will use the current
stack pointer (often in the struct proc!) as the kernel stack and start
clobbering things.  This manifested itself as a proc that was
incorrectly locked.

kern/arch/i386/env.c
kern/arch/i386/trapentry.S

index 45132c0..d241bab 100644 (file)
@@ -15,6 +15,9 @@
 //
 void env_pop_tf(trapframe_t *tf)
 {
+       /* In case they are enabled elsewhere.  We can't take an interrupt in these
+        * routines, due to how they play with the kernel stack pointer. */
+       disable_irq();
        /*
         * If the process entered the kernel via sysenter, we need to leave via
         * sysexit.  sysenter trapframes have 0 for a CS, which is pushed in
@@ -35,19 +38,34 @@ void env_pop_tf(trapframe_t *tf)
                              : : "g" (tf) : "memory");
                panic("iret failed");  /* mostly to placate the compiler */
        } else {
-               /* Return path of sysexit.  See sysenter_handler's asm for details. */
+               /* Return path of sysexit.  See sysenter_handler's asm for details.
+                * One difference is that this tf could be somewhere other than a stack
+                * (like in a struct proc).  We need to make sure esp is valid once
+                * interrupts are turned on (which would happen on popfl normally), so
+                * we need to save and restore a decent esp (the current one).  We need
+                * a place to save it that is accessible after we change the stack
+                * pointer to the tf *and* that is specific to this core/instance of
+                * sysexit.  The simplest and nicest is to use the tf_esp, which we
+                * can just pop.  Incidentally, the value in oesp would work too.
+                * To prevent popfl from turning interrupts on, we hack the tf's eflags
+                * so that we have a chance to change esp to a good value before
+                * interrupts are enabled.  The other option would be to throw away the
+                * eflags, but that's less desirable. */
+               tf->tf_eflags &= !FL_IF;
+               tf->tf_esp = read_esp();
                asm volatile ("movl %0,%%esp;           "
                              "popal;                   "
                              "popl %%es;               "
                              "popl %%ds;               "
-                             "addl $0x10, %%esp;       "
+                             "addl $0x10,%%esp;        "
                              "popfl;                   "
-                             "movl %%ebp, %%ecx;       "
-                             "movl %%esi, %%edx;       "
+                             "movl %%ebp,%%ecx;        "
+                             "movl %%esi,%%edx;        "
+                             "popl %%esp;              "
                              "sti;                     "
                              "sysexit                  "
                              : : "g" (tf) : "memory");
-               panic("sysexit failed");  /* mostly to placate the compiler */
+               panic("sysexit failed");  /* mostly to placate your mom */
        }
 }
 
index 2cac166..f934025 100644 (file)
@@ -245,7 +245,7 @@ sysenter_handler:
        popl %es
        popl %ds
        addl $0x10, %esp                # pop T_SYSCALL and the three zeros
-       popfl                                   # restore EFLAGS
+       popfl                                   # restore EFLAGS (and usually enables interrupts!)
        movl %ebp, %ecx
        movl %esi, %edx
        sti                                             # interrupts are turned off when starting a core