| Message ID | 20240320001542.3203871-3-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Fix for a mostly benign gpc WARN | expand |
On Tue, 2024-03-19 at 17:15 -0700, Sean Christopherson wrote: > When activating a gfn_to_pfn_cache, verify that the offset+length is sane > and usable before marking the cache active. Letting __kvm_gpc_refresh() > detect the problem results in a cache being marked active without setting > the GPA (or any other fields), which in turn results in KVM trying to > refresh a cache with INVALID_GPA. > > Attempting to refresh a cache with INVALID_GPA isn't functionally > problematic, but it runs afoul of the sanity check that exactly one of > GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or > HVA-based. > > Reported-by: syzbot+106a4f72b0474e1d1b33@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/0000000000005fa5cc0613f1cebd@google.com > Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA") Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
On 20/03/2024 00:15, Sean Christopherson wrote: > When activating a gfn_to_pfn_cache, verify that the offset+length is sane > and usable before marking the cache active. Letting __kvm_gpc_refresh() > detect the problem results in a cache being marked active without setting > the GPA (or any other fields), which in turn results in KVM trying to > refresh a cache with INVALID_GPA. > > Attempting to refresh a cache with INVALID_GPA isn't functionally > problematic, but it runs afoul of the sanity check that exactly one of > GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or > HVA-based. > > Reported-by: syzbot+106a4f72b0474e1d1b33@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/0000000000005fa5cc0613f1cebd@google.com > Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA") > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Paul Durrant <paul@xen.org> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > virt/kvm/pfncache.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > Reviewed-by: Paul Durrant <paul@xen.org>
diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 8f2121b5f2a0..91b0e329006b 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -245,8 +245,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) return -EFAULT; } -static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva, - unsigned long len) +static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long uhva) { unsigned long page_offset; bool unmap_old = false; @@ -260,9 +259,6 @@ static int __kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned l if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva))) return -EINVAL; - if (!kvm_gpc_is_valid_len(gpa, uhva, len)) - return -EINVAL; - lockdep_assert_held(&gpc->refresh_lock); write_lock_irq(&gpc->lock); @@ -365,6 +361,9 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len) guard(mutex)(&gpc->refresh_lock); + if (!kvm_gpc_is_valid_len(gpc->gpa, gpc->uhva, len)) + return -EINVAL; + /* * If the GPA is valid then ignore the HVA, as a cache can be GPA-based * or HVA-based, not both. For GPA-based caches, the HVA will be @@ -372,7 +371,7 @@ int kvm_gpc_refresh(struct gfn_to_pfn_cache *gpc, unsigned long len) */ uhva = kvm_is_error_gpa(gpc->gpa) ? gpc->uhva : KVM_HVA_ERR_BAD; - return __kvm_gpc_refresh(gpc, gpc->gpa, uhva, len); + return __kvm_gpc_refresh(gpc, gpc->gpa, uhva); } void kvm_gpc_init(struct gfn_to_pfn_cache *gpc, struct kvm *kvm) @@ -392,6 +391,9 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned { struct kvm *kvm = gpc->kvm; + if (!kvm_gpc_is_valid_len(gpa, uhva, len)) + return -EINVAL; + guard(mutex)(&gpc->refresh_lock); if (!gpc->active) { @@ -411,7 +413,7 @@ static int __kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned gpc->active = true; write_unlock_irq(&gpc->lock); } - return __kvm_gpc_refresh(gpc, gpa, uhva, len); + return __kvm_gpc_refresh(gpc, gpa, uhva); } int kvm_gpc_activate(struct gfn_to_pfn_cache *gpc, gpa_t gpa, unsigned long len)
When activating a gfn_to_pfn_cache, verify that the offset+length is sane and usable before marking the cache active. Letting __kvm_gpc_refresh() detect the problem results in a cache being marked active without setting the GPA (or any other fields), which in turn results in KVM trying to refresh a cache with INVALID_GPA. Attempting to refresh a cache with INVALID_GPA isn't functionally problematic, but it runs afoul of the sanity check that exactly one of GPA or userspace HVA is valid, i.e. that a cache is either GPA-based or HVA-based. Reported-by: syzbot+106a4f72b0474e1d1b33@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/0000000000005fa5cc0613f1cebd@google.com Fixes: 721f5b0dda78 ("KVM: pfncache: allow a cache to be activated with a fixed (userspace) HVA") Cc: David Woodhouse <dwmw2@infradead.org> Cc: Paul Durrant <paul@xen.org> Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/pfncache.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)