Re: [PATCH] x86: refine guest_mode()

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan