Re: [PATCH v7 08/17] KVM: x86/pmu: Check mediated PMU counter enablement before event filters

[Sean Christopherson <seanjc@google.com>]

On Thu, May 28, 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Guest can bypass PMU event filters for fixed counters by modifying MSR_CORE_PERF_FIXED_CTR_CTRL.
> --
> 
> commit 270fc772a7eb457f31ead154a0c426817961ae8a
> Author: Yosry Ahmed <yosry@kernel.org>
> 
> KVM: x86/pmu: Check mediated PMU counter enablement before event filters
> 
> This commit checks if a PMU counter is enabled by the guest before performing
> the potentially expensive PMU event filter lookup. It modifies
> kvm_mediated_pmu_refresh_event_filter to use pmc_is_locally_enabled.
> 
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index afbc731e72174..67dbbd4c73036 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -532,7 +532,7 @@ static bool pmc_is_event_allowed(struct kvm_pmc *pmc)
> >  
> >  static void kvm_mediated_pmu_refresh_event_filter(struct kvm_pmc *pmc)
> >  {
> > -	bool allowed = pmc_is_event_allowed(pmc);
> > +	bool allowed = pmc_is_locally_enabled(pmc) && pmc_is_event_allowed(pmc);
> 
> [Severity: High]
> This is a pre-existing issue, but was it intended for guests to be able to
> bypass PMU event filters for fixed counters by writing to the MSR twice?
> 
> Looking at reprogram_fixed_counters() in arch/x86/kvm/vmx/pmu_intel.c,
> pmu->fixed_ctr_ctrl_hw is unconditionally overwritten with raw guest data
> before checking which counters changed:
> 
> arch/x86/kvm/vmx/pmu_intel.c:reprogram_fixed_counters() {
>     ...
> 	pmu->fixed_ctr_ctrl = data;
> 	pmu->fixed_ctr_ctrl_hw = data;
> 	for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
> 		u8 new_ctrl = fixed_ctrl_field(data, i);
> 		u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
> 
> 		if (old_ctrl == new_ctrl)
> 			continue;
>     ...
> 		kvm_pmu_request_counter_reprogram(pmc);
> 	}
> }
> 
> Because kvm_pmu_request_counter_reprogram() is skipped when the counter's
> control bits haven't changed, kvm_mediated_pmu_refresh_event_filter() is
> never called to apply the filter mask.
> 
> Could a guest enable a rejected counter (which gets correctly filtered and
> cleared in fixed_ctr_ctrl_hw by the first write), and then write the MSR a
> second time to enable a different counter while leaving the rejected
> counter's bits set?
> 
> The second write would inject the unfiltered enable bits into
> fixed_ctr_ctrl_hw, and since the software state (old_ctrl == new_ctrl)
> matches, KVM skips reprogramming it. Does this allow the rejected counter to
> remain enabled in hardware upon the next VM-enter, completely bypassing the
> event filters?

Hrm, does look like that's the case.  If the guest toggles a different PMC, KVM
will update pmu->fixed_ctr_ctrl_hw and reprogram the PMC that is changing, but
not the others that are enabled in pmu->fixed_ctr_ctrl_hw.

Aha!  I think this is quite easy to fix: use fixed_ctr_ctrl_hw as the basis for
the diff.  For the perf-based PMU, fixed_ctr_ctrl_hw == fixed_ctr_ctrl at all
times.  For the mediated PMU, "reprogramming" means changes the values that are
loaded into hardware, and so unless I'm missing something, this?  I'll test and
send a patch next week.

idiff --git arch/x86/kvm/vmx/pmu_intel.c arch/x86/kvm/vmx/pmu_intel.c
index 453cb3d3ec9b..a73a9515d96c 100644
--- arch/x86/kvm/vmx/pmu_intel.c
+++ arch/x86/kvm/vmx/pmu_intel.c
@@ -56,8 +56,16 @@ static struct x86_pmu_lbr *vcpu_to_lbr_records(struct kvm_vcpu *vcpu)
 
 static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
 {
+       /*
+        * Compare against the value the mediated PMU shoves into hardware, not
+        * the guest's desired value.  For the emulated PMU (proxied via perf),
+        * they are one and the same (fixed_ctr_ctrl_hw isn't used other than
+        * here).  For the mediated PMU, KVM needs to reprogram the actual MSR,
+        * and so needs to react to potential changes in the value shoved into
+        * hardware, e.g. to ensure the event filter is enforced.
+        */
+       u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl_hw;
        struct kvm_pmc *pmc;
-       u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
        int i;
 
        pmu->fixed_ctr_ctrl = data;


Note, general purpose counters (thankfully) don't have the same bug.

			if (data != pmc->eventsel) {
				pmc->eventsel = data;
				pmc->eventsel_hw = data;
				kvm_pmu_request_counter_reprogram(pmc);
			}

Oh, and for posterity, using fixed_ctr_ctrl_hw won't mess up pmc_in_use, because
it's unused by the mediated PMU.  It's purpose is solely for releasing perf events
that are no longer being actively used.  It's a bit hard to see, but need_cleanup
will never be set for the mediated PMU, because pmu->event_count will always be
zero.

	if (vcpu->scheduled_out && pmu->version && pmu->event_count) {
		pmu->need_cleanup = true;
		kvm_make_request(KVM_REQ_PMU, vcpu);
	}

To make that more obvious, I think it makes sense to add

  static inline void __kvm_pmu_mark_pmc_in_use(struct kvm_vcpu *vcpu, u8 idx)
 {
	if (kvm_vcpu_has_mediated_pmu(vcpu))
		return;

	__set_bit(idx, vcpu_to_pmu(vcpu)->pmc_in_use);
  }

and then use that everywhere, and WARN if kvm_pmu_cleanup() is ever called with
a mediated PMU.