Re: [PATCH v2 07/16] KVM: Store gfn_to_pfn_cache length as an immutable property

[Sean Christopherson]

On Mon, Nov 21, 2022, David Woodhouse wrote:
> On Thu, 2022-10-13 at 21:12 +0000, Sean Christopherson wrote:
> > From: Michal Luczaj <mhal@xxxxxxx>
> > 
> > Make the length of a gfn=>pfn cache an immutable property of the cache
> > to cleanup the APIs and avoid potential bugs, e.g calling check() with a
> > larger size than refresh() could put KVM into an infinite loop.
> 
> Hm, that's a strange hypothetical bug to be worried about, given the
> pattern is usually to have the check() and refresh() within a few lines
> of each other with just atomicity/locking stuff in between them.

Why do you say it's strange to be worried about?  The GPC and Xen code is all quite
complex and has had multiple bugs, several of which are not exactly edge cases.
I don't think it's at all strange to want to make it difficult to introduce a bug
that would in many ways be worse than panicking the kernel.

But as Paolo said, the APIs themselves are to blame[*], check() and refresh()
shouldn't be split for the common case, i.e. this particular concern should largely
be a non-issue in the long run.

[*] https://lore.kernel.org/all/c61f6089-57b7-e00f-d5ed-68e62237eab0@xxxxxxxxxx

> I won't fight for it, but I quite liked the idea that each user of a
> GPC would know how much space *it* is going to access, and provide that
> length as a required parameter. I do note you've added a WARN_ON to one
> such user, and that's great — but overall, this patch makes that
> checking *optional* instead of mandatory.

I honestly don't see a meaningful difference in this case.  The only practical
difference is that shoving @len into the cache makes the check a one-time thing.
The "mandatory" check at use time still relies on a human to not make a mistake.
If the check were derived from the actual access, a la get_user(), then I would
feel differently.

Case in point, the mandatory check didn't prevent KVM from screwing up bounds
checking in kvm_xen_schedop_poll().  The PAGE_SIZE passed in for @len is in no
way tied to actual access that's being performed, the code is simply regurgitating
the size of the cache.

> > All current (and anticipated future) users access the cache with a
> > predetermined size, which isn't a coincidence as using a dedicated cache
> > really only make sense when the access pattern is "fixed".
> 
> In fixing up the runstate area, I've made that not true. Not only does
> the runstate area change size at runtime if the guest changes between
> 32-bit and 64-bit mode, but it also now uses *two* GPCs to cope with a
> region that crosses a page boundary, and the size of the first
> therefore changes according to how much fits on the tail of the page.
> 
> > Add a WARN in kvm_setup_guest_pvclock() to assert that the offset+size
> > matches the length of the cache, both to make it more obvious that the
> > length really is immutable in that case, and to detect future bugs.
> ...
> > @@ -3031,13 +3030,13 @@ static void kvm_setup_guest_pvclock(struct kvm_vcpu *v,
> >  	struct pvclock_vcpu_time_info *guest_hv_clock;
> >  	unsigned long flags;
> >  
> > +	WARN_ON_ONCE(gpc->len != offset + sizeof(*guest_hv_clock));
> > +
> 
> That ought to be 'gpc->len < offset + sizeof(*guest_hv_clock)' I think?
> 
> In the case where we are writing a clock *within* a mapped Xen
> vcpu_info structure, it doesn't have to be at the *end* of that
> structure. I think the xen_shinfo_test should have caught that?

The WARN doesn't get false positive because "struct pvclock_vcpu_time_info" is
placed at the end of "struct vcpu_info" and "struct compat_vcpu_info".
 
I don't have a strong opinion on whether it's "!=" of "<", my goal in adding the
WARN was primarily to show that @len really is immutable in this case.  Guarding
against future overrun bugs was a bonus.