Make exec_user_param() safe with SMEP and SMAP active

To make this safe, the iret must move straight from supervisor code/stack to
user code/stack.  Therefore, the stack cannot be shared any more.

The existing user_stack[] can be used as the separate stack.  This make the
exec_user_param() infrastructure no longer reentrant, but this isn't expected
to be a problem for tests.

A new .text.user section is introduced, which is automatically mapped as user
during setup.

The behaviour of exec_user_param() and X86_VEC_RET2KERN are altered to match.
exec_user_param() stores the supervisor stack in %rbp across the user
execution, and fakes up a return address as if it had simply called the user
code.  X86_VEC_RET2KERN restores the stack from %rbp and follows the fake
return address to reenter exec_user_param()'s context.

Invocation of the user function call moves into exec_user_stub() which is
located inside .text.user.  The 32bit version must pass all parameters in
registers, rather than on the stack.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff --git a/arch/x86/entry_32.S b/arch/x86/entry_32.S
index 08609e6a91827b8e048bd67bd631a561dcd2365c..d1bdbab2548efa8b73c8ba4cc675dff7e0561a2c 100644 (file)
--- a/arch/x86/entry_32.S
+++ b/arch/x86/entry_32.S
@@ -1,4 +1,5 @@
 #include <arch/idt.h>
+#include <arch/page.h>
 #include <arch/processor.h>
 #include <arch/segment.h>
 #include <xtf/asm_macros.h>
@@ -100,13 +101,8 @@ ENDFUNC(handle_exception)
 
 
 ENTRY(entry_ret_to_kernel)      /* int $X86_VEC_RET2KERN */
-
-        /* User required to ensure this is called from CPL > KERNEL_RPL */
-
-        mov 0*4(%esp), %ecx     /* Stash %eip from iret frame */
-        mov 3*4(%esp), %esp     /* Load %esp from iret frame  */
-        jmp *%ecx               /* Jump back                  */
-
+        mov %ebp, %esp          /* Restore %esp to exec_user_param()'s context. */
+        ret
 ENDFUNC(entry_ret_to_kernel)
 
 ENTRY(exec_user_param)
@@ -115,10 +111,18 @@ ENTRY(exec_user_param)
          * 1*4(%esp) ulong (*fn)(ulong)
          * 0*4(%esp) return address
          */
+        push %ebp
+
+        /* Prepare to "call" exec_user_stub(). */
+        mov (1+1)*4(%esp), %eax /* Pass fn() in %eax */
+        mov (1+2)*4(%esp), %ecx /* Pass p1 in %ecx */
+        push $1f                /* Fake return addr as if we'd called exec_user_stub(). */
+        mov %esp, %ebp          /* Stash %esp for entry_ret_to_kernel(). */
 
+        /* Prepare an IRET frame. */
         push $__USER_DS         /* SS */
-        push %esp
-        addl $4, (%esp)         /* ESP */
+                                /* ESP */
+        push $user_stack + PAGE_SIZE
         pushf                   /* EFLAGS */
 
 #if defined(CONFIG_PV)          /* PV guests see the real interrupt flag. Clobber it. */
@@ -126,19 +130,29 @@ ENTRY(exec_user_param)
 #endif
 
         push $__USER_CS         /* CS */
-        push $1f                /* EIP */
+        push $exec_user_stub    /* EIP */
 
         env_IRET                /* Drop to user privilege. */
-1:
-        push 2*4(%esp)          /* Re-push p1 for fn()'s call frame. */
-        call *(1+1)*4(%esp)     /* fn(p1) */
-        add $1*4, %esp          /* Pop p1. */
 
-        int $X86_VEC_RET2KERN   /* Return to kernel privilege. */
+1:      /* entry_ret_to_kernel() returns here with a sensible stack. */
+        pop %ebp
         ret
 
 ENDFUNC(exec_user_param)
 
+.pushsection .text.user, "ax", @progbits
+ENTRY(exec_user_stub)
+        /*
+         * For SMEP/SMAP safety, no shared stack can be used, so all
+         * parameters are passed in registers.
+         */
+        push %ecx               /* Push p1 for fn()'s call frame. */
+        call *%eax              /* fn(p1) */
+
+        int $X86_VEC_RET2KERN   /* Return to kernel privilege. */
+ENDFUNC(exec_user_stub)
+.popsection
+
 /*
  * Local variables:
  * tab-width: 8

diff --git a/arch/x86/entry_64.S b/arch/x86/entry_64.S
index 8b70158d3142b37be36242916a7016fc9df390fe..e6a81f07f7843038f3eb6ac854dae54ad1bb9a66 100644 (file)
--- a/arch/x86/entry_64.S
+++ b/arch/x86/entry_64.S
@@ -1,4 +1,5 @@
 #include <arch/idt.h>
+#include <arch/page.h>
 #include <arch/processor.h>
 #include <arch/segment.h>
 #include <xtf/asm_macros.h>
@@ -104,17 +105,21 @@ ENDFUNC(handle_exception)
 ENTRY(entry_ret_to_kernel)      /* int $X86_VEC_RET2KERN */
         env_ADJUST_FRAME
 
-        mov 0*8(%rsp), %rdi     /* Stash %rip from iret frame */
-        mov 3*8(%rsp), %rsp     /* Load %esp from iret frame  */
-        jmp *%rdi               /* Jump back                  */
-
+        mov %rbp, %rsp          /* Restore %rsp to exec_user_param()'s context. */
+        ret
 ENDFUNC(entry_ret_to_kernel)
 
 ENTRY(exec_user_param)          /* ulong (*fn)(ulong), ulong p1 */
+        push %rbp
+
+        /* Prepare to "call" exec_user_stub(). */
+        push $1f                /* Fake return addr as if we'd called exec_user_stub(). */
+        mov %rsp, %rbp          /* Stash %rsp for entry_ret_to_kernel(). */
 
+        /* Prepare an IRET frame. */
         push $__USER_DS         /* SS */
-        push %rsp
-        addq $8, (%rsp)         /* RSP */
+                                /* RSP */
+        push $user_stack + PAGE_SIZE
         pushf                   /* RFLAGS */
 
 #if defined(CONFIG_PV)          /* PV guests see the real interrupt flag. Clobber it. */
@@ -122,18 +127,25 @@ ENTRY(exec_user_param)          /* ulong (*fn)(ulong), ulong p1 */
 #endif
 
         push $__USER_CS         /* CS */
-        push $1f                /* RIP */
+        push $exec_user_stub    /* RIP */
 
         env_IRETQ               /* Drop to user privilege. */
-1:
-        xchg %rdi, %rsi         /* Swap p1 to be first parameter to fn(). */
-        call *%rsi              /* fn(p1) */
 
-        int $X86_VEC_RET2KERN   /* Return to kernel privilege. */
+1:      /* entry_ret_to_kernel() returns here with a sensible stack. */
+        pop %rbp
         ret
 
 ENDFUNC(exec_user_param)
 
+.pushsection .text.user, "ax", @progbits
+ENTRY(exec_user_stub)
+        xchg %rdi, %rsi         /* Swap p1 to be first parameter to fn(). */
+        call *%rsi              /* fn(p1) */
+
+        int $X86_VEC_RET2KERN   /* Return to kernel privilege. */
+ENDFUNC(exec_user_stub)
+.popsection
+
 /*
  * Local variables:
  * tab-width: 8

diff --git a/arch/x86/hvm/traps.c b/arch/x86/hvm/traps.c
index cfaa69b31c2c25f4973cf8a068e7116732cf63a6..102af86453f39bd250d52a42fd85005c136dc745 100644 (file)
--- a/arch/x86/hvm/traps.c
+++ b/arch/x86/hvm/traps.c
@@ -168,6 +168,15 @@ void arch_init_traps(void)
 
         l1_identmap[gfn] |= _PAGE_USER;
 
+        extern const char __start_user_text[], __end_user_text[];
+        unsigned long end = virt_to_gfn(__end_user_text);
+
+        if ( gfn >= ARRAY_SIZE(l1_identmap) )
+            panic("__{start,end}_user_text[] outside of l1_identmap[]\n");
+
+        for ( gfn = virt_to_gfn(__start_user_text); gfn < end; ++gfn )
+            l1_identmap[gfn] |= _PAGE_USER;
+
         write_cr3((unsigned long)&cr3_target);
     }
 }

diff --git a/arch/x86/link.lds.S b/arch/x86/link.lds.S
index 1f4f604a4c094e414dae0877567e76f79700db64..3941858f7d12ecd69fecd9be4d6a3bc50a66b4b3 100644 (file)
--- a/arch/x86/link.lds.S
+++ b/arch/x86/link.lds.S
@@ -36,6 +36,13 @@ SECTIONS
 
         .text : {
                 *(.text)
+
+       . = ALIGN(PAGE_SIZE);
+        __start_user_text = .;
+                *(.text.user)
+       . = ALIGN(PAGE_SIZE);
+        __end_user_text = .;
+
         } :load = 0
 
         .data : {
@@ -74,6 +81,9 @@ ASSERT(IS_ALIGNED(hypercall_page, PAGE_SIZE), "hypercall_page misaligned");
 ASSERT(IS_ALIGNED(boot_stack, PAGE_SIZE), "boot_stack misaligned");
 ASSERT(IS_ALIGNED(user_stack, PAGE_SIZE), "user_stack misaligned");
 
+ASSERT(IS_ALIGNED(__start_user_text, PAGE_SIZE), "__start_user_text misaligned");
+ASSERT(IS_ALIGNED(__end_user_text,   PAGE_SIZE), "__end_user_text misaligned");
+
 #ifdef CONFIG_HVM
 
 ASSERT(IS_ALIGNED(pae_l1_identmap, PAGE_SIZE), "pae_l1_identmap misaligned");

diff --git a/arch/x86/pv/traps.c b/arch/x86/pv/traps.c
index f4a1b17281520dc92912e46aeb3ddc9031f256b0..1eef8d8053a6e2674bca4feed89c51e6c6e7bb02 100644 (file)
--- a/arch/x86/pv/traps.c
+++ b/arch/x86/pv/traps.c
@@ -222,6 +222,18 @@ void arch_init_traps(void)
 
         if ( hypercall_update_va_mapping(user_stack, nl1e, UVMF_INVLPG) )
             panic("Unable to remap user_stack with _PAGE_USER\n");
+
+        extern const char __start_user_text[], __end_user_text[];
+        unsigned long va = (unsigned long)__start_user_text;
+        while ( va < (unsigned long)__end_user_text )
+        {
+            nl1e = pte_from_virt(_p(va), PF_SYM(AD, U, RW, P));
+
+            if ( hypercall_update_va_mapping(_p(va), nl1e, UVMF_INVLPG) )
+                panic("Unable to remap user_text with _PAGE_USER\n");
+
+            va += PAGE_SIZE;
+        }
     }
 #endif
 

diff --git a/include/xtf/lib.h b/include/xtf/lib.h
index 9d21a79e40337d6ef107154578c7bbc6a7d3d9d1..634d0291463fafbeb3978e358727f5a87a29b93e 100644 (file)
--- a/include/xtf/lib.h
+++ b/include/xtf/lib.h
@@ -55,6 +55,8 @@ void heapsort(void *base, size_t nmemb, size_t size,
 
 /**
  * Execute fn(p1) at user privilege, passing its return value back.
+ *
+ * fn() is executed on user_stack[], and is non-reentrant.
  */
 unsigned long exec_user_param(unsigned long (*fn)(unsigned long),
                               unsigned long p1);

diff --git a/tests/pv-iopl/asm.S b/tests/pv-iopl/asm.S
index 3ef8fbffea69060cbfc3127a9c21b2f5f0ad52c0..ac1c85d440e520f1be2980b2df8aeb51371dca07 100644 (file)
--- a/tests/pv-iopl/asm.S
+++ b/tests/pv-iopl/asm.S
@@ -1,45 +1,45 @@
 #include <arch/processor.h>
+#include <arch/page.h>
 #include <arch/segment.h>
 #include <xtf/asm_macros.h>
 
 ENTRY(exec_user_with_iopl)      /* void (*fn)(void), unsigned int iopl */
+        push %_ASM_BP
 
-        push $__USER_DS         /* SS */
+        /* Prepare to "call" exec_user_stub(). */
 #ifdef __i386__
-        push %esp
-        addl $4, (%esp)         /* ESP */
-#else
-        push %rsp
-        addq $8, (%rsp)
+        mov (1+1)*4(%esp), %eax /* Pass fn() in %eax */
 #endif
+        push $1f                /* Fake return addr as if we'd called exec_user_stub(). */
+        mov %_ASM_SP, %_ASM_BP  /* Stash %esp for entry_ret_to_kernel(). */
+
+        /* Prepare an IRET frame. */
+        push $__USER_DS         /* SS */
+        push $user_stack + PAGE_SIZE
+                                /* ESP */
         pushf                   /* EFLAGS */
 
                                 /* PV guests see the real interrupt flag. Clobber it. */
         andl $~(X86_EFLAGS_IOPL | X86_EFLAGS_IF), (%_ASM_SP)
 #ifdef __i386__
-        mov  5*4(%esp), %eax
-        shl  $12, %eax
-        or   %eax, (%esp)
+        mov  (5+2)*4(%esp), %ecx
+        shl  $12, %ecx
+        or   %ecx, (%esp)
 #else
         shl  $12, %esi
         or   %esi, (%rsp)
 #endif
 
         push $__USER_CS         /* CS */
-        push $1f                /* EIP */
+        push $exec_user_stub    /* EIP */
 
 #ifdef __x86_64__
         push $0
 #endif
         jmp HYPERCALL_iret      /* Drop to user privilege. */
-1:
-#ifdef __i386__
-        call *4(%esp)           /* fn() */
-#else
-        call *%rdi
-#endif
 
-        int $0x20               /* Return to kernel privilege. */
+1:      /* entry_ret_to_kernel() returns here with a sensible stack. */
+        pop %_ASM_BP
         ret
 
 ENDFUNC(exec_user_with_iopl)

diff --git a/tests/pv-iopl/main.c b/tests/pv-iopl/main.c
index db50dc3b8e13c90ed8accc72827fa446a4f124c0..74f47b6f68065f26df2c53efa7928f89f8a24bd2 100644 (file)
--- a/tests/pv-iopl/main.c
+++ b/tests/pv-iopl/main.c
@@ -47,8 +47,7 @@ const char test_title[] = "PV IOPL emulation";
 bool test_wants_user_mappings = true;
 
 /**
- * Execute @p fn at user privilege on the current stack, folding @p iopl into
- * the iret frame.
+ * Execute @p fn at user privilege, folding @p iopl into the iret frame.
  */
 void exec_user_with_iopl(void (*fn)(void), unsigned int iopl);