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: Thursday, April 24, 2014 6:51 PM
> To: Wu, Feng
> Cc: Jan Beulich; 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 08:20, Wu, Feng wrote:
> >
> >> -----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.
> 
> It was my mistake - I got the sysenter and syscall entry points confused
> given a brief look at the code yesterday.  On more careful review it is
> currently fine.
> 
> >
> >>>>> @@ -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.
> 
> It must be after sysenter_eflags_saved, or we will erroneously clear the
> AC flag from the flags used to restore guest context.

Great comments, will follow it!

> 
> >>
> >>>>> @@ -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.
> 
> The handler is used to prevent NMIs and MCEs getting in the way of the
> crash path.  Slowing down the return to the crash path is counter
> productive in all cases.
> 

Got it, thanks a lot for the clear explanation!

> ~Andrew

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