[PATCH 0/2] KVM: guest_memfd: Handle xarray binding errors

[PATCH 0/2] KVM: guest_memfd: Handle xarray binding errors

[Sean Christopherson]

Handle errors when inserting into guest_memfd's binding xarray, e.g. to
do the right thing on ENOMEM.

Patch 2 is a related cleanup to remove a superflous WRITE_ONCE() (unwinding
the slot update on insertion failure isn't an option if the slot is observable,
i.e. if the WRITE_ONCE() is actually necessary).

Sean Christopherson (2):
  KVM: guest_memfd: Gracefully handle xarray errors when binding a
    memslot
  KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

 virt/kvm/guest_memfd.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)


base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 1/2] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[Sean Christopherson]

If inserting a memslot into a guest_memfd's bindings xarray fails,
propagate the error back to the caller, i.e. fail memslot creation as well.
Signalling success and continuing on with memslot creation results in
use-after-free, as the guest_memfd instance will remain reachable via the
memslot after the file is freed (kvm_gmem_release() won't nullify the file
pointer due to lack of a valid binding).

Opportunistically WARN and reject binding if KVM_MEMSLOT_GMEM_ONLY is
already set, partly to guard against goofs elsewhere, but mostly so that
KVM doesn't need to worry about clobbering flags when unwinding on failure.

Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
Cc: stable@vger.kernel.org
Reported-by: Stefan Teodorescu <fane@google.com>
Reported-by: Dennis Tighe <dtighe@google.com>
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index b596486d184c..1ef9e2916423 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -612,10 +612,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	struct inode *inode;
 	struct file *file;
 	int r = -EINVAL;
+	void *xar;
 
 	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
 	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
 
+	if (WARN_ON_ONCE(slot->flags & KVM_MEMSLOT_GMEM_ONLY))
+		return -EINVAL;
+
 	file = fget(fd);
 	if (!file)
 		return -EBADF;
@@ -654,7 +658,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (kvm_gmem_supports_mmap(inode))
 		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
 
-	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
+	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
 	filemap_invalidate_unlock(inode->i_mapping);
 
 	/*
@@ -662,7 +666,12 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	 * not the other way 'round.  Active bindings are invalidated if the
 	 * file is closed before memslots are destroyed.
 	 */
-	r = 0;
+	r = xa_is_err(xar) ? xa_err(xar) : 0;
+	if (r) {
+		slot->gmem.file = NULL;
+		slot->gmem.pgoff = 0;
+		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
+	}
 err:
 	fput(file);
 	return r;
-- 
2.55.0.887.g758fc8c411-goog

[PATCH 2/2] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

[Sean Christopherson]

Drop the superfluous WRITE_ONCE() when setting a memslot's guest_memfd file
during initial binding, as the memslot *must* be inactive and unreachable.
The superfluous WRITE_ONCE() was added by commit 67b43038ce14 ("KVM:
guest_memfd: Remove RCU-protected attribute from slot->gmem.file") to
maintain rough "parity" with the existing rcu_assign_pointer(), not
realizing that the only reason rcu_assign_pointer() was used was to make
sparse and other checkers happy.

Cc: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 1ef9e2916423..1d2de9a1bc3f 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -653,7 +653,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot
 	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
 	 */
-	WRITE_ONCE(slot->gmem.file, file);
+	slot->gmem.file = file;
 	slot->gmem.pgoff = start;
 	if (kvm_gmem_supports_mmap(inode))
 		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
-- 
2.55.0.887.g758fc8c411-goog

Re: [PATCH 1/2] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Partial failure of xa_store_range() leaves dangling pointers to a freed memslot in the guest_memfd bindings xarray, leading to a Use-After-Free when the file is released.
--

commit 31a2b2a91bb8b7043e1a087271db5def7eb591b8
Author: Sean Christopherson <seanjc@google.com>

KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

This commit updates kvm_gmem_bind() to handle errors returned by
xa_store_range(). If inserting a memslot into a guest_memfd bindings xarray
fails, the error is propagated to the caller to abort memslot creation.

Cc: stable@vger.kernel.org
Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

> @@ -654,7 +658,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>  
> -	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> +	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
>  	filemap_invalidate_unlock(inode->i_mapping);
>  
>  	/*
> @@ -662,7 +666,12 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	 * not the other way 'round.  Active bindings are invalidated if the
>  	 * file is closed before memslots are destroyed.
>  	 */
> -	r = 0;
> +	r = xa_is_err(xar) ? xa_err(xar) : 0;
> +	if (r) {
> +		slot->gmem.file = NULL;
> +		slot->gmem.pgoff = 0;
> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> +	}

[Severity: High]
If xa_store_range() encounters an error halfway through the range (for
example, -ENOMEM), can it partially succeed and leave some entries pointing
to the memslot in the f->bindings xarray?

If this happens, the error is propagated back to kvm_set_memory_region(),
which then aborts the operation and frees the memslot structure. Does this
leave dangling pointers in the f->bindings xarray?

Later, when the guest_memfd file is closed, kvm_gmem_release() iterates
over the xarray:

virt/kvm/guest_memfd.c:kvm_gmem_release() {
    ...
    xa_for_each(&f->bindings, index, slot)
        WRITE_ONCE(slot->gmem.file, NULL);
    ...
}

Could this trigger a use-after-free when kvm_gmem_release() writes NULL to
the already freed memslot? Should any partially inserted xarray elements be
erased from f->bindings before propagating the error?

>  err:
>  	fput(file);
>  	return r;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826165154.766699-1-seanjc@google.com?part=1

Re: [PATCH 2/2] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent guest_memfd hole punch triggers NULL-pointer dereference via uninitialized memslot.
--

commit 2a171d16c640a591ee0f3fb012de05495cd1214e
Author: Sean Christopherson <seanjc@google.com>

KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

Drop the superfluous WRITE_ONCE() when setting a memslot's guest_memfd file
during initial binding, as the memslot must be inactive and unreachable.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 1ef9e29164237..1d2de9a1bc3f0 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -653,7 +653,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot
>  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
>  	 */

[Severity: High]
This is a pre-existing issue, but is the new memslot actually completely
invisible here?

In virt/kvm/kvm_main.c:kvm_set_memory_region(), the new memslot is bound and
published to guest_memfd before its architecture metadata is prepared:

	r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);

When virt/kvm/guest_memfd.c:kvm_gmem_bind() executes, it inserts the partially
initialized memslot into the bindings xarray and drops the lock:

	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);

If userspace concurrently issues a FALLOC_FL_PUNCH_HOLE on the guest_memfd with
an offset greater than the memslot's offset, it can acquire the lock, iterate
over f->bindings, and find this uninitialized memslot. This triggers an MMU
invalidation over the uninitialized memslot:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
    ...
	flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);
    ...
}

On x86 with rmaps, gfn_to_rmap() will perform pointer arithmetic on the
unallocated slot->arch.rmap (which is NULL), yielding an invalid non-NULL
pointer. This bypasses the NULL check in slot_rmap_walk_okay() and causes
a kernel panic in kvm_zap_rmap().

Does this race condition allow a concurrent hole punch to dereference an invalid
pointer?

> -	WRITE_ONCE(slot->gmem.file, file);
> +	slot->gmem.file = file;
>  	slot->gmem.pgoff = start;
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826165154.766699-1-seanjc@google.com?part=2

Re: [PATCH 1/2] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[Sean Christopherson]

On Wed, Aug 26, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> 
> [ ... ]
> 
> > @@ -654,7 +658,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >  	if (kvm_gmem_supports_mmap(inode))
> >  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> >  
> > -	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> > +	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> >  	filemap_invalidate_unlock(inode->i_mapping);
> >  
> >  	/*
> > @@ -662,7 +666,12 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >  	 * not the other way 'round.  Active bindings are invalidated if the
> >  	 * file is closed before memslots are destroyed.
> >  	 */
> > -	r = 0;
> > +	r = xa_is_err(xar) ? xa_err(xar) : 0;
> > +	if (r) {
> > +		slot->gmem.file = NULL;
> > +		slot->gmem.pgoff = 0;
> > +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> > +	}
> 
> [Severity: High]
> If xa_store_range() encounters an error halfway through the range (for
> example, -ENOMEM), can it partially succeed and leave some entries pointing
> to the memslot in the f->bindings xarray?

Something tells me this isn't idle curiosity...  Assuming Sashiko is correct,
this as fixup?

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 1d2de9a1bc3f..323159c9b5f3 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -668,6 +668,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	 */
 	r = xa_is_err(xar) ? xa_err(xar) : 0;
 	if (r) {
+		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
 		slot->gmem.file = NULL;
 		slot->gmem.pgoff = 0;
 		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;

Re: [PATCH 2/2] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

[Sean Christopherson]

On Wed, Aug 26, 2026, sashiko-bot@kernel.org wrote:
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 1ef9e29164237..1d2de9a1bc3f0 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -653,7 +653,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot
> >  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
> >  	 */
> 
> [Severity: High]
> This is a pre-existing issue, but is the new memslot actually completely
> invisible here?
> 
> In virt/kvm/kvm_main.c:kvm_set_memory_region(), the new memslot is bound and
> published to guest_memfd before its architecture metadata is prepared:
> 
> 	r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
> 
> When virt/kvm/guest_memfd.c:kvm_gmem_bind() executes, it inserts the partially
> initialized memslot into the bindings xarray and drops the lock:
> 
> 	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> 
> If userspace concurrently issues a FALLOC_FL_PUNCH_HOLE on the guest_memfd with
> an offset greater than the memslot's offset, it can acquire the lock, iterate
> over f->bindings, and find this uninitialized memslot. This triggers an MMU
> invalidation over the uninitialized memslot:
> 
> virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
>     ...
> 	flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);
>     ...
> }
> 
> On x86 with rmaps, gfn_to_rmap() will perform pointer arithmetic on the
> unallocated slot->arch.rmap (which is NULL), yielding an invalid non-NULL
> pointer. This bypasses the NULL check in slot_rmap_walk_okay() and causes
> a kernel panic in kvm_zap_rmap().
> 
> Does this race condition allow a concurrent hole punch to dereference an invalid
> pointer?

Fuuuuudge.  Sashiko is right, the slot is reachable as soon as it's stored in
the binding.  I've fiddled with a few ideas, and they're all awful.  Ok, that's
not entirely true.  Moving the call to kvm_gmem_bind() into kvm_set_memslot() is
very doable, I'm just annoyed that the aesthetically pleasing prototype for
kvm_set_memslot() gets polluted with gmem parameters. :-/

AFAICT, binding after the memslot is prepared is the only sane option.  Because
memslots are protected by SRCU, it's simply not possible to ensure readers can't
see half-baked state if the binding is established before the memslot is fully
prepared.

And if the slot is committed before bindings are established, then PUNCH_HOLE
won't zap SPTEs created between the slot being reachable and the bindings being
established.  That, and KVM has a ton of code that assumes kvm_commit_memory_region()
occurs after the point of no return, i.e. unwinding the commit is a non-starter.

I think this would work?  Compile-tested only.

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..19b50be4fe20 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1887,7 +1887,8 @@ static void kvm_update_flags_memslot(struct kvm *kvm,
 static int kvm_set_memslot(struct kvm *kvm,
 			   struct kvm_memory_slot *old,
 			   struct kvm_memory_slot *new,
-			   enum kvm_mr_change change)
+			   enum kvm_mr_change change,
+			   unsigned int gmem_fd, uoff_t gmem_offset)
 {
 	struct kvm_memory_slot *invalid_slot;
 	int r;
@@ -1931,20 +1932,16 @@ static int kvm_set_memslot(struct kvm *kvm,
 	}
 
 	r = kvm_prepare_memory_region(kvm, old, new, change);
-	if (r) {
-		/*
-		 * For DELETE/MOVE, revert the above INVALID change.  No
-		 * modifications required since the original slot was preserved
-		 * in the inactive slots.  Changing the active memslots also
-		 * release slots_arch_lock.
-		 */
-		if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
-			kvm_activate_memslot(kvm, invalid_slot, old);
-			kfree(invalid_slot);
-		} else {
-			mutex_unlock(&kvm->slots_arch_lock);
-		}
-		return r;
+	if (r)
+		goto err;
+
+	if (new && new->flags & KVM_MEM_GUEST_MEMFD) {
+		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
+			goto err;
+
+		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
+		if (r)
+			goto err;
 	}
 
 	/*
@@ -1977,6 +1974,20 @@ static int kvm_set_memslot(struct kvm *kvm,
 	kvm_commit_memory_region(kvm, old, new, change);
 
 	return 0;
+
+err:
+	/*
+	 * For DELETE/MOVE, revert the above INVALID change.  No modifications
+	 * required since the original slot was preserved in the inactive slots.
+	 * Changing the active memslots also release slots_arch_lock.
+	 */
+	if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
+		kvm_activate_memslot(kvm, invalid_slot, old);
+		kfree(invalid_slot);
+	} else {
+		mutex_unlock(&kvm->slots_arch_lock);
+	}
+	return r;
 }
 
 static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
@@ -2058,7 +2069,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 		if (WARN_ON_ONCE(kvm->nr_memslot_pages < old->npages))
 			return -EIO;
 
-		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE);
+		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE, -1, 0);
 	}
 
 	base_gfn = (mem->guest_phys_addr >> PAGE_SHIFT);
@@ -2105,21 +2116,14 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	new->npages = npages;
 	new->flags = mem->flags;
 	new->userspace_addr = mem->userspace_addr;
-	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
-		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
-		if (r)
-			goto out;
-	}
 
-	r = kvm_set_memslot(kvm, old, new, change);
+	r = kvm_set_memslot(kvm, old, new, change,
+			    mem->guest_memfd, mem->guest_memfd_offset);
 	if (r)
-		goto out_unbind;
+		goto out;
 
 	return 0;
 
-out_unbind:
-	if (mem->flags & KVM_MEM_GUEST_MEMFD)
-		kvm_gmem_unbind(new);
 out:
 	kfree(new);
 	return r;