Re: [PATCH v2 08/17] KVM: arm64: Handle PSCI calls for protected VMs at EL2

[sashiko-bot@kernel.org]

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

New issues:
- [High] The error path in `pkvm_reset_vcpu()` leaves `reset_state->reset = true`, breaking the synchronization barrier for subsequent `CPU_ON` calls.

Pre-existing issues:
- [High] Memory leak of SVE state pages when VM-wide initialization fails, due to clearing `KVM_ARM_VCPU_SVE`.
--

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

    KVM: arm64: Handle PSCI calls for protected VMs at EL2

This commit implements PSCI 1.1 handling at EL2 for protected VMs. It manages
the vCPU power state transitions between OFF, ON_PENDING, and ON, and
intercepts PSCI calls like CPU_ON, CPU_OFF, and AFFINITY_INFO. Calls that
require host intervention, such as scheduling, are safely forwarded.

> diff --git a/arch/arm64/kvm/hyp/nvhe/pkvm.c b/arch/arm64/kvm/hyp/nvhe/pkvm.c
> index 69f13362b7ab0..4ce79e191ff45 100644
> --- a/arch/arm64/kvm/hyp/nvhe/pkvm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/pkvm.c

[ ... ]

> @@ -407,6 +442,9 @@ static void unpin_host_sve_state(struct pkvm_hyp_vcpu *hyp_vcpu)
>  		return;
>  
>  	sve_state = hyp_vcpu->vcpu.arch.sve_state;
> +	if (!sve_state)
> +		return;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
unpin_host_sve_state() cause a memory leak of SVE state pages when VM-wide
initialization fails?

If pkvm_vcpu_init_sve() fails on a secondary vCPU (for example, due to an
invalid configuration from host userspace), the error path in
pkvm_vcpu_init_sve() clears the KVM_ARM_VCPU_SVE bit in the VM-wide
vcpu_features bitmap.

During the subsequent teardown of the VM, unpin_host_vcpus() calls
unpin_host_sve_state() for all vCPUs. Because the feature bit was cleared,
unpin_host_sve_state() returns early and fails to unpin the SVE state
memory that was successfully pinned for previously initialized vCPUs. This
allows for a denial of service via resource exhaustion from host userspace.

>  	hyp_unpin_shared_mem(sve_state,
>  			     sve_state + vcpu_sve_state_size(&hyp_vcpu->vcpu));
>  }

[ ... ]

> +int pkvm_reset_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
> +{
> +	struct vcpu_reset_state *reset_state = &hyp_vcpu->vcpu.arch.reset_state;
> +	int prev;
> +
> +	/*
> +	 * Pairs with smp_store_release(&reset_state->reset, true) in
> +	 * pvm_psci_vcpu_on(). The acquire must precede the cmpxchg: reversed, a
> +	 * winning cmpxchg with a false acquire would leave power_state == ON
> +	 * with the reset skipped.
> +	 */
> +	if (!smp_load_acquire(&reset_state->reset))
> +		return -ECANCELED;
> +
> +	prev = cmpxchg_relaxed(&hyp_vcpu->power_state,
> +			       PSCI_0_2_AFFINITY_LEVEL_ON_PENDING,
> +			       PSCI_0_2_AFFINITY_LEVEL_ON);
> +	if (prev != PSCI_0_2_AFFINITY_LEVEL_ON_PENDING) {
> +		/* The only other writer of ON_PENDING is the rollback. */
> +		WARN_ON(prev != PSCI_0_2_AFFINITY_LEVEL_OFF);
> +		return -ECANCELED;

[Severity: High]
Does the -ECANCELED error path in pkvm_reset_vcpu() leave
reset_state->reset set to true, breaking the synchronization barrier for
subsequent CPU_ON calls?

If a CPU_ON hypercall is followed by a host rollback (which sets power_state
back to OFF but intentionally leaves reset_state.reset = true), a
subsequent CPU_ON from the guest sets power_state to ON_PENDING, writes
the new pc/r0, and sets reset_state->reset = true.

However, because reset_state->reset was already left as true, if the target
vCPU executes pkvm_reset_vcpu() concurrently, its
smp_load_acquire(&reset_state->reset) will immediately succeed. The target
vCPU then reads pc and r0 before the CPU_ON call has finished writing them,
resulting in a data race and the guest booting with corrupted state.

> +	}
> +
> +	kvm_reset_vcpu_core(&hyp_vcpu->vcpu);
> +	kvm_reset_pvm_sys_regs(&hyp_vcpu->vcpu);
> +
> +	/* Must be done after resetting sys registers. */
> +	kvm_reset_vcpu_psci(&hyp_vcpu->vcpu, reset_state);
> +
> +	hyp_vcpu->exit_code = 0;
> +	/*
> +	 * power_state == ON rules out the other two writers, the source
> +	 * cmpxchg(OFF, ON_PENDING) and the rollback cmpxchg(ON_PENDING, OFF).
> +	 */
> +	reset_state->reset = false;
> +	return 0;
> +}

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