Re: [PATCH] x86: refine guest_mode()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2020 22:11, Andrew Cooper wrote:
> 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.

"Variability" is an understatement - there's no poisoning at all
in release builds afaics (and to be honest it seems a somewhat
pointless to write the same values over and over again in debug
mode). With this, ...

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

... what's your suggestion? Basing on _just_ CS.RPL obviously won't
work. Not even if we put in place the guest's CS (albeit that
somewhat depends on the meaning we assign to the macro's returned
value). Using current inside the macro to determine whether the
guest is HVM would also seem fragile to me - there are quite a few
uses of guest_mode(). Which would leave passing in a const struct
vcpu * (or domain *), requiring to touch all call sites, including
Arm's.

Compared to this it would seem to me that the change as presented
is a clear improvement without becoming overly large of a change.

Jan