Re: [PATCH v3 04/15] 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: x86: premature VMA unmap in kvm_arch_free_memslot() while SMM slot is active
--

commit 6659f140d07225e838a8f211264ec11bfb5e9df2
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Unmap VMAs for KVM-internal memslots when the memslot is freed

This commit moves the VMA unmapping for x86 KVM-internal memslots from
__x86_set_memory_region() into kvm_arch_free_memslot(), and removes the
explicit internal memslot teardown from kvm_arch_destroy_vm().

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 32563a91fba99..0f3fa45713097 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -10058,8 +10058,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
>  		if (!slot || !slot->npages)
>  			return NULL;
>  
> -		old_npages = slot->npages;
> -		hva = slot->userspace_addr;
> +		hva = 0;
>  	}
>  
>  	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
> @@ -10075,9 +10074,6 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
>  			return ERR_PTR_USR(r);
>  	}
>  
> -	if (!size)
> -		vm_munmap(hva, old_npages * PAGE_SIZE);
> -
>  	return (void __user *)hva;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(__x86_set_memory_region);

[ ... ]

> @@ -10159,6 +10141,18 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
>  	}
>  
>  	kvm_page_track_free_memslot(slot);
> +
> +	/*
> +	 * 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 &&
> +	    !slot->as_id && !WARN_ON_ONCE(!slot->npages))
> +		vm_munmap(slot->userspace_addr, slot->npages * PAGE_SIZE);

[Severity: High]
Does unmapping the shared VMA in arch/x86/kvm/x86.c:kvm_arch_free_memslot()
when !slot->as_id cause a premature unmap while the SMM memslot (as_id == 1)
is still active?

When an internal memslot is deleted at runtime while vCPUs are running, such
as via arch/x86/kvm/lapic.c:kvm_inhibit_apic_access_page():

	if (kvm->arch.apic_access_memslot_enabled) {
		__x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT, 0, 0);

__x86_set_memory_region() iterates over the address spaces from i = 0 to
kvm_arch_nr_memslot_as_ids(kvm) - 1:

	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;
		m.memory_size = size;
		r = kvm_set_internal_memslot(kvm, &m);
		if (r < 0)
			return ERR_PTR_USR(r);
	}

Previously, vm_munmap() was called only after this loop finished deleting the
memslot from all address spaces.

With this change, on iteration i = 0 (as_id == 0), the call chain:

  kvm_set_internal_memslot()
    -> kvm_set_memory_region()
      -> kvm_set_memslot(..., KVM_MR_DELETE)
        -> kvm_commit_memory_region()
          -> kvm_free_memslot()
            -> kvm_arch_free_memslot()

sees !slot->as_id and immediately calls vm_munmap(slot->userspace_addr, ...),
releasing mmap_write_lock before iteration i = 1 begins.

Until iteration i = 1 calls kvm_invalidate_memslot() on the SMM slot, the
memslot in kvm->memslots[1] remains active and valid while pointing to the
already-unmapped userspace_addr. If a vCPU in SMM accesses or faults on that
GPA during this window, couldn't it resolve the active SMM memslot to an
unmapped HVA (failing KVM_RUN with -EFAULT) or to an unrelated VMA if another
host thread concurrently calls mmap()?

In addition, if iteration i = 1 fails in virt/kvm/kvm_main.c:kvm_set_memslot()
when allocating invalid_slot:

	if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
		invalid_slot = kzalloc_obj(*invalid_slot, GFP_KERNEL_ACCOUNT);
		if (!invalid_slot) {
			mutex_unlock(&kvm->slots_arch_lock);
			return -ENOMEM;
		}
		kvm_invalidate_memslot(kvm, old, invalid_slot);
	}

__x86_set_memory_region() aborts after as_id == 0 has already been deleted and
unmapped. Because __x86_set_memory_region() checks id_to_memslot(slots, id) in
as_id == 0 on entry and returns NULL if !slot, wouldn't the SMM memslot in
kvm->memslots[1] be left permanently active with a dangling userspace_addr?

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