Re: [PATCH 4/4] KVM: x86: Reject user memory regions covering the APIC base

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added APIC base memslot check unconditionally rejects KVM's internal APIC access page, breaking VMX vCPU creation and triggering a guaranteed VMA leak.

Pre-existing issues:
- [High] Preexisting Use-After-Free in KVM_CREATE_IRQCHIP due to lockless read of the memslots array.
--

commit 75a39a0e2b5c8384d4f938fb4214b675289701b6
Author: Tim Wiederhake <twiederh@redhat.com>

KVM: x86: Reject user memory regions covering the APIC base

This commit adds an explicit check in kvm_arch_prepare_memory_region()
to reject user space attempts to map guest memory covering the default
APIC base address when an in-kernel irqchip is in use.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cb7f759a7b21e..f7f2bd4d8b334 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -10129,12 +10129,19 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  		return -EINVAL;
>  
>  	if (change == KVM_MR_CREATE || change == KVM_MR_MOVE) {
> +		const gfn_t apic_gfn = gpa_to_gfn(APIC_DEFAULT_PHYS_BASE);
> +
>  		if ((new->base_gfn + new->npages - 1) > kvm_mmu_max_gfn())
>  			return -EINVAL;
>  
>  		if (kvm_is_gfn_alias(kvm, new->base_gfn + new->npages - 1))
>  			return -EINVAL;
>  
> +		if (irqchip_in_kernel(kvm) &&
> +		    new->base_gfn <= apic_gfn &&
> +		    apic_gfn < new->base_gfn + new->npages)
> +			return -EEXIST;

[Severity: High]
Could this check inadvertently catch KVM's own internal memslot allocations,
such as APIC_ACCESS_PAGE_PRIVATE_MEMSLOT?

When kvm_alloc_apic_access_page() attempts to allocate the internal memslot
exactly at APIC_DEFAULT_PHYS_BASE, it appears this new check will intercept
it and return -EEXIST, which would break vCPU creation.

Furthermore, if this returns -EEXIST during an internal memslot allocation,
could it cause a VMA memory leak?

Looking at __x86_set_memory_region() in arch/x86/kvm/x86.c:

	hva = vm_mmap(NULL, 0, size, PROT_READ | PROT_WRITE,
		      MAP_SHARED | MAP_ANONYMOUS, 0);
	...
	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
		...
		r = kvm_set_internal_memslot(kvm, &m);
		if (r < 0)
			return ERR_PTR_USR(r);
	}

If kvm_set_internal_memslot() fails here because the new check returns
-EEXIST, the error path returns immediately without calling vm_munmap()
to clean up the VMA allocated earlier.

> +
>  		return kvm_alloc_memslot_metadata(kvm, new);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the APIC
base and irqchip code, I noticed a potential use-after-free when
KVM_CREATE_IRQCHIP accesses the memslots array.

Looking at kvm_arch_vm_ioctl() in arch/x86/kvm/x86.c:

	case KVM_CREATE_IRQCHIP: {
		mutex_lock(&kvm->lock);
		...
		r = -EEXIST;
		if (gfn_to_memslot(kvm, gpa_to_gfn(APIC_DEFAULT_PHYS_BASE)))
			goto create_irqchip_unlock;

Is it safe to call gfn_to_memslot() here outside of any SRCU read-side
critical section and without holding kvm->slots_lock?

A concurrent thread executing KVM_SET_USER_MEMORY_REGION (which holds
kvm->slots_lock but not kvm->lock) could install a new memslots array, issue
synchronize_srcu(), and free the old memslots array while KVM_CREATE_IRQCHIP
is still traversing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706092021.3625908-2-twiederh@redhat.com?part=4