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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-after-free and race condition in error path of kvm_gmem_bind()
--

commit 750801b0d7bf80e6bf0cad3be58657ec4af30b90
Author: Sean Christopherson <seanjc@google.com>

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

This commit propagates errors from inserting a memslot into a guest_memfd's
bindings xarray back to the caller to prevent a use-after-free when the file
is released. It also opportunistically rejects bindings if
KVM_MEMSLOT_GMEM_ONLY is already set.

Link: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index b596486d184ca..2c8d8735de5fe 100644
> --- 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,13 @@ 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) {
> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);

[Severity: High]
Does this error path modify the bindings XArray without holding the
filemap_invalidate_lock?

If the initial xa_store_range() fails (for example, returning -ENOMEM),
filemap_invalidate_unlock() has already been called just above.

Concurrently, if a thread calls fallocate(PUNCH_HOLE) on the guest_memfd,
it could acquire filemap_invalidate_lock, iterate the XArray, and observe
the partially inserted slot.

While fallocate() is passing the slot to kvm_mmu_unmap_gfn_range(), this
error path clears the binding locklessly and returns the error to
userspace (e.g. from KVM_SET_USER_MEMORY_REGION). This causes the slot to
be freed immediately by kvm_set_memory_region(). Can this result in a
use-after-free since fallocate() is concurrently dereferencing it?

> +		slot->gmem.file = NULL;
> +		slot->gmem.pgoff = 0;
> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> +	}
>  err:
>  	fput(file);
>  	return r;

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