Re: [PATCH] x86: refine guest_mode()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/04/2020 16:15, Jan Beulich wrote:
> On 27.04.2020 16:35, Andrew Cooper wrote:
>> On 27/04/2020 09:03, Jan Beulich wrote:
>>> The 2nd of the assertions as well as the macro's return value have been
>>> assuming we're on the primary stack. While for most IST exceptions we
>>> eventually switch back to the main one,
>> "we switch to the main one when interrupting user mode".
>>
>> "eventually" isn't accurate as it is before we enter C.
> Right, will change.
>
>>> --- a/xen/include/asm-x86/regs.h
>>> +++ b/xen/include/asm-x86/regs.h
>>> @@ -10,9 +10,10 @@
>>>      /* Frame pointer must point into current CPU stack. */                 
>>>    \
>>>      ASSERT(diff < STACK_SIZE);                                             
>>>    \
>>>      /* If not a guest frame, it must be a hypervisor frame. */             
>>>    \
>>> -    ASSERT((diff == 0) || (r->cs == __HYPERVISOR_CS));                     
>>>    \
>>> +    if ( diff < PRIMARY_STACK_SIZE )                                       
>>>    \
>>> +        ASSERT(!diff || ((r)->cs == __HYPERVISOR_CS));                     
>>>    \
>>>      /* Return TRUE if it's a guest frame. */                               
>>>    \
>>> -    (diff == 0);                                                           
>>>    \
>>> +    !diff || ((r)->cs != __HYPERVISOR_CS);                                 
>>>    \
>> The (diff == 0) already worried me before because it doesn't fail safe,
>> but this makes things more problematic.  Consider the case back when we
>> had __HYPERVISOR_CS32.
> Yes - if __HYPERVISOR_CS32 would ever have been to be used for
> anything, it would have needed checking for here.
>
>> Guest mode is strictly "(r)->cs & 3".
> As long as CS (a) gets properly saved (it's a "manual" step for
> SYSCALL/SYSRET as well as #VMEXIT) and (b) didn't get clobbered. I
> didn't write this code, I don't think, so I can only guess that
> there were intentions behind this along these lines.

Hmm - the VMExit case might be problematic here, due to the variability
in the poison used.

>
>> Everything else is expectations about how things ought to be laid out,
>> but for safety in release builds, the final judgement should not depend
>> on the expectations evaluating true.
> Well, I can switch to a purely CS.RPL based approach, as long as
> we're happy to live with the possible downside mentioned above.
> Of course this would then end up being a more intrusive change
> than originally intended ...

I'd certainly prefer to go for something which is more robust, even if
it is a larger change.

~Andrew