Re: [PATCH 03/21] KVM: arm64: Reorganize PMU functions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS ioctl modifies VM-wide state and VCPU system registers without checking if the VM has already run, allowing a data race with active VCPUs.
- [High] Host-userspace-reachable WARN_ONCE in __kvm_pmu_event_mask allows unprivileged userspace to trigger a Denial of Service.
--

commit 5436d8ff9f4b80e9bf5e0df08454fa9ddf4d9e29
Author: Colton Lewis <coltonlewis@google.com>

KVM: arm64: Reorganize PMU functions

A lot of functions in pmu-emul.c aren't specific to the emulated PMU
implementation. Move them to the more appropriate pmu.c file where
shared PMU functions should live.

> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 6b48a3d16d0d5..9ad3520417413 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c

[ ... ]

> +static u32 __kvm_pmu_event_mask(unsigned int pmuver)
> +{
> +	switch (pmuver) {
> +	case ID_AA64DFR0_EL1_PMUVer_IMP:
> +		return GENMASK(9, 0);
> +	case ID_AA64DFR0_EL1_PMUVer_V3P1:
> +	case ID_AA64DFR0_EL1_PMUVer_V3P4:
> +	case ID_AA64DFR0_EL1_PMUVer_V3P5:
> +	case ID_AA64DFR0_EL1_PMUVer_V3P7:
> +		return GENMASK(15, 0);
> +	default:		/* Shouldn't be here, just for sanity */
> +		WARN_ONCE(1, "Unknown PMU version %d\n", pmuver);
> +		return 0;
> +	}
> +}

[Severity: High]
This is a pre-existing issue, but can this warning be triggered by
unprivileged host userspace?

The pmuver here is extracted from the guest's SYS_ID_AA64DFR0_EL1 register.
KVM allows userspace to downgrade ID registers using safe semantics, meaning
userspace might purposefully set an unhandled PMUVer (such as 2 or 3) via the
KVM_SET_ONE_REG ioctl.

If this happens, wouldn't it hit this default case and trigger the warning,
potentially causing a host denial of service if panic_on_warn is enabled?

[ ... ]

> +static void kvm_arm_set_nr_counters(struct kvm *kvm, unsigned int nr)
> +{
> +	kvm->arch.nr_pmu_counters = nr;
> +
> +	/* Reset MDCR_EL2.HPMN behind the vcpus' back... */
> +	if (test_bit(KVM_ARM_VCPU_HAS_EL2, kvm->arch.vcpu_features)) {
> +		struct kvm_vcpu *vcpu;
> +		unsigned long i;
> +
> +		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);
> +		}
> +	}
> +}

[ ... ]

> +int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> +{

[ ... ]

> +	case KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS: {
> +		unsigned int __user *uaddr = (unsigned int __user *)(long)attr->addr;
> +		unsigned int n;
> +
> +		if (get_user(n, uaddr))
> +			return -EFAULT;
> +
> +		return kvm_arm_pmu_v3_set_nr_counters(vcpu, n);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but does this ioctl bypass the
kvm_vm_has_ran_once() check applied to other VM-wide attributes?

If userspace initializes VCPU 0, starts it via KVM_RUN, and then invokes this
ioctl on an uninitialized VCPU 1, the check for vcpu->arch.pmu.created at the
start of kvm_arm_pmu_v3_set_attr() would pass.

Since kvm_arm_set_nr_counters() loops over all VCPUs and performs a
read-modify-write on MDCR_EL2 without taking vcpu->mutex or pausing VCPUs:

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);
    }
}

Could this cause a data race on hypervisor control registers if VCPU 0 is
actively running, potentially allowing unprivileged userspace to corrupt
execution state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612192909.1153907-1-coltonlewis@google.com?part=3