Skip to contentSkip to conversation

KVM: guest_memfd: Handle xarray binding errors

PATCH Superseded Sean Christopherson 2 participants 7 messages Aug 26

Subsystems (2)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sean Christopherson

author maintainer
#0 lore raw

cover KVM: guest_memfd: Handle xarray binding errors

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

Signature · 1 line
2.55.0.887.g758fc8c411-goog
author maintainer
#1 lore raw

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

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.

---
 virt/kvm/guest_memfd.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
virt/kvm/guest_memfd.c +11 -2
index b596486d184c..1ef9e2916423 100644
@@ -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;
Signature · 1 line
2.55.0.887.g758fc8c411-goog
#2 lore raw

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

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…

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 <[email protected]>

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.

Quoted 3 lines
virt/kvm/guest_memfd.c +0 -0

[ ... ]

Quoted 10 lines
virt/kvm/guest_memfd.c +1 -1
@@ -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);
 
 	/*

virt/kvm/guest_memfd.c int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, · line 674

Quoted 10 lines
virt/kvm/guest_memfd.c +6 -1
@@ -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?

Quoted 3 lines
virt/kvm/guest_memfd.c +0 -0
 err:
 	fput(file);
 	return r;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

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

Quoted 24 lines · On Wed, Aug 26, 2026, [email protected] wrote:

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

Quoted 3 lines
virt/kvm/guest_memfd.c +0 -0

[ ... ]

Quoted 17 lines
virt/kvm/guest_memfd.c +4 -2
@@ -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;
Quoted 8 lines

virt/kvm/guest_memfd.c int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, · line 674

Quoted 3 lines
virt/kvm/guest_memfd.c +3 -0
+		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?

virt/kvm/guest_memfd.c +1 -0
index 1d2de9a1bc3f..323159c9b5f3 100644
@@ -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;
author maintainer
#4 lore raw

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

In reply to 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.

---
 virt/kvm/guest_memfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
virt/kvm/guest_memfd.c +1 -1
index 1ef9e2916423..1d2de9a1bc3f 100644
@@ -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;
Signature · 1 line
2.55.0.887.g758fc8c411-goog
#5 lore raw

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

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

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 <[email protected]>

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.

virt/kvm/guest_memfd.c int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, · line 655

Quoted 8 lines
virt/kvm/guest_memfd.c +0 -0
index 1ef9e29164237..1d2de9a1bc3f0 100644
@@ -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?

Quoted 5 lines
virt/kvm/guest_memfd.c +1 -1
-	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;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
author maintainer
#6 lore raw

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

Quoted 34 lines · On Wed, Aug 26, 2026, [email protected] wrote:

virt/kvm/guest_memfd.c int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, · line 655

Quoted 8 lines
virt/kvm/guest_memfd.c +0 -0
index 1ef9e29164237..1d2de9a1bc3f0 100644
@@ -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);
    ...
}
Quoted 7 lines

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.

virt/kvm/kvm_main.c +30 -26
index 65eb26a0520d..19b50be4fe20 100644
@@ -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;