* [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() @ 2026-03-09 7:56 phind.uet 2026-03-09 14:39 ` Sean Christopherson 0 siblings, 1 reply; 4+ messages in thread From: phind.uet @ 2026-03-09 7:56 UTC (permalink / raw) To: Paolo Bonzini Cc: Nguyen Dinh Phi, syzbot+cde12433b6c56f55d9ed, kvm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() 2026-03-09 7:56 [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() phind.uet @ 2026-03-09 14:39 ` Sean Christopherson 2026-03-09 16:02 ` Phi Nguyen 0 siblings, 1 reply; 4+ messages in thread From: Sean Christopherson @ 2026-03-09 14:39 UTC (permalink / raw) To: phind.uet; +Cc: Paolo Bonzini, syzbot+cde12433b6c56f55d9ed, kvm, linux-kernel 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; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() 2026-03-09 14:39 ` Sean Christopherson @ 2026-03-09 16:02 ` Phi Nguyen 2026-03-09 19:39 ` Sean Christopherson 0 siblings, 1 reply; 4+ messages in thread From: Phi Nguyen @ 2026-03-09 16:02 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, syzbot+cde12433b6c56f55d9ed, kvm, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() 2026-03-09 16:02 ` Phi Nguyen @ 2026-03-09 19:39 ` Sean Christopherson 0 siblings, 0 replies; 4+ messages in thread From: Sean Christopherson @ 2026-03-09 19:39 UTC (permalink / raw) To: Phi Nguyen; +Cc: Paolo Bonzini, syzbot+cde12433b6c56f55d9ed, kvm, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-03-09 19:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-09 7:56 [PATCH] KVM: pfncache: Fix uhva validity check in kvm_gpc_is_valid_len() phind.uet 2026-03-09 14:39 ` Sean Christopherson 2026-03-09 16:02 ` Phi Nguyen 2026-03-09 19:39 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox