Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by SMAP

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/04/14 15:35, Feng Wu wrote:
> Clear AC bit in RFLAGS at the beginning of exception, interrupt, hypercall,
> so Xen itself can be protected by SMAP mechanism.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/x86_64/compat/entry.S     |  1 +
>  xen/arch/x86/x86_64/entry.S            | 26 ++++++++++++++++++++++++++
>  xen/include/asm-x86/x86_64/asm_defns.h |  1 +
>  3 files changed, 28 insertions(+)
>
> diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
> index 32b3bcc..ac594c9 100644
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -13,6 +13,7 @@
>  #include <irq_vectors.h>
>  
>  ENTRY(compat_hypercall)
> +        ASM_CLAC
>          pushq $0
>          SAVE_VOLATILE type=TRAP_syscall compat=1
>  
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 3ea4683..d294064 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -120,6 +120,7 @@ restore_all_xen:
>   * the space left by the trampoline.
>   */
>  ENTRY(syscall_enter)
> +        ASM_CLAC

Surely this can be sorted more succinctly by setting X86_EFLAGS_AC in
MSR 0xc0000084 ?

You also need to patch the entry points in the compat trampoline in

>          sti
>          movl  $FLAT_KERNEL_SS,24(%rsp)
>          pushq %rcx
> @@ -268,6 +269,7 @@ bad_hypercall:
>          jmp  test_all_events
>  
>  ENTRY(sysenter_entry)
> +        ASM_CLAC
>          sti
>          pushq $FLAT_USER_SS
>          pushq $0
> @@ -309,6 +311,7 @@ UNLIKELY_END(sysenter_gpf)
>          jmp   .Lbounce_exception
>  
...
>  
>          .pushsection .init.text, "ax", @progbits
>  ENTRY(early_page_fault)
> +        ASM_CLAC

I don't think CLAC is appropriate here.  This is a pagefault handler for
Xen early boot, and is replaced with a real handler substantially before
dom0 is created.

>          SAVE_ALL
>          movq  %rsp,%rdi
>          call  do_early_page_fault
> @@ -622,6 +644,7 @@ ENTRY(early_page_fault)
>          .popsection
>  
>  ENTRY(nmi)
> +        ASM_CLAC
>          pushq $0
>          movl  $TRAP_nmi,4(%rsp)
>  handle_ist_exception:
> @@ -659,6 +682,7 @@ handle_ist_exception:
>          jmp   compat_restore_all_guest
>  
>  ENTRY(nmi_crash)
> +        ASM_CLAC
>          pushq $0
>          movl $TRAP_nmi,4(%rsp)
>          SAVE_ALL
> @@ -667,6 +691,7 @@ ENTRY(nmi_crash)
>          ud2
>  
>  ENTRY(machine_check)
> +        ASM_CLAC
>          pushq $0
>          movl  $TRAP_machine_check,4(%rsp)
>          jmp   handle_ist_exception
> @@ -689,6 +714,7 @@ ENTRY(enable_nmis)
>  
>  /* No op trap handler.  Required for kexec crash path. */
>  GLOBAL(trap_nop)
> +        ASM_CLAC
>          iretq

This is not sensible in the slightest, given the following instruction.

~Andrew

>  
>  
> diff --git a/xen/include/asm-x86/x86_64/asm_defns.h b/xen/include/asm-x86/x86_64/asm_defns.h
> index bf63ac1..69f76b2 100644
> --- a/xen/include/asm-x86/x86_64/asm_defns.h
> +++ b/xen/include/asm-x86/x86_64/asm_defns.h
> @@ -212,6 +212,7 @@
>  __asm__(                                        \
>      "\n" __ALIGN_STR"\n"                        \
>      "common_interrupt:\n\t"                     \
> +    ASM_CLAC(%)"\n\t"                           \
>      STR(SAVE_ALL) "\n\t"                        \
>      "movq %rsp,%rdi\n\t"                        \
>      "callq " STR(do_IRQ) "\n\t"                 \