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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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


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