Re: [PATCH v8 01/11] KVM: arm64: Serialize userspace MDCR_EL2 access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing kvm_vm_has_ran_once() check allows KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS to cause data races on MDCR_EL2 for running vCPUs.
- [High] Changes to MDCR_EL2 fields (HPMN, HPMD, HLP) fail to trigger a PMU reload, leaving active perf events with a stale configuration.
- [High] Secondary KVM_ARM_VCPU_INIT calls bypass config_lock, causing data races with PMU configuration on MDCR_EL2.
--

--- Patch [1]: [PATCH v8 01/11] KVM: arm64: Serialize userspace MDCR_EL2 access ---
commit baa2d20fcb9f1e3b0daa7a4b72d1709acdab0380
Author: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

KVM: arm64: Serialize userspace MDCR_EL2 access

This patch addresses a race condition between PMU configuration and register
restore by adding explicit userspace accessors for MDCR_EL2. It serializes
these accesses using config_lock to ensure whole-register writes do not
conflict with HPMN rewrites.

> kvm_arm_set_nr_counters() updates MDCR_EL2.HPMN for every vCPU while
> holding kvm->arch.config_lock. However, KVM_SET_ONE_REG currently writes
> MDCR_EL2 through the generic sysreg path without taking the same lock.
> Concurrent PMU configuration and register restore can therefore race and
> lose updates to unrelated MDCR_EL2 bits.

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_arm_set_nr_counters()
still have a race with running vCPUs?

Userspace can invoke KVM_ARM_VCPU_PMU_V3_SET_NR_COUNTERS on an uninitialized
vCPU while another vCPU in the same VM is running. The ioctl handler in
arch/arm64/kvm/pmu-emul.c:kvm_arm_pmu_v3_set_attr() does not check
kvm_vm_has_ran_once(kvm).

It proceeds to call kvm_arm_set_nr_counters(), which performs a read-modify-write
of MDCR_EL2 across all vCPUs 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);
	}
}

If a running vCPU concurrently traps on MDCR_EL2, access_mdcr() will perform a
racing read-modify-write. Could this cause data corruption of the guest's
MDCR_EL2 sysreg?

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 33c921df19b54..6ab46f21aa614 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c

[ ... ]

> +static int set_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> +		    u64 val)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +	u64 old, hpmn = FIELD_GET(MDCR_EL2_HPMN, val);
> +
> +	guard(mutex)(&kvm->arch.config_lock);

[Severity: High]
This isn't a bug introduced by this patch, but does the use of config_lock here
completely prevent races with KVM_ARM_VCPU_INIT?

While this patch adds config_lock to serialize userspace writes against
kvm_arm_set_nr_counters(), MDCR_EL2 is also updated during KVM_ARM_VCPU_INIT.

For secondary inits (already initialized vCPUs), the config_lock is bypassed:

arch/arm64/kvm/arm.c:kvm_vcpu_set_target() {
	if (!kvm_vcpu_initialized(vcpu))
		return __kvm_vcpu_set_target(vcpu, init);

	if (kvm_vcpu_init_changed(vcpu, init))
		return -EINVAL;

	kvm_reset_vcpu(vcpu);
	return 0;
}

This allows reset_mdcr() to read nr_pmu_counters and write MDCR_EL2 without
config_lock, which could race with kvm_arm_set_nr_counters() modifying all
vCPUs' MDCR_EL2 registers.

> +
> +	if (hpmn > vcpu->kvm->arch.nr_pmu_counters)
> +		return -EINVAL;
> +
> +	old = __vcpu_sys_reg(vcpu, MDCR_EL2);
> +	__vcpu_assign_sys_reg(vcpu, MDCR_EL2, val);
> +
> +	/*
> +	 * Request a reload of the PMU to enable/disable the counters
> +	 * affected by HPME.
> +	 */
> +	if ((old ^ val) & MDCR_EL2_HPME)
> +		kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
> +	return 0;
> +}

[Severity: High]
This is a pre-existing issue, but does checking only HPME leave the PMU in a
stale configuration if other fields change?

Both the newly added set_mdcr() and the existing access_mdcr() only request a
PMU reload when the MDCR_EL2_HPME bit toggles. However, if fields like HPMN,
HPMD, or HLP change, shouldn't active perf events be re-evaluated to reflect
the new semantics?

If a guest or userspace modifies these fields but leaves HPME unchanged, could
host perf events count incorrectly or run with stale privilege exclusion
semantics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-hybrid-v8-0-621409f3a592@rsg.ci.i.u-tokyo.ac.jp?part=1