Re: [PATCH 35/60] kvm: Add VCPU plane-scheduling state and helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Error path during secondary VCPU creation destroys shared structure, causing Use-After-Free.
- [High] Missing IPI kick to the active VCPU prevents prompt preemption during plane switch.
- [High] Unhandled KVM_REQ_PLANE_RESCHED request causes infinite vCPU loop.
- [High] Concurrent VCPU creation bypasses ID checks, allowing unprivileged userspace to trigger a host DoS via WARN_ON_ONCE.
- [High] Non-atomic state transitions in plane switching result in state corruption and force plane 0 to execute.
--

commit 1f17dacf62458889297d745dc273354c7b7cb798
Author: Joerg Roedel <joerg.roedel@amd.com>

KVM: Plane switching algorithm implementation

This commit introduces the algorithm to run the lowest runnable plane.
It implements plane switches by stopping the current plane and setting
another to runnable, adding state tracking and requests to manage this.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 5c3f9dfa15ea..e3611e6cc3e4 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -168,6 +168,7 @@ static inline bool kvm_is_error_gpa(gpa_t gpa)
>  #define KVM_REQ_VM_DEAD			(1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_UNBLOCK			2
>  #define KVM_REQ_DIRTY_RING_SOFT_FULL	3
> +#define KVM_REQ_PLANE_RESCHED		4

[Severity: High]
This patch adds KVM_REQ_PLANE_RESCHED and asserts it via kvm_make_request(),
but is there a corresponding handler to process and clear this request?

Without a call to kvm_clear_request(), the bit remains permanently set.
Could this cause kvm_request_pending() to perpetually return true,
forcing the VCPU to endlessly abort guest entry and spin in a soft lockup?

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9d30fd85ce5f..a30123b77112 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4397,6 +4397,7 @@ static int kvm_plane_ioctl_create_vcpu(struct kvm_plane *plane, unsigned long id
>  	vcpu->vcpu_idx = vcpu->common->vcpu_idx;
>  	vcpu->plane = plane;
>  	vcpu->plane_level = plane->level;
> +	vcpu->plane_state = STOPPED;
>  	vcpu->run = vcpu->common->run;
>  
>  	kvm_vcpu_init(vcpu, kvm, id);

[Severity: High]
Can a concurrent VCPU creation bypass the ID checks in this function?

Looking at kvm_plane_ioctl_create_vcpu(), the plane_get_vcpu_by_id() check
is performed locklessly before acquiring kvm->lock:

	r = -EEXIST;
	if (plane_get_vcpu_by_id(plane, id))
		goto vcpu_free;

	...
	mutex_lock(&kvm->lock);
	r = xa_insert(&plane->vcpu_array, vcpu->vcpu_idx, vcpu, ...);
	WARN_ON_ONCE(r == -EBUSY);

If a malicious userspace process concurrently creates multiple VCPUs with the
same ID, could multiple threads pass the existence check?

When they subsequently acquire the lock to insert into the xarray, the
second thread would receive -EBUSY and hit the WARN_ON_ONCE(r == -EBUSY)
inside the lock. Could this regression allow unprivileged userspace to trigger
a host denial-of-service if panic_on_warn is set?

[Severity: Critical]
Does the error path for secondary VCPU creation safely handle the shared
common structure?

When creating a secondary VCPU, it inherits the common state from the plane 0
VCPU:

	vcpu->common = vcpu_plane0->common;

If a subsequent step like kvm_arch_vcpu_create() fails, the error path jumps
to vcpu_free_common, which unconditionally executes
kvm_vcpu_common_destroy(vcpu):

vcpu_free_common:
	kvm_vcpu_common_destroy(vcpu);

Because this destruction logic does not verify whether the common struct
is exclusively owned by the failing VCPU, could it destroy the shared
structure while the plane 0 VCPU is still actively using it, resulting
in a use-after-free and state corruption for the primary plane?

[ ... ]

> @@ -4938,6 +4939,50 @@ static struct file_operations kvm_plane_fops = {
>  	KVM_COMPAT(kvm_plane_ioctl),
>  };
>  
> +void kvm_vcpu_set_plane_runnable(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->plane_state = RUNNABLE;
> +	vcpu->common->plane_switch = true;
> +	kvm_make_request(KVM_REQ_PLANE_RESCHED, vcpu);
> +}
> +EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_vcpu_set_plane_runnable);

[Severity: High]
Is this request being sent to the correct VCPU?

When switching planes, KVM_REQ_PLANE_RESCHED is sent to the target vcpu.
Because the target VCPU is often inactive (in the STOPPED state, not loaded
on a physical CPU, and not in guest mode), kvm_vcpu_kick() will not issue
an IPI.

Will this prevent the currently running VCPU (vcpu->common->current_vcpu)
from being promptly preempted, allowing it to continue executing until an
unrelated VM exit occurs?

[ ... ]

> +struct kvm_vcpu *kvm_vcpu_select_plane(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_vcpu_common *common = vcpu->common;
> +	struct kvm_vcpu *ret = NULL;
> +	unsigned i;
> +
> +	for (i = 0; i < KVM_MAX_PLANES; i++) {
> +		if (common->vcpus[i] == NULL)
> +			continue;
> +
> +		if (common->vcpus[i]->plane_state == RUNNABLE) {
> +			ret = common->vcpus[i];
> +			break;
> +		}
> +	}
> +
> +	if (ret == NULL) {
> +		ret = common->vcpus[0];
> +		ret->plane_state = RUNNABLE;
> +	}

[Severity: High]
Could this fallback resurrect plane 0 during an in-progress plane switch?

Plane switching involves independent calls to kvm_vcpu_set_plane_stopped(old)
and kvm_vcpu_set_plane_runnable(new). This non-atomic sequence creates a race
window where all planes temporarily appear STOPPED.

If kvm_vcpu_select_plane() runs during this window, this fallback logic
forces plane 0 to RUNNABLE. When the target plane is subsequently marked
runnable by the transitioning thread, both plane 0 and the target plane
are RUNNABLE.

Because select_plane() always selects the lowest index, will this trap the
VCPU on plane 0 and defeat the intended switch to the higher plane?

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