KVM: guest_memfd: Handle xarray binding errors
Subsystems (2)
Sean Christopherson
maintainercover 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
Sean Christopherson
maintainerPATCH 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
@@ -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
[email protected]
AI reviewRe: [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
-0err:fput (file);return r;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Sean Christopherson
maintainerRe: [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]
Ifxa_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 thef->bindingsxarray?
Something tells me this isn't idle curiosity... Assuming Sashiko is correct, this as fixup?
virt/kvm/guest_memfd.c +1 -0
@@ -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 ;
Sean Christopherson
maintainerPATCH 2/2 KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot
In reply to Sean ChristophersonDrop 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
@@ -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
[email protected]
AI reviewRe: [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@@ -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
Sean Christopherson
maintainerRe: [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@@ -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_memfdbefore 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_HOLEon theguest_memfdwith an offset greater than the memslot's offset, it can acquire the lock, iterate overf->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 unallocatedslot->arch.rmap(which is NULL), yielding an invalid non-NULL pointer. This bypasses the NULL check inslot_rmap_walk_okay()and causes a kernel panic inkvm_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
@@ -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;