Re: [PATCH] x86: refine guest_mode()

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

On Wed, May 20, 2020 at 10:56:26AM +0200, Jan Beulich wrote:
> On 18.05.2020 16:51, Roger Pau Monné wrote:
> > 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:
> >>>>>> --- 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?)
> 
> Of course (and in particular Xen as a guest would). My "Basing on
> _just_ CS.RPL" wasn't meant to exclude the rest of the selector,
> but to contrast this to the case where "diff" also is involved in
> the calculation (which looks to be what Andrew would prefer to see
> go away).
> 
> >> 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?
> 
> This question is why I said "there are quite a few uses of
> guest_mode()" - auditing them all is just one side of the medal.
> The other is to prevent a new use appearing in the future that
> can be reached by a call path in the time window where a lazy
> context switch is pending (i.e. when current has already been
> updated, but register state hasn't been yet).
> 
> >> 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.
> 
> Indeed. I'd be happy to make further adjustments, if only I had a
> clear understanding of what is wanted (or why leaving things as
> they are is better than a little bit of an improvement).

OK, so I think I'm starting to understand this all. Sorry it's taken
me so long. So it's my understanding that diff != 0 can only happen in
Xen context, or when in an IST that has a different stack (ie: MCE, NMI
or DF according to current.h) and running in PV mode?

Wouldn't in then be fine to use (r)->cs & 3 to check we are in guest
mode if diff != 0? I see a lot of other places where cs & 3 is already
used to that effect AFAICT (like entry.S).

Roger.