Re: [PATCH v4 16/18] KVM: arm64: Reject host power-on of a vCPU that EL2 holds powered off

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: PSCI CPU_ON spuriously fails for protected secondary vCPUs
- [Medium] arm64: KVM_SET_MP_STATE rejects KVM_MP_STATE_UNINITIALIZED with -EINVAL
--

commit f4c00ca4b159d4b7954978f9de7770cc8b67b029
Author: Fuad Tabba <fuad.tabba@linux.dev>

KVM: arm64: Reject host power-on of a vCPU that EL2 holds powered off

This patch records a protected vCPU held powered off by EL2 as
KVM_MP_STATE_UNINITIALIZED. It restricts KVM_SET_MP_STATE from overwriting
this value with RUNNABLE or SUSPENDED, and updates the guest CPU_ON checks
to gate on this state rather than STOPPED.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 14fe4e7275796..a89087c83e27f 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c

[ ... ]

> @@ -818,12 +829,24 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  
>  	switch (mp_state->mp_state) {
>  	case KVM_MP_STATE_RUNNABLE:
> +		if (kvm_pkvm_vcpu_is_powered_off(vcpu)) {
> +			ret = -EPERM;
> +			break;
> +		}
> +
>  		WRITE_ONCE(vcpu->arch.mp_state, *mp_state);
>  		break;
>  	case KVM_MP_STATE_STOPPED:
> -		__kvm_arm_vcpu_power_off(vcpu);
> +		/* Keep EL2's off record: STOPPED over it would let RUNNABLE through. */
> +		if (!kvm_pkvm_vcpu_is_powered_off(vcpu))
> +			__kvm_arm_vcpu_power_off(vcpu, KVM_MP_STATE_STOPPED);
>  		break;
>  	case KVM_MP_STATE_SUSPENDED:
> +		if (kvm_pkvm_vcpu_is_powered_off(vcpu)) {
> +			ret = -EPERM;
> +			break;
> +		}
> +
>  		kvm_arm_vcpu_suspend(vcpu);
>  		break;
>  	default:
>  		ret = -EINVAL;
>  	}

[Severity: Medium]
Does this break the userspace API for KVM_SET_MP_STATE?

The switch statement lacks a case for KVM_MP_STATE_UNINITIALIZED, defaulting
to -EINVAL for a state that KVM now actively exposes. If a userspace VMM
attempts to save and restore the state by reading via KVM_GET_MP_STATE and
later passing that exact state back via KVM_SET_MP_STATE, it will fail with
-EINVAL. Can we add a case to properly handle the uninitialized state?

[ ... ]

> @@ -1788,7 +1811,7 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,
>  	spin_lock(&vcpu->arch.mp_state_lock);
>  
>  	if (power_off)
> -		__kvm_arm_vcpu_power_off(vcpu);
> +		__kvm_arm_vcpu_power_off(vcpu, KVM_MP_STATE_STOPPED);

[Severity: High]
Can this cause a spurious PSCI_RET_ALREADY_ON failure when booting secondary
protected vCPUs?

When the VMM initializes a secondary protected vCPU with
KVM_ARM_VCPU_POWER_OFF, it explicitly hardcodes its mp_state to
KVM_MP_STATE_STOPPED here.

>  	else
>  		WRITE_ONCE(vcpu->arch.mp_state.mp_state, KVM_MP_STATE_RUNNABLE);
>  

[ ... ]

> diff --git a/arch/arm64/kvm/psci.c b/arch/arm64/kvm/psci.c
> index e3db84400d1f8..b01f8d44033f1 100644
> --- a/arch/arm64/kvm/psci.c
> +++ b/arch/arm64/kvm/psci.c
> @@ -63,7 +63,12 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
>  		return PSCI_RET_INVALID_PARAMS;
>  
>  	spin_lock(&vcpu->arch.mp_state_lock);
> -	if (!kvm_arm_vcpu_stopped(vcpu)) {
> +	/*
> +	 * A protected vCPU is off once the host has acted on EL2's CPU_OFF.
> +	 * STOPPED before that is the VMM's pause.
> +	 */
> +	if (!kvm_arm_vcpu_stopped(vcpu) ||
> +	    (vcpu_is_protected(vcpu) && !kvm_pkvm_vcpu_is_powered_off(vcpu))) {
>  		if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1)
>  			ret = PSCI_RET_ALREADY_ON;
>  		else

[Severity: High]
Following up on the KVM_ARM_VCPU_POWER_OFF initialization above, does this
condition incorrectly reject guest CPU_ON requests?

If the primary vCPU executes a PSCI CPU_ON call before the secondary vCPU
thread has executed its first KVM_RUN, __pkvm_create_hyp_vcpu() hasn't yet
upgraded the state to KVM_MP_STATE_UNINITIALIZED.

Consequently, when kvm_psci_vcpu_on() evaluates the condition above, it
sees the vCPU as STOPPED but not UNINITIALIZED. The condition evaluates
to true, causing it to return PSCI_RET_ALREADY_ON to the guest. This
deterministically breaks SMP boot for protected VMs on VMMs that block
secondary vCPU threads prior to wakeup. 

Should kvm_arch_vcpu_ioctl_vcpu_init() directly set the state to
KVM_MP_STATE_UNINITIALIZED for protected vCPUs to avoid this race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001135711.1640520-1-fuad.tabba@linux.dev?part=16