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

[David Woodhouse]

On Mon, 2022-11-21 at 19:11 +0000, Sean Christopherson wrote:
> 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.

The check() and refresh() calls are within a few lines of each other,
and it'd be really strange for them to have a *different* idea about
what the length is, surely?

> 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

Yeah. As I said to Paul, I've been tempted by that. I've so far not
done it because although they look broadly similar, a bunch of the
sites do end up with *different* code between the check() and the
refresh(), for various locking and atomicity reasons.

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

True, but that's a different class of bug, and the human needs to make
a more *egregious* mistake.

If the function itself writes outside the size that *it* thinks *it's*
going to write, right there and then in that function, that's utterly
hosed (and the SCHEDOP_poll thing was indeed so hosed).

The mandatory check *did* save us from configuring a 32-bit runstate
area at the end of a page, then *writing* to it in 64-bit mode (where
it's larger) and running off the end of the page.

It saved us from "knowing", a few seconds ago under different
circumstances, what the size of the runstate area was... and then it
actually being different when it's written.

But that's not the common case, so again, I won't fight for it.

I've reworked the unapplied parts of this series on top of the poll and
runstate fixes in my tree, *except* for this one making the length
immutable, and I'm running some tests.

https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/gpc-fixes

I'm happy to reinstate the immutable length thing in some form on top.

Given that the runstate code already calculates for itself how many
bytes it can fit onto the first page, it really doesn't care about the
length field in the GPC. As a nasty hack, the runstate code could
probably even get away with setting 'len' to zero. That's kind of
awful, but maybe we could introduce a __kvm_gpc_activate() which does
take a new length, leaving kvm_gpc_activate() without it?

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

Ah right, I think I was looking at the pvclock_wall_clock field in the
shared_info, not the time field in the vcpu_info.

Attachment: smime.p7s
Description: S/MIME cryptographic signature