Re: [PATCH 12/60] kvm: Move vcpu accounting to struct kvm_vcpu_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Concurrent KVM_CREATE_VCPU ioctls can bypass the VM's `max_vcpus` limit due to a TOCTOU race.
- [High] Concurrent vCPU creation assigns identical `vcpu_idx` values, leading to `xa_insert()` collisions and a host-reachable `WARN_ON_ONCE`.
- [High] Mixing goto-based error handling with `__free()` cleanup in `kvm_vcpu_init_common()`.
--

commit 13e7e9109dee4a6b84e351c0217863b5f716ad31
Author: Joerg Roedel <joerg.roedel@amd.com>
Subject: kvm: Move vcpu accounting to struct kvm_vcpu_common

This patch moves vCPU accounting and sanity checks into a common structure
(`kvm_vcpu_common`) to perform them only once per plane.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 71c806ad469c8..700f65f3ad037 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -438,18 +438,58 @@ void *kvm_mmu_memory_cache_alloc(struct kvm_mmu_memory_cache *mc)
>  }
>  #endif
>  
> -static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm)
> +static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned long id)
>  {
> -	struct kvm_vcpu_common *common = kzalloc(sizeof(*common), GFP_KERNEL_ACCOUNT);
> +	struct kvm_vcpu_common *common __free(kfree) = kzalloc(sizeof(*common), GFP_KERNEL_ACCOUNT);
> +	int r;

[Severity: High]
Does using __free(kfree) here conflict with the goto-based unwinding at
out_drop_counter below?

The cleanup guidelines state that usage of goto and cleanup helpers should
never be mixed in the same function.

> +
> +	/*
> +	 * KVM tracks vCPU IDs as 'int', be kind to userspace and reject
> +	 * too-large values instead of silently truncating.
> +	 *
> +	 * Ensure KVM_MAX_VCPU_IDS isn't pushed above INT_MAX without first
> +	 * changing the storage type (at the very least, IDs should be tracked
> +	 * as unsigned ints).
> +	 */
> +	BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);
> +	if (id >= KVM_MAX_VCPU_IDS)
> +		return -EINVAL;
> +
> +	mutex_lock(&kvm->lock);
> +	kvm->created_vcpus++;
> +	mutex_unlock(&kvm->lock);
> +
> +	if (common == NULL) {
> +		r = -ENOMEM;
> +		goto out_drop_counter;
> +	}
> +
> +	common->vcpu_idx = atomic_read(&kvm->online_vcpus);

[Severity: High]
Can concurrent vCPU creations read the same value for vcpu_idx here?

Because this is read outside of kvm->lock, multiple threads could obtain
identical indices, which would later cause an xa_insert() collision in
kvm_vm_ioctl_create_vcpu().

>  
>  	common->kvm = kvm;
>  	common->current_vcpu = vcpu;
> -	vcpu->common = common;
> +	vcpu->common = no_free_ptr(common);
>  
>  	return 0;
> +
> +out_drop_counter:
> +	mutex_lock(&kvm->lock);
> +	kvm->created_vcpus--;
> +	mutex_unlock(&kvm->lock);
> +
> +	return r;
> +}

[ ... ]

> @@ -4234,22 +4283,10 @@ static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
>   */
>  static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  {
> -	int r;
> +	int r = -EINVAL;
>  	struct kvm_vcpu *vcpu;
>  	struct page *page;
>  
> -	/*
> -	 * KVM tracks vCPU IDs as 'int', be kind to userspace and reject
> -	 * too-large values instead of silently truncating.
> -	 *
> -	 * Ensure KVM_MAX_VCPU_IDS isn't pushed above INT_MAX without first
> -	 * changing the storage type (at the very least, IDs should be tracked
> -	 * as unsigned ints).
> -	 */
> -	BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);
> -	if (id >= KVM_MAX_VCPU_IDS)
> -		return -EINVAL;
> -
>  	mutex_lock(&kvm->lock);
>  	if (kvm->created_vcpus >= kvm->max_vcpus) {
>  		mutex_unlock(&kvm->lock);
> @@ -4257,24 +4294,20 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  	}
>  
>  	r = kvm_arch_vcpu_precreate(kvm, id);
> -	if (r) {
> -		mutex_unlock(&kvm->lock);
> -		return r;
> -	}
> -
> -	kvm->created_vcpus++;
>  	mutex_unlock(&kvm->lock);

[Severity: High]
Does dropping kvm->lock here create a race with the kvm->created_vcpus
increment now located in kvm_vcpu_init_common()?

Multiple concurrent threads could pass the max_vcpus check above, drop the
lock here, and then indiscriminately increment created_vcpus. This would
allow an unprivileged host-userspace process to bypass the VM's configured
max_vcpus limit.

> +	if (r)
> +		return r;
>  
>  	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
> -	if (!vcpu) {
> -		r = -ENOMEM;
> -		goto vcpu_decrement;
> -	}
> +	if (!vcpu)
> +		return -ENOMEM;
>  
> -	r = kvm_vcpu_init_common(vcpu, kvm);
> +	r = kvm_vcpu_init_common(vcpu, kvm, id);
>  	if (r)
>  		goto vcpu_free;
>  
> +	vcpu->vcpu_idx = vcpu->common->vcpu_idx;
> +
>  	BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE);
>  	page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>  	if (!page) {
> @@ -4303,7 +4336,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  		goto unlock_vcpu_destroy;
>  	}
>  
> -	vcpu->vcpu_idx = atomic_read(&kvm->online_vcpus);
>  	r = xa_insert(&kvm->planes[0]->vcpu_array, vcpu->vcpu_idx, vcpu, GFP_KERNEL_ACCOUNT);
>  	WARN_ON_ONCE(r == -EBUSY);

[Severity: High]
Can a vcpu_idx collision from kvm_vcpu_init_common() trigger this
WARN_ON_ONCE?

If concurrent vCPU creations get the same vcpu_idx, this xa_insert() will
fail with -EBUSY. Since this WARN_ON_ONCE is reachable by an unprivileged
host-userspace process, it could be used to arbitrarily flood the kernel log
or crash the host if panic_on_warn is set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=12