Re: [PATCH RFC 03/10] KVM: guest_memfd: Avoid conditional mmu_lock acquisition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent memslot unbind in `!file` case bypasses `filemap_invalidate_lock`, causing UAF and unbalanced MMU invalidate.
--

commit 48bb5bc5161f90dfbe496e0e21d2ca910bf0e571
Author: Marco Elver <elver@google.com>

KVM: guest_memfd: Avoid conditional mmu_lock acquisition

This refactors __kvm_gmem_invalidate_start() to check for present bindings
upfront via xa_find(). It eliminates conditional locking inside the loop to
simplify control flow.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index b596486d184ca..1983c1e2dba17 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -124,10 +124,16 @@ static void __kvm_gmem_invalidate_start(struct gmem_file *f, pgoff_t start,
>  					pgoff_t end,
>  					enum kvm_gfn_range_filter attr_filter)
>  {
[ ... ]
> -	unsigned long index;
> +	pgoff_t index = start;
> +
> +	if (!xa_find(&f->bindings, &index, end - 1, XA_PRESENT))
> +		return;
> +
> +	KVM_MMU_LOCK(kvm);
> +	kvm_mmu_invalidate_start(kvm);

[Severity: High]
This is a pre-existing issue, but does this upfront check introduce a
time-of-check to time-of-use (TOCTOU) window when interacting with a
concurrent memslot unbind?

Looking at kvm_gmem_unbind() in virt/kvm/guest_memfd.c, if a guest_memfd
file is closing (refcount drops to 0), it skips acquiring
filemap_invalidate_lock:

virt/kvm/guest_memfd.c:kvm_gmem_unbind() {
    ...
    if (!file) {
        __kvm_gmem_unbind(slot, slot->gmem.file->private_data);
        return;
    }
    ...
}

This allows __kvm_gmem_unbind() to call xa_store_range(..., NULL)
concurrently without holding the lock. If the binding is removed
immediately after the new xa_find() check above, the loop in
__kvm_gmem_invalidate_start() won't execute, but the invalidate counter
is already incremented.

Subsequently, in __kvm_gmem_invalidate_end(), if the lockless unbind
causes its xa_find() to return false:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_end() {
    if (xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
        KVM_MMU_LOCK(kvm);
        kvm_mmu_invalidate_end(kvm);
        KVM_MMU_UNLOCK(kvm);
    }
}

Could this skip kvm_mmu_invalidate_end() and permanently leak the
mmu_invalidate_in_progress counter, potentially causing all vCPUs to hang
by infinitely retrying in mmu_invalidate_retry()?

>  	xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
>  		pgoff_t pgoff = slot->gmem.pgoff;
[ ... ]
>  		flush |= kvm_mmu_unmap_gfn_range(kvm, &gfn_range);

[Severity: High]
This is also a pre-existing issue, but does yielding here open a race
window that could lead to a use-after-free?

If __kvm_gmem_invalidate_start() is called via a memory failure path
(e.g., kvm_gmem_error_folio() acquiring filemap_invalidate_lock_shared()),
and kvm_gmem_unbind() concurrently skips the lock, the slot could be
removed from f->bindings and freed by kvm_free_memslot().

Since kvm_mmu_unmap_gfn_range() can drop mmu_lock to yield, could the
iterator access a freed slot when it resumes?

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