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

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

[phind.uet]

From: Nguyen Dinh Phi <phind.uet@gmail.com>

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.

An invalid uhva could lead to incorrect offset calculations and potentially
trigger a WARN_ON_ONCE in __kvm_gpc_refresh().

Fixing it by adding an additional check for uhva.

Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Reported-by: syzbot+cde12433b6c56f55d9ed@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cde12433b6c56f55d9ed

---
 virt/kvm/pfncache.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c
index 728d2c1b488a..707ead0a096c 100644
--- a/virt/kvm/pfncache.c
+++ b/virt/kvm/pfncache.c
@@ -60,8 +60,16 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
 static bool kvm_gpc_is_valid_len(gpa_t gpa, unsigned long uhva,
 				 unsigned long len)
 {
-	unsigned long offset = kvm_is_error_gpa(gpa) ? offset_in_page(uhva) :
-						       offset_in_page(gpa);
+	unsigned long offset;
+
+	if (kvm_is_error_gpa(gpa)) {
+		if (kvm_is_error_hva(uhva))
+			return false;
+
+		offset = offset_in_page(uhva);
+	} else {
+		offset = offset_in_page(gpa);
+	}
 
 	/*
 	 * The cached access must fit within a single page. The 'len' argument
-- 
2.43.0

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

[Sean Christopherson]

On Mon, Mar 09, 2026, phind.uet@gmail.com wrote:
> From: Nguyen Dinh Phi <phind.uet@gmail.com>
> 
> 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;

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

[Phi Nguyen]

On 3/9/2026 10:39 PM, Sean Christopherson wrote:
> On Mon, Mar 09, 2026, phind.uet@gmail.com wrote:
>> From: Nguyen Dinh Phi <phind.uet@gmail.com>
>>
>> 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()?
Br,
Phi

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@gmail.com wrote:
> > > From: Nguyen Dinh Phi <phind.uet@gmail.com>
> > > 
> > > 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.