Re: [PATCH 11/21] KVM: arm64: Context swap Partitioned PMU guest registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] TOCTOU race in `kvm_pmu_put()` leads to lost hardware overflows and inconsistent counter values.
- [High] `kvm_pmu_put()` clobbers the guest's virtual `PMCR_EL0` with host physical read-only fields.
- [High] Out-of-bounds bit shift and undefined behavior in `GENMASK_ULL` for `kvm_pmu_host_counter_mask()`.
- [High] Missing mandatory `isb()` after writing control-plane PMU registers in `kvm_pmu_put()`.
- [High] Guest accesses to the trapped `PMOVSSET_EL0` register return stale data, causing the guest to permanently miss PMU overflows.
--

commit 1e067bf0381df5bfe1db83a20f08aa650fd65999
Author: Colton Lewis <coltonlewis@google.com>

KVM: arm64: Context swap Partitioned PMU guest registers

This commit saves and restores untrapped PMU registers when the PMU is
partitioned, bypassing the context swap for emulated vPMUs. It also clears
PMOVS guest hardware counters to avoid generating spurious interrupts.

> diff --git a/arch/arm64/kvm/pmu-direct.c b/arch/arm64/kvm/pmu-direct.c
> index 43f04c2f33d44..79022447cfb9a 100644
> --- a/arch/arm64/kvm/pmu-direct.c
> +++ b/arch/arm64/kvm/pmu-direct.c

[ ... ]

> +u64 kvm_pmu_host_counter_mask(struct arm_pmu *pmu)
> +{
> +	u8 nr_counters = *host_data_ptr(nr_event_counters);
> +
> +	if (pmu_is_partitioned(pmu))
> +		return GENMASK_ULL(nr_counters - 1, pmu->max_guest_counters);

[Severity: High]
Does this code trigger undefined behavior if the PMU implements zero event
counters or if the guest is configured to use all available counters?

If nr_counters == 0, nr_counters - 1 evaluates to -1, resulting in
GENMASK_ULL(-1, 0), which expands to a right shift by 64.

If max_guest_counters == nr_counters, the high bound is strictly less than
the low bound, violating the macro's constraints. Both cases require
explicit bounds checking.

> +
> +	return ARMV8_PMU_CNT_MASK_ALL;
> +}

[ ... ]

> +void kvm_pmu_put(struct kvm_vcpu *vcpu)
> +{

[ ... ]

> +	val = read_sysreg(pmselr_el0);
> +	__vcpu_assign_sys_reg(vcpu, PMSELR_EL0, val);
> +
> +	val = read_sysreg(pmcr_el0);
> +	__vcpu_assign_sys_reg(vcpu, PMCR_EL0, val);

[Severity: High]
Can this clobber the guest's virtual PMCR_EL0 with host-specific read-only
fields?

By reading the raw hardware pmcr_el0 and directly assigning it to the vCPU
state, KVM overwrites the virtualized read-only fields (such as IMP, IDCODE,
and N) with the host's physical values.

This might serialize the leaked host topology when userspace reads the
register, causing migration issues if the VM is moved to a host with
different PMU capabilities.

> +
> +	/* Mask these to only save the guest relevant bits. */
> +	mask = kvm_pmu_guest_counter_mask(pmu);

[ ... ]

> +	/* Save pending guest hardware overflows. */
> +	local_irq_save(flags);
> +	val = read_sysreg(pmovsset_el0);
> +	__vcpu_rmw_sys_reg(vcpu, PMOVSSET_EL0, |=, val & mask);
> +	write_sysreg(val & mask, pmovsclr_el0);
> +	local_irq_restore(flags);
> +
> +	/* Stop guest counters and disable interrupts in hardware. */
> +	write_sysreg(mask, pmcntenclr_el0);

[Severity: High]
Does this create a race condition where hardware overflows can be
permanently lost?

The hardware counters are still running while kvm_pmu_put() reads their
state and the overflow register. A counter might overflow in the window
between reading pmovsset_el0 and stopping the counters with pmcntenclr_el0.

If the guest had disabled physical PMU interrupts for that counter, no IRQ
fires to rescue the overflow state. The overflow bit remains set in hardware
but is missed by KVM, and when the vCPU is scheduled back in, kvm_pmu_load()
will unconditionally clear pmovsclr_el0, destroying the unrecorded overflow.

Also, does this design lead to stale reads when the guest polls for
overflows?

Since the counters run directly in hardware without syncing back to memory
until vcpu_put, if a guest traps to pmu_reg_read() in
arch/arm64/kvm/sys_regs.c to read PMOVSSET_EL0:

pmu_reg_read()
    case PMOVSSET_EL0:
        val = __vcpu_sys_reg(vcpu, reg);
        break;

It will receive the stale in-memory value instead of seeing the active
hardware overflow accumulating in the physical register.

> +	write_sysreg(mask, pmintenclr_el1);
> +
> +	kvm_pmu_set_guest_counters(pmu, 0);
> +	preempt_enable();
> +}

[Severity: High]
Are we missing a mandatory Context Synchronization Event here?

After writing to control-plane registers like pmcntenclr_el0 and
pmintenclr_el1, an isb() must follow to ensure the CPU pipeline
resolves the new configuration.

Without the barrier, the CPU could continue executing the host kernel using
a stale configuration, potentially allowing hardware counters to run and
deliver spurious physical PMU interrupts during host execution.

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