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

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

On 08/05/14 10:21, Feng Wu wrote:
> Clear AC bit in RFLAGS at the beginning of exception, interrupt, hypercall,
> so Xen itself can be protected by SMAP mechanism. This patch also sets AC
> bit at the beginning of double_fault and fatal_trap() to reduce the likelihood
> of taking a further fault while trying to dump state.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  xen/arch/x86/acpi/suspend.c        |  2 +-
>  xen/arch/x86/traps.c               |  6 ++++++
>  xen/arch/x86/x86_64/compat/entry.S |  3 ++-
>  xen/arch/x86/x86_64/entry.S        | 12 +++++++++---
>  xen/arch/x86/x86_64/traps.c        |  2 +-
>  xen/include/asm-x86/asm_defns.h    | 15 ++++++++++++++-
>  6 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
> index a373e9a..acf667d 100644
> --- a/xen/arch/x86/acpi/suspend.c
> +++ b/xen/arch/x86/acpi/suspend.c
> @@ -57,7 +57,7 @@ void restore_rest_processor_state(void)
>      wrmsrl(MSR_CSTAR, saved_cstar);
>      wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
>      wrmsr(MSR_SYSCALL_MASK,
> -          X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
> +          X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
>            X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_TF,

As this is written in two locations and *really* needs to be the same,
can I suggest a

#define XEN_SYSCALL_MASK (X86_EFLAGS_VM | ... )

In processor.h

>            0U);
>  
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 5d27581..1082270 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -402,6 +402,12 @@ void fatal_trap(int trapnr, struct cpu_user_regs *regs)
>      static DEFINE_PER_CPU(char, depth);
>  
>      /*
> +     * Set AC bit to reduce the likelihood of taking a further fault
> +     * while trying to dump state.
> +     */
> +    stac();
> +
> +    /*
>       * In some cases, we can end up in a vicious cycle of fatal_trap()s
>       * within fatal_trap()s. We give the problem a couple of iterations to
>       * bottom out, and then we just panic.
> diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
> index 32b3bcc..1d1d3d6 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
>  
> @@ -178,7 +179,7 @@ ENTRY(compat_restore_all_guest)
>  
>  .section .fixup,"ax"
>  .Lfx0:  sti
> -        SAVE_ALL
> +        SAVE_ALL 2
>          movq  UREGS_error_code(%rsp),%rsi
>          movq  %rsp,%rax
>          andq  $~0xf,%rsp
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 1c81852..ed7b96f 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -68,7 +68,7 @@ iret_exit_to_guest:
>  
>  .section .fixup,"ax"
>  .Lfx0:  sti
> -        SAVE_ALL
> +        SAVE_ALL 2
>          movq  UREGS_error_code(%rsp),%rsi
>          movq  %rsp,%rax
>          andq  $~0xf,%rsp
> @@ -273,6 +273,7 @@ ENTRY(sysenter_entry)
>          pushq $0
>          pushfq
>  GLOBAL(sysenter_eflags_saved)
> +        ASM_CLAC
>          pushq $3 /* ring 3 null cs */
>          pushq $0 /* null rip */
>          pushq $0
> @@ -309,6 +310,7 @@ UNLIKELY_END(sysenter_gpf)
>          jmp   .Lbounce_exception
>  
>  ENTRY(int80_direct_trap)
> +        ASM_CLAC
>          pushq $0
>          SAVE_VOLATILE 0x80
>  
> @@ -614,14 +616,18 @@ ENTRY(spurious_interrupt_bug)
>  
>  ENTRY(double_fault)
>          movl  $TRAP_double_fault,4(%rsp)
> -        SAVE_ALL
> +        /*
> +         * Set AC bit to reduce the likelihood of taking a further fault
> +         * while trying to dump state.
> +         */

Comments like this are useful, but could easily be much shorter.

/* Set AC to reduce chance of further SMAP faults */

The fact this is the double_fault handler makes it clear that we are
dumping state.  The same goes for fatal_trap()

~Andrew

> +        SAVE_ALL 1
>          movq  %rsp,%rdi
>          call  do_double_fault
>          ud2
>  
>          .pushsection .init.text, "ax", @progbits
>  ENTRY(early_page_fault)
> -        SAVE_ALL
> +        SAVE_ALL 2
>          movq  %rsp,%rdi
>          call  do_early_page_fault
>          jmp   restore_all_xen
> diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
> index 90072c1..b87b33e 100644
> --- a/xen/arch/x86/x86_64/traps.c
> +++ b/xen/arch/x86/x86_64/traps.c
> @@ -437,7 +437,7 @@ void __devinit subarch_percpu_traps_init(void)
>      /* Common SYSCALL parameters. */
>      wrmsr(MSR_STAR, 0, (FLAT_RING3_CS32<<16) | __HYPERVISOR_CS);
>      wrmsr(MSR_SYSCALL_MASK,
> -          X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
> +          X86_EFLAGS_AC|X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT|
>            X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_TF,
>            0U);
>  }
> diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
> index b75905a..ce2d89b 100644
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -190,7 +190,20 @@ static inline void stac(void)
>  #endif
>  
>  #ifdef __ASSEMBLY__
> -.macro SAVE_ALL
> +/*
> + * Save all registers.
> + *
> + * @ac:
> + *     0 - claer AC bit
> + *     1 - set AC bit
> + *     others - don't modify AC bit
> + */
> +.macro SAVE_ALL ac=0
> +.if \ac == 0
> +        ASM_CLAC
> +.elseif \ac == 1
> +        ASM_STAC
> +.endif
>          addq  $-(UREGS_error_code-UREGS_r15), %rsp
>          cld
>          movq  %rdi,UREGS_rdi(%rsp)