Re: [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len()

[Sean Christopherson]

On Tue, Mar 10, 2026, Phi Nguyen wrote:
> On 3/9/2026 10:39 PM, Sean Christopherson wrote:
> > On Mon, Mar 09, 2026, phind.uet@xxxxxxxxx wrote:
> > > From: Nguyen Dinh Phi <phind.uet@xxxxxxxxx>
> > > 
> > > In kvm_gpc_is_valid_len(), if the GPA is an error GPA, the function uses
> > > uhva to calculate the page offset. However, if uhva is invalid, its value
> > > can still be page-aligned (for example, PAGE_OFFSET) and this function will
> > > still return true.
> > 
> > The HVA really shouldn't be invalid in the first place.  Ideally, Xen code wouldn't
> > call kvm_gpc_refresh() on an inactive cache, but I suspect we'd end up with TOCTOU
> > flaws even if we tried to add checks.
> > 
> > The next best thing would be to explicitly check if the gpc is active.  That should
> > preserve the WARN if KVM tries to pass in a garbage address to __kvm_gpc_activate().
> > 
> > diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
> > index 728d2c1b488a..8372d1712471 100644
> > --- a/virt/kvm/pfncache.c
> > +++ b/virt/kvm/pfncache.c
> > @@ -369,6 +369,9 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len)
> >          guard(mutex)(&gpc->refresh_lock);
> > +       if (!gpc->active)
> > +               return -EINVAL;
> > +
> >          if (!kvm_gpc_is_valid_len(gpc->gpa, gpc->uhva, len))
> >                  return -EINVAL;
> In this reproducer, userspace invokes KVM_XEN_HVM_EVTCHN_SEND without first
> configuring the cache. As a result, kvm_xen_set_evtchn_fast() returns
> -EWOULDBLOCK when kvm_gpc_check() fails. The -EWOULDBLOCK error then causes
> kvm_xen_set_evtchn() to fall back to calling kvm_gpc_refresh().
> 
> IMO, if the cache is not active, kvm_xen_set_evtchn_fast() should return
> -EINVAL instead. It may be better to check the active state of the GPC in
> kvm_xen_set_evtchn_fast() rather than kvm_gpc_refresh()?

That'd be subject to the TOCTOU race I mentioned.  gpc->active is guarded by
gpc->refresh_lock, which as the name suggests is taken only by __kvm_gpc_activate(),
kvm_gpc_deactivate(), and kvm_gpc_refresh().  Checking gpc->active outside of
those paths can get false positives, e.g. in this case if there's a racing call
to deactivate a cache via KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA.

So no matter what, kvm_gpc_refresh() needs to check gpc->active.  At that point,
I don't see any value in having callers check, because they can't be trusted to
do the right thing, and even worse might give a false sense of security.