Re: [PATCH v6 05/10] Clear AC bit in RFLAGS to protect Xen itself by SMAP

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

On 08/05/2014 03:02, Wu, Feng wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
>> Andrew Cooper
>> Sent: Thursday, May 08, 2014 9:58 AM
>> To: Wu, Feng; Jan Beulich
>> Cc: ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian, Kevin;
>> xen-devel@lists.xen.org
>> Subject: Re: [PATCH v6 05/10] Clear AC bit in RFLAGS to protect Xen itself by
>> SMAP
>>
>> On 08/05/2014 02:41, Wu, Feng wrote:
>>>> -----Original Message-----
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: Wednesday, May 07, 2014 7:54 PM
>>>> To: Jan Beulich
>>>> Cc: Wu, Feng; ian.campbell@citrix.com; Dong, Eddie; Nakajima, Jun; Tian,
>>>> Kevin; xen-devel@lists.xen.org
>>>> Subject: Re: [PATCH v6 05/10] Clear AC bit in RFLAGS to protect Xen itself by
>>>> SMAP
>>>>
>>>> On 07/05/14 12:40, Jan Beulich wrote:
>>>>>>>> On 07.05.14 at 11:44, <andrew.cooper3@citrix.com> wrote:
>>>>>> On 07/05/14 09:19, Feng Wu wrote:
>>>>>>> @@ -673,6 +675,7 @@ ENTRY(nmi_crash)
>>>>>>>          ud2
>>>>>>>
>>>>>>>  ENTRY(machine_check)
>>>>>>> +        ASM_CLAC
>>>>>> This is not needed.  the start of handle_ist_exception has a SAVE_ALL,
>>>>>> which also covers the nmi entry point.
>>>>>>
>>>>>> On the subject of IST exceptions, perhaps the double fault explicitly
>>>>>> wants a STAC to reduce the likelihood of taking a further fault while
>>>>>> trying to dump state. ?
>>>>> I agree. And perhaps along with do_double_fault(), fatal_trap()
>>>>> should then also get a stac() added?
>>>>>
>>>>> Jan
>>>>>
>>>> With doubt_fault: being sole caller of do_double_fault(), editing the
>>>> entry point in entry.S to "ASM_STAC; SAVE_ALL 0" is sufficient to avoid
>>>> stac() in do_doube_fault() itself.
>>> I think it's better to add "ASM_STAC" just before " call  do_double_fault".
>>> Do you think this is okay, Andrew? Thanks!
>> I am not fussed where exactly the STAC goes in the entry point, but
>> don't leave a CLAC in the SAVE_ALL.
> Sure, thanks for the suggestion! I will pass 0 to SAVE_ALL. 
> My point is that ASM_STAC should be deferred as much as possible.

In the double_fault entry, currently only way to get an SMAP violation
would if the Interrupt Stack Table mechanism landed us on a stack with
user mapings, at which point all bets are already off with respect to
dumping state and rebooting.

In the case that our stack is actually good, a pagefault from trying to
peek at boot_cpu_data would land us in a loop of nested pagefaults until
we wandered off the stack, as each entry point attempted to CLAC.

>From a safety point of view clearing CR4.SMAP would be the best action,
as we can manage that safely on the stack before looking outside.

On the otherhand, if we fault when looking at boot_cpu_data, the chances
of successfully calling do_page_fault() is tantamount to 0.

In the past while hacking^W debugging, I have noticed that the double
fault handling in Xen does have a tendency to end itself up in infinite
loops.  I don't think this series is the appropriate place to sure this
up (I might find some particularly distant copious free time), although
not leaving it in a worse state than it currently is would be nice.

~Andrew