Re: [PATCH 17/60] kvm: Move VCPU locking to struct kvm_vcpu_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Interrupt handlers for KVM arch timers and VGIC maintenance dereference `common->current_vcpu` before verifying if `common` is `NULL`, causing a host kernel panic.
- [High] TOCTOU race in KVM_CREATE_VCPU allows userspace to bypass kvm->max_vcpus limit by moving `kvm->created_vcpus` increment out of the `kvm->lock` critical section.
- [High] Assigning `vcpu->vcpu_idx` locklessly in `kvm_vcpu_init_common()` allows concurrent vCPU creations to evaluate the same index, causing a `-EBUSY` error in `xa_insert()` and triggering a host DoS via `WARN_ON_ONCE()`.
--

commit 67b86047d814218cd13d6ae4125264614146e77c
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: Move VCPU locking to struct kvm_vcpu_common

Fields in struct kvm_vcpu which are protected by these locks is going
to move to struct kvm_vcpu_common. So move the locks as well.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 611bba515ac07..c8085c23e18e8 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -330,6 +330,13 @@ struct kvm_vcpu_common {
>  	/* Currently active VCPU */
>  	struct kvm_vcpu *current_vcpu;

[Severity: Critical]
This isn't a bug introduced by this patch, but from an earlier patch
in this series. Does this cause a crash?

In the arm64 timer and VGIC interrupt handlers, common is dereferenced
to retrieve current_vcpu before checking if common itself is NULL:

arch/arm64/kvm/arch_timer.c:kvm_arch_timer_handler() {
    struct kvm_vcpu_common *common = *(struct kvm_vcpu_common **)dev_id;
    struct kvm_vcpu *vcpu = common->current_vcpu;
    ...
}

arch/arm64/kvm/vgic/vgic-init.c:vgic_maintenance_handler() {
    struct kvm_vcpu_common *common = *(struct kvm_vcpu_common **)data;
    struct kvm_vcpu *vcpu = common->current_vcpu;
    ...
}

Since kvm_running_vcpu is cleared during vcpu_put() and kvm_sched_out(),
an interrupt firing shortly after could observe a NULL common pointer.

Would this lead to a crash when common->current_vcpu is dereferenced?

[ ... ]
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 009adede51ba6..84ac6792a2227 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -466,6 +466,8 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned
>  
>  	common->vcpu_idx = atomic_read(&kvm->online_vcpus);

[Severity: High]
This isn't a bug introduced by this patch, but from an earlier patch
in this series. Is there a race condition here?

In kvm_vm_ioctl_create_vcpu(), the maximum vCPU limit is checked under
kvm->lock, but the lock is then dropped:

virt/kvm/kvm_main.c:kvm_vm_ioctl_create_vcpu() {
    ...
    if (kvm->created_vcpus >= kvm->max_vcpus) {
        mutex_unlock(&kvm->lock);
        return -EINVAL;
    }
    ...
}

Later, in kvm_vcpu_init_common(), the kvm->created_vcpus increment happens
under a newly acquired lock:

virt/kvm/kvm_main.c:kvm_vcpu_init_common() {
    ...
    mutex_lock(&kvm->lock);
    kvm->created_vcpus++;
    mutex_unlock(&kvm->lock);
    ...
}

Because the lock is dropped between the check and the increment, concurrent
vCPU creations can pass the limit check simultaneously. 

Can this allow an unbounded number of vCPUs to be created, bypassing capacity
constraints and potentially causing a host-side denial of service?

[Severity: High]
This is also a pre-existing issue, but could this lockless assignment cause
a denial of service?

Because common->vcpu_idx = atomic_read(&kvm->online_vcpus); is assigned
without holding a lock, concurrent creations can evaluate the same index.

They later serialize on kvm->lock before insertion:

virt/kvm/kvm_main.c:kvm_vm_ioctl_create_vcpu() {
    ...
    r = xa_insert(&kvm->planes[0]->vcpu_array, vcpu->vcpu_idx, vcpu,
                  GFP_KERNEL_ACCOUNT);
    WARN_ON_ONCE(r == -EBUSY);
    ...
}

Will the second thread attempt to insert at the already-occupied index, fail
with -EBUSY, and unexpectedly trigger the WARN_ON_ONCE()? 

As this is reachable by unprivileged userspace, could this crash the host if
panic_on_warn is set?

>  
> +	mutex_init(&common->mutex);
> +
>  	common->kvm = kvm;
>  	common->current_vcpu = vcpu;

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