Re: [RFC PATCH v7 13/28] KVM: arm64: Add SPE VCPU device attribute to set the SPE device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Incorrect declaration of `supported_cpus` causes stack corruption and NULL pointer dereferences when `CONFIG_CPUMASK_OFFSTACK=n`.
- [Medium] Changing the PMU identifier after SPE is configured incorrectly accumulates CPU mask restrictions, potentially rejecting valid reassignments.
--

commit b2f754414c4648e5b2996f0b8f1a6f27ec6f0cf7
Author: Alexandru Elisei <alexandru.elisei@arm.com>

KVM: arm64: Add SPE VCPU device attribute to set the SPE device

This commit introduces a new KVM device attribute to assign an SPE
instance to a virtual machine. It adds support for tracking physical
CPUs that share the assigned SPE and PMU instances, ensuring VCPUs
only run on appropriate hardware.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 763b102dc51fa..132df44c30887 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c

[ ... ]

> @@ -612,6 +614,47 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
>  
>  }
>  
> +int kvm_update_supported_cpus(struct kvm *kvm, cpumask_t *pmu_cpus,
> +			      cpumask_t *spe_cpus)
> +{
> +	cpumask_t *supported_cpus;
> +	cpumask_t *cpus;
> +	int ret;
> +
> +	if (!zalloc_cpumask_var(&supported_cpus, GFP_KERNEL_ACCOUNT))
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Critical]
Does this cause stack memory corruption and a NULL pointer dereference
when CONFIG_CPUMASK_OFFSTACK=n?

In kvm_update_supported_cpus(), supported_cpus is declared as a simple pointer
rather than cpumask_var_t. When CONFIG_CPUMASK_OFFSTACK is disabled,
cpumask_var_t is defined as a 1-element array. Passing &supported_cpus to
zalloc_cpumask_var() will zero out NR_CPUS bits starting at the stack address
of the pointer itself, overwriting the pointer and potentially adjacent
stack variables.

The subsequent cpumask_copy() then dereferences a NULL pointer. Could we
use cpumask_var_t for the declaration here?

> +		return -ENOMEM;
> +
> +	cpumask_copy(supported_cpus, kvm->arch.supported_cpus);
> +
> +	lockdep_assert_held(&kvm->arch.config_lock);
> +
> +	ret = -EINVAL;
> +	/* Cannot update for SPE and PMU at the same time. */
> +	if (WARN_ON_ONCE(pmu_cpus && spe_cpus))
> +		goto out;
> +
> +	if (pmu_cpus)
> +		cpus = pmu_cpus;
> +	else
> +		cpus = spe_cpus;
> +
> +	if ((spe_cpus && kvm->arch.arm_pmu) ||
> +	    (pmu_cpus && kvm_spe_pmu_set(kvm)))
> +		cpumask_and(supported_cpus, cpus, supported_cpus);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Could this incorrectly accumulate CPU mask restrictions and reject valid
reassignments?

If userspace configures a VM by first setting a PMU, then setting the SPE ID,
and later changes the PMU to a different instance (which is permitted before
the VM runs), this logic intersects the new PMU mask with the
already-intersected kvm->arch.supported_cpus mask.

Since the current mask already includes the first PMU's restrictions, the new
mask accumulates both PMUs' restrictions instead of replacing the old PMU's
mask. If the two PMUs have disjoint CPU masks, the intersection becomes empty
and the operation fails with -EINVAL. 

> +	else
> +		cpumask_copy(supported_cpus, cpus);
> +
> +	if (cpumask_empty(supported_cpus))
> +		goto out;
> +
> +	cpumask_copy(kvm->arch.supported_cpus, supported_cpus);
> +	ret = 0;
> +out:
> +	free_cpumask_var(supported_cpus);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903160623.315525-1-alexandru.elisei@arm.com?part=13