Re: [PATCH v8 6/7] KVM: arm64: PMU: Introduce FIXED_COUNTERS_ONLY

[sashiko-bot@kernel.org]

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

New issues:
- [High] Host-userspace can trigger a WARN_ON_ONCE in KVM by scheduling a VCPU on a physical CPU without a PMU.
- [Medium] KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS can silently override the KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY restriction.

Pre-existing issues:
- [High] kvm_arm_set_nr_counters() modifies MDCR_EL2 for all vCPUs without acquiring their vcpu->mutex, causing a data race with KVM_SET_ONE_REG.
--

commit 18166de56dc2a0622e0ddd3b21dbd03b3b4d6bd7
Author: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

KVM: arm64: PMU: Introduce FIXED_COUNTERS_ONLY

Introduce the KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY attribute to
create a "fixed-counters-only" PMU.

> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c

[ ... ]

> @@ -1151,6 +1153,22 @@ static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)
>  	return ret;
>  }
>  
> +static int kvm_arm_pmu_v3_set_pmu_fixed_counters_only(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	lockdep_assert_held(&kvm->arch.config_lock);
> +
> +	if (kvm_vm_has_ran_once(kvm) || kvm->arch.pmu_filter ||
> +	    test_bit(KVM_ARCH_FLAG_PMU_V3_EXPLICIT, &kvm->arch.flags))
> +		return -EBUSY;
> +
> +	set_bit(KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY, &kvm->arch.flags);
> +	kvm_arm_set_nr_counters(kvm, 0);

[Severity: High]
Can an unprivileged host userspace trigger a WARN_ON_ONCE if they bind the
VCPU thread to a physical CPU without a PMU and run the VM?

If KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY is set, it appears
kvm_pmu_create_perf_event() will attempt to find a PMU using
kvm_pmu_probe_armpmu(vcpu->cpu):

arch/arm64/kvm/pmu-emul.c:kvm_pmu_create_perf_event() {
    ...
	if (kvm_pmu_fixed_counters_only(vcpu->kvm)) {
		arm_pmu = kvm_pmu_probe_armpmu(vcpu->cpu);
		if (WARN_ON_ONCE(!arm_pmu))
			return;
	}
    ...
}

If the CPU lacks a PMU, wouldn't this hit the WARN_ON_ONCE and act as a host
denial of service if panic_on_warn is set?


[Severity: High]
This is a pre-existing issue, but can this call to kvm_arm_set_nr_counters()
cause a data race with KVM_SET_ONE_REG?

Looking at kvm_arm_set_nr_counters(), it iterates over all vCPUs and modifies
__vcpu_sys_reg(vcpu, MDCR_EL2) "behind their back" without acquiring their
vcpu->mutex:

arch/arm64/kvm/pmu-emul.c:kvm_arm_set_nr_counters() {
    ...
		kvm_for_each_vcpu(i, vcpu, kvm) {
			u64 val = __vcpu_sys_reg(vcpu, MDCR_EL2);
			val &= ~MDCR_EL2_HPMN;
			val |= FIELD_PREP(MDCR_EL2_HPMN, kvm->arch.nr_pmu_counters);
			__vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
		}
    ...
}

Concurrently, userspace could call KVM_SET_ONE_REG on another vCPU to modify
MDCR_EL2 bits, leading to a read-modify-write corruption since the global
config_lock is not taken by KVM_SET_ONE_REG.

> +
> +	return 0;
> +}
> +
>  static int kvm_arm_pmu_v3_set_nr_counters(struct kvm_vcpu *vcpu, unsigned int n)
>  {

[ ... ]

> @@ -1270,6 +1288,8 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  
>  		return kvm_arm_pmu_v3_set_nr_counters(vcpu, n);
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_FIXED_COUNTERS_ONLY:
> +		return kvm_arm_pmu_v3_set_pmu_fixed_counters_only(vcpu);

[Severity: Medium]
If userspace configures the PMU for fixed-counters-only, can this state be
silently overwritten if userspace subsequently calls
KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS?

The kvm_arm_pmu_v3_set_nr_counters() function does not check if
KVM_ARCH_FLAG_PMU_V3_FIXED_COUNTERS_ONLY is active:

arch/arm64/kvm/pmu-emul.c:kvm_arm_pmu_v3_set_nr_counters() {
    ...
	if (n > kvm_arm_pmu_get_max_counters(kvm))
		return -EINVAL;

	kvm_arm_set_nr_counters(kvm, n);
	return 0;
}

Could this allow exposing programmable counters to the guest despite being
configured for fixed-counters-only?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-hybrid-v8-0-de459617b59d@rsg.ci.i.u-tokyo.ac.jp?part=6