Re: [PATCH v2 4/4] 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:
- [Critical] Modifying the guest_memfd xarray outside of filemap_invalidate_lock on the error path causes a Use-After-Free and permanent MMU invalidation lockup.
--

commit c377e9963148427885842d70319259945c649a8a
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.
The superfluous WRITE_ONCE() was added by commit 67b43038ce14 to
maintain rough "parity" with the existing rcu_assign_pointer().

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 2c8d8735de5fe..323159c9b5f34 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.
>  	 */
> -	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;

[Severity: High]
This isn't a bug introduced by this patch, but modifying the xarray outside
the filemap_invalidate_lock on the error path further down in this function
appears to cause a use-after-free and a permanent MMU invalidation lockup.

If xa_store_range() encounters -ENOMEM during the store, it can abort and
leave the slot partially inserted in the tree. kvm_gmem_bind() then explicitly
drops filemap_invalidate_lock before executing its error path. 

On the error path, it attempts to clean up by calling xa_store_range() with
NULL outside the lock:

    xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
    filemap_invalidate_unlock(inode->i_mapping);

    r = xa_is_err(xar) ? xa_err(xar) : 0;
    if (r) {
        xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);

Concurrently, a PUNCH_HOLE operation runs __kvm_gmem_invalidate_start()
which takes filemap_invalidate_lock and iterates over the XArray.

Because the error path doesn't hold the lock, it can race with the iteration:

CPU1 (kvm_gmem_bind error path)
    if (r) {
        xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
        ...

CPU2 (__kvm_gmem_invalidate_start)
    xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
        ...

xa_for_each_range() does not establish an RCU read-side critical section for
the loop body. The error path removes the partially inserted slot and returns,
causing kvm_set_memory_region() to immediately kfree() the slot. The
PUNCH_HOLE iteration then dereferences the freed slot, causing a
use-after-free.

Additionally, the PUNCH_HOLE operation successfully called
kvm_mmu_invalidate_start(), but because the slot is removed by the racing
error path before __kvm_gmem_invalidate_end() executes, the closing
xa_find() check fails and kvm_mmu_invalidate_end() is permanently skipped.

Does the error path need to execute with filemap_invalidate_lock() held?

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