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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/04/14 08:20, Wu, Feng wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, April 24, 2014 3:19 PM
>> To: Wu, Feng
>> Cc: Andrew Cooper; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian,
>> Kevin; xen-devel@lists.xen.org
>> 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@intel.com> wrote:
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> 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.

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

~Andrew