Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Bypassing filemap_invalidate_lock in kvm_gmem_unbind when the file is dying allows concurrent xarray modifications, causing Use-After-Free and leaked mmu_invalidate_in_progress.
--

commit 22b749f62017c828a40c33364eefe3474406907f
Author: Sean Christopherson <seanjc@google.com>

KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

This commit updates the comment in kvm_gmem_release() to elaborate on the
synchronization between release() and get_pfn(). It explains why neither
RCU nor kvm->srcu synchronization is necessary when nullifying memslot
bindings, detailing the barriers used to prevent a use-after-free.

Link: https://lore.kernel.org/all/CAEvNRgGmyd1yqQXsnz5hWRZpBZUs%3DpiEWbEaqP9%2Bcz9ZqEMQ6g@mail.gmail.com [*]

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index b596486d184ca..7f1c6a0f8039d 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -300,17 +300,55 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>  	 * dereferencing the slot for existing bindings needs to be protected
>  	 * against memslot updates, specifically so that unbind doesn't race
>  	 * and free the memslot (kvm_gmem_get_file() will return NULL).
>  	 */
>  	mutex_lock(&kvm->slots_lock);
>  
>  	filemap_invalidate_lock(inode->i_mapping);
>  
> +	/*
> +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
[ ... ]
> +	 * Obviously KVM would be broken in many places if the synchronization
> +	 * were omitted, but it's important to note that get_file_active() does
> +	 * NOT guarantee a reference to the correct file was obtained, only
> +	 * that the file doesn't point at a reallocated object.
> +	 */
>  	xa_for_each(&f->bindings, index, slot)
>  		WRITE_ONCE(slot->gmem.file, NULL);
>  

[Severity: Critical]
This is a pre-existing issue, but could bypassing filemap_invalidate_lock in
kvm_gmem_unbind() when the file is dying allow concurrent xarray modifications
that result in a use-after-free?

In kvm_gmem_unbind(), when the file is dying:

	if (!file) {
		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
		return;
	}

Because kvm_gmem_error_folio() handles memory failure on guest_memfd pages by
holding filemap_invalidate_lock_shared(mapping) and iterates f->bindings
without holding kvm->srcu or kvm->slots_lock, it relies entirely on
filemap_invalidate_lock.

If kvm_gmem_unbind() bypasses filemap_invalidate_lock when the underlying
file is dying, it can run concurrently with kvm_gmem_error_folio() and
remove a slot from f->bindings. This allows kvm_set_memory_region() to free
the memslot while kvm_gmem_error_folio() is still holding a pointer to it.

Could this cause a use-after-free during the iteration:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
	xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
		pgoff_t pgoff = slot->gmem.pgoff;
		...
}

Additionally, if the slot is removed before __kvm_gmem_invalidate_end(),
xa_find() returns false, which skips kvm_mmu_invalidate_end() and
permanently leaks mmu_invalidate_in_progress:

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);
	}
}

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