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

[Sean Christopherson <seanjc@google.com>]

On Fri, Oct 02, 2026, sashiko-bot@kernel.org wrote:
> 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

Ugh, SMM, which is the "twin" aspect Sashiko is referring to.

> 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?

Heh, I arrived at the same conclusion before reading this.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e91bd79dd3cc..e55aa26e26cc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10050,13 +10050,15 @@ 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.
+	 * Unmap VMAs allocated on behalf of userspace for internal memslots,
+	 * unless the memory map has changed due to process exit or fd copying.
+	 * Do the unmap only for the non-SMM slot, as the same VMA is used for
+	 * both non-SMM and SMM memslots.  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))
+	    !slot->as_id && !WARN_ON_ONCE(!slot->npages))
 		vm_munmap(slot->userspace_addr, slot->npages * PAGE_SIZE);
 }