Re: [PATCH 1/5] 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:
- [High] Ignoring the return value of xa_store_range() in guest_memfd binding paths leads to a critical Use-After-Free vulnerability.
--

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

KVM: guest_memfd: take the invalidate lock when unbinding a dying file

kvm_gmem_unbind() skips mapping->invalidate_lock when the guest_memfd
file is already dying. All other paths that modify f->bindings hold
that lock. Take the invalidate lock in the dying-file path too to prevent
unbind from removing a binding and leaking mmu_invalidate_in_progress.

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

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index f0e5da4908660..f848120af84be 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -721,6 +721,8 @@ static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value
of xa_store_range() here and in kvm_gmem_bind() lead to a use-after-free?

Looking at kvm_gmem_bind() in virt/kvm/guest_memfd.c, xa_store_range() is
called with GFP_KERNEL:

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

If memory allocation fails during this operation, xa_store_range() returns an
error pointer, but the return value is ignored. This masks ENOMEM errors and
leads to an inconsistent state where the slot points to the file but the file
doesn't track the slot.

Because the slot is missing from the file's XArray, when the file is later
closed, kvm_gmem_release() iterates over f->bindings but fails to find this
slot, leaving slot->gmem.file as a dangling pointer.

When the VM destroys the memslot, kvm_gmem_unbind() accesses this dangling
pointer and calls get_file_active() on it. Can this cause a use-after-free on
the freed struct file, or improperly increment the refcount of an unrelated,
reused memory object?

Similarly, in __kvm_gmem_unbind():

virt/kvm/guest_memfd.c:__kvm_gmem_unbind() {
	...
	xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
	...
}

The return value is ignored here as well, masking errors when removing from
the XArray. Should the return value of xa_store_range() be checked and
handled to prevent these vulnerabilities?

>  
>  void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  {
> +	struct file *gmem_file;
> +

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