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
