Re: [PATCH v2 3/4] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] kvm_gmem_bind drops filemap_invalidate_lock before rolling back a failed xa_store_range, exposing a partially bound, not-yet-active memslot to concurrent PUNCH_HOLE operations and leading to a severe Use-After-Free on the error path.
--

commit cf9b1c6186b61c3381316f5121f13c09601e0b01
Author: Sean Christopherson <seanjc@google.com>
KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

This commit defers binding a memslot to a guest_memfd instance until the memslot
is fully prepared. It prevents exposing the memslot to readers prematurely,
which could break various memslot assumptions and rules.

Closes: https://lore.kernel.org/all/20260826170551.BEF801F000E9@smtp.kernel.org

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3c0dbe60a5b4f..21c10cbbac661 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -1934,6 +1935,15 @@ static int kvm_set_memslot(struct kvm *kvm,
>  	if (r)
>  		goto err;
>  
> +	if (new && new->flags & KVM_MEM_GUEST_MEMFD) {
> +		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
> +			goto err_bind;
> +
> +		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
> +		if (r)
> +			goto err_bind;
> +	}
> +
>  	/*
>  	 * For DELETE and MOVE, the working slot is now active as the INVALID
[ ... ]
> @@ -1965,6 +1975,13 @@ static int kvm_set_memslot(struct kvm *kvm,
>  
>  	return 0;
>  
> +err_bind:
> +	if (new) {
> +		kvm_arch_free_memslot(kvm, new);
> +
> +		if (new->dirty_bitmap && (!old || !old->dirty_bitmap))
> +			kvm_destroy_dirty_bitmap(new);
> +	}

[Severity: Critical]
This is a pre-existing issue, but does moving kvm_gmem_bind() into this
sequence create an exploitable use-after-free on the error path?

Looking at kvm_gmem_bind(), if xa_store_range() fails, the lock is released
before rolling back the partial entry:

virt/kvm/guest_memfd.c:kvm_gmem_bind() {
    ...
	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);
    ...
}

If a concurrent PUNCH_HOLE operation acquires the lock right after
filemap_invalidate_unlock(), could it extract the partially bound slot and
proceed to kvm_mmu_unmap_gfn_range()?

If so, while the concurrent thread is accessing the slot, this error path
executes kvm_arch_free_memslot(), and then the caller, kvm_set_memory_region(),
executes kfree(). Could this result in the PUNCH_HOLE thread operating on
completely freed memory?

>  err:
>  	/*
>  	 * For DELETE/MOVE, revert the above INVALID change.  No modifications

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