Re: [PATCH] x86: refine guest_mode()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Apr 28, 2020 at 08:30:12AM +0200, Jan Beulich wrote:
> 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).

Just to check I'm following this correctly, using CS.RPL won't work
for HVM guests, as HVM can legitimately use a RPL of 0 (which is not
the case for PV guests). Doesn't the same apply to the usage of
__HYPERVISOR_CS? (A HVM guest could also use the same code segment
value as Xen?)

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

Fragile or slow? Are there corner cases where guest_mode is used where
current is not reliable?

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

Using the cs register is already part of the guest_mode code, even if
just in debug mode, hence I don't see it as a regression from existing
code. It however feels weird to me that the reporter of the issue
doesn't agree with the fix, and hence would like to know if there's a
way we could achieve consensus on this.

Roger.