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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Thursday, May 08, 2014 6:07 PM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx; JBeulich@xxxxxxxx; Tian, Kevin; Dong, Eddie;
> Nakajima, Jun; ian.campbell@xxxxxxxxxx
> Subject: Re: [PATCH v7 2/7] x86: Clear AC bit in RFLAGS to protect Xen itself 
> by
> SMAP
> 
> 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@xxxxxxxxx>
> > ---
> >  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

That's a good suggestion!

Thanks,
Feng

> 
> >            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)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel