Re: [PATCH] x86: refine guest_mode()

[Roger Pau Monné <roger@xxxxxxx>]

On Tue, May 26, 2020 at 03:55:39PM +0200, Jan Beulich wrote:
> On 26.05.2020 12:56, Roger Pau Monné wrote:
> > On Fri, May 22, 2020 at 02:00:22PM +0200, Jan Beulich wrote:
> >> On 22.05.2020 12:48, Roger Pau Monné wrote:
> >>> On Fri, May 22, 2020 at 11:52:42AM +0200, Jan Beulich wrote:
> >>>> On 20.05.2020 17:13, Roger Pau Monné wrote:
> >>>>> 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).
> >>>>
> >>>> Technically this would be correct afaics, but the idea with all this
> >>>> is (or should I say "looks to be"?) to have the checks be as tight as
> >>>> possible, to make sure we don't mistakenly consider something "guest
> >>>> mode" which really isn't. IOW your suggestion would be fine with me
> >>>> if we could exclude bugs anywhere in the code. But since this isn't
> >>>> realistic, I consider your suggestion to be relaxing things by too
> >>>> much.
> >>>
> >>> OK, so I take that (long time) we might also want to change the cs & 3
> >>> checks from entry.S to check against __HYPERVISOR_CS explicitly?
> >>
> >> I didn't think so, no (not the least because of there not being any
> >> guarantee afaik that EFI runtime calls couldn't play with segment
> >> registers; they shouldn't, yes, but there's a lot of other "should"
> >> many don't obey to). Those are guaranteed PV-only code paths. The
> >> main issue here is that ->cs cannot be relied upon when a frame
> >> points at HVM state.
> > 
> > Well, if it points at HVM state it could equally have __HYPERVISOR_CS
> > set by the guest.
> 
> No, that's not the point. ->cs will never be __HYPERVISOR_CS in that
> case, as we never store the guest's CS selector there. Instead
> hvm_invalidate_regs_fields() clobbers the field in debug builds (with
> a value resulting in RPL 3), but zero (i.e. a value implying RPL 0)
> remains in place in release builds.
> 
> Instead of doing this clobbering in debug mode only, we could - as I
> think I did suggest before - clobber always, but just once during vCPU
> init rather than on every VM exit. In debug mode we could then instead
> check that the dummy values didn't themselves get clobbered.

It would make sense to clobber it always with a value that has RPL >
0, so that it's consistent with PV state.

> > Will things work anyway if you get here from an exception generated by
> > EFI code that has changed the code segment? You are going to hit the
> > assert at least, since diff will be != 0 and cs != __HYPERVISOR_CS?
> 
> What would guarantee the latter? Additionally they could in principle
> also have switched stacks then, i.e. diff may then also be larger than
> PRIMARY_STACK_SIZE, in which case - with the patch in place - the
> assertion is bypassed altogether.
> 
> > I would prefer to keep things coherent by either using cs & 3 or
> > cs == __HYPERVISOR_CS everywhere if possible, as I'm still unsure of
> > the benefit of using __HYPERVISOR_CS.
> 
> See above.

Well, I think it's an improvement overall, as it allows to properly
handle the case where a PV guest could manage to trigger an exception
that uses a stack different than the primary one.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Roger.