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

[Sean Christopherson <seanjc@google.com>]

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;