Re: [PATCH v2 03/10] KVM: x86: Unmap VMAs for KVM-internal memslots when the memslot is freed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: double unmap of internal memslot VMA causes host UAF
--

commit 120ca6d762cdf95d09e394aff4bb4af6d01288d6
Author: Sean Christopherson <seanjc@google.com>
KVM: x86: Unmap VMAs for KVM-internal memslots when the memslot is freed

This commit unmaps KVM-created VMAs for KVM-internal memslots when the
associated memslot is freed directly, instead of using x86's internal API,
to prevent VMA leaks on teardown.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 79468ddfe4736..e91bd79dd3cc6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -10066,6 +10048,16 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
>  	}
>  
>  	kvm_page_track_free_memslot(slot);
> +
> +	/*
> +	 * Free memory regions allocated on behalf of userspace, unless the
> +	 * memory map has changed due to process exit or fd copying.  Leak the
> +	 * mapping on failure, e.g. if the task is killed, worst case scenario,
> +	 * the page(s) will be reclaimed when the process exits.
> +	 */
> +	if (current->mm == kvm->mm && slot->id >= KVM_USER_MEM_SLOTS &&
> +	    !WARN_ON_ONCE(!slot->npages))
> +		vm_munmap(slot->userspace_addr, slot->npages * PAGE_SIZE);
>  }

[Severity: High]
Does this unconditionally unmap the VMA for every freed internal memslot object,
potentially leading to a double unmap?

In __x86_set_memory_region(), the twin memslot objects are created with the
exact same userspace_addr for each address space:

	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
		struct kvm_userspace_memory_region2 m;

		m.slot = id | (i << 16);
		m.flags = 0;
		m.guest_phys_addr = gpa;
		m.userspace_addr = hva;
...

When the VM is destroyed or the memslot is deleted, KVM iterates over all
address spaces and calls kvm_arch_free_memslot() for each memslot object.

Because vm_munmap() is called here without checking if slot->as_id is 0, could
it unmap the same HVA range multiple times?

If a concurrent thread maps memory between the two unmaps, the new memory
might be silently unmapped, potentially leading to memory corruption and
use-after-free in the host.

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