Re: [Xen-devel] [PATCH v2 2/7] 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: Wednesday, April 23, 2014 6:20 PM
> To: Wu, Feng
> Cc: xen-devel@xxxxxxxxxxxxx; JBeulich@xxxxxxxx; Tian, Kevin; Dong, Eddie;
> Nakajima, Jun; ian.campbell@xxxxxxxxxx
> Subject: Re: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by 
> SMAP
> 
> 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@xxxxxxxxx>
> > ---
> >  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 ?

Okay.

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

The MSR_SYSCALL_MASK is common in long mode and compat mode, right?
Seems no need to do anything else for compat mode.

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

Adding CLAC here is not so useful, but harmful neither. If you think it should 
be
removed, I will do that in the next post.

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

The same comments as early_page_fault case.

> >
> >
> > 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"                 \

Thanks,
Feng

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