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: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Thursday, April 24, 2014 3:19 PM
> To: Wu, Feng
> Cc: Andrew Cooper; ian.campbell@xxxxxxxxxx; Dong, Eddie; Nakajima, Jun; Tian,
> Kevin; xen-devel@xxxxxxxxxxxxx
> Subject: RE: [PATCH v2 2/7] Clear AC bit in RFLAGS to protect Xen itself by 
> SMAP
> 
> >>> On 24.04.14 at 08:45, <feng.wu@xxxxxxxxx> wrote:
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> On 23/04/14 15:35, Feng Wu wrote:
> >> > --- 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.
> 
> The same stub is being written there twice, just with different CS
> selectors, and both are behind that EFLAGS masking MSR. So I'm not
> sure what Andrew's apparently incomplete sentence was actually
> intended to mean.

Yes, that is also my understanding.

> 
> >> > @@ -268,6 +269,7 @@ bad_hypercall:
> >> >          jmp  test_all_events
> >> >
> >> >  ENTRY(sysenter_entry)
> >> > +        ASM_CLAC
> >> >          sti
> >> >          pushq $FLAT_USER_SS
> >> >          pushq $0
> 
> Looking at this again, btw, makes me thing that the clac should go
> after the sti here.
> 
> >> > @@ -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.
> 
> Yes, let's not scatter it around pointlessly (even more so now that
> you plan on enabling SMAP only after having built Dom0).
> 
> >> > @@ -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.
> >
> > The same comments as early_page_fault case.
> 
> The situation is different here, but the addition indeed doesn't seem
> to make sense.
> 
> Jan

Thanks,
Feng

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