Re: [PATCH v3 1/9] KVM: guest_memfd: take the invalidate lock when unbinding a dying file

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] kvm_gmem_bind() ignores the return value of xa_store_range(), which can fail with -ENOMEM, leading to inconsistent memslot tracking and guest-to-host use-after-free.
--

--- Patch [1]: [PATCH v3 1/9] KVM: guest_memfd: take the invalidate lock when unbinding a dying file ---
commit 4192ee62a3c9cd53bedefe498647d781d59dfdf5
Author: Shivank Garg <[email protected]>
KVM: guest_memfd: take the invalidate lock when unbinding a dying file

This patch takes the invalidate lock during the dying-file path in
kvm_gmem_unbind() to prevent a race condition with memory failures.
It ensures the mmu_invalidate_in_progress counter remains balanced
when a binding is removed concurrently.

Link: https://lore.kernel.org/all/[email protected]

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index db57c5766ab60..45cbdf4801ece 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in kvm_gmem_bind() where an error return is ignored:

virt/kvm/guest_memfd.c:kvm_gmem_bind() {
        ...
        xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
        filemap_invalidate_unlock(inode->i_mapping);

        /*
         * Drop the reference to the file, even on success.  The file pins KVM,
         * not the other way 'round.  Active bindings are invalidated if the
         * file is closed before memslots are destroyed.
         */
        r = 0;
err:
        fput(file);
        return r;
}

Does this code ignore the return value of xa_store_range(), which can fail
with -ENOMEM under memory pressure?

If xa_store_range() fails, the memslot won't be recorded in the guest_memfd
file's bindings. Subsequent host invalidations, such as FALLOC_FL_PUNCH_HOLE
or a memory failure, would fail to find the memslot and wouldn't invalidate
the guest's page tables.

Can this allow the guest to retain access to freed host memory, leading to
host memory use-after-free or cross-VM data corruption if the memory is
reallocated to other host processes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1