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

[sashiko-bot@kernel.org]

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