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

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Sashiko,

On Thu, Sep 03, 2026 at 04:39:44PM +0000, sashiko-bot@kernel.org wrote:
> 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?

This indeed look like a bug, I'll fix it, thanks for spotting it.

> 
> > +		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. 

My mistake here, I was under the impression that you can assign a PMU *exactly*
once to a VM, but looking at the code it turns out I was wrong and you can
assign different PMUs (where each assignment overwrites the previous one), as
long as you haven't set a filter. I'll fix it in the next iteration.

This begs the question: should the same behaviour be allowed with SPE? Because
right now trying to set a different SPE PMU is rejected by KVM.

Thanks,
Alex

> 
> > +	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