Re: [PATCH v7 5/9] KVM: x86/pmu: Support PERF_METRICS MSR in mediated vPMU
"Chen, Zide" <[email protected]>
| Newsgroups | dev.linux.lists.sashiko-reviews,org.kernel.vger.kvm |
|---|---|
| Message-ID | <[email protected]> |
On 7/27/2026 2:50 PM, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: > - [High] Guest PMU state in MSR_PERF_METRICS is leaked to the host and other VMs because KVM fails to clear it on PMU context switch when the guest is configured without the PERF_CAP_PERF_METRICS feature. > -- > > --- Patch [5]: [PATCH v7 5/9] KVM: x86/pmu: Support PERF_METRICS MSR in mediated vPMU --- > commit 2933f2f58fa948287c388e166de23d159332fdf8 > Author: Dapeng Mi <[email protected]> > > KVM: x86/pmu: Support PERF_METRICS MSR in mediated vPMU > > This commit exposes the IA32_PERF_METRICS MSR to guests when mediated > vPMU is enabled. It handles passthrough of the MSR and saves or restores > its state during PMU context switches to prevent clobbering or leaking > state. > >> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c >> --- a/arch/x86/kvm/vmx/pmu_intel.c >> +++ b/arch/x86/kvm/vmx/pmu_intel.c > > [ ... ] > >> @@ -831,6 +852,19 @@ static void intel_mediated_pmu_put(struct kvm_vcpu *vcpu) >> */ >> if (pmu->fixed_ctr_ctrl_hw) >> wrmsrq(MSR_CORE_PERF_FIXED_CTR_CTRL, 0); >> + >> + if (kvm_vcpu_has_perf_metrics(vcpu)) { > ^^^^ > > [Severity: High] > Could this leak guest PMU state to the host or other VMs? > > In intel_mediated_pmu_put(), this checks if the guest has the > PERF_CAP_PERF_METRICS capability before clearing MSR_PERF_METRICS. > However, if the physical CPU supports this feature but the guest is > configured without it, could the guest still activate it? > > If KVM configures MSR_CORE_PERF_GLOBAL_CTRL for passthrough (which happens > when counter counts match, as kvm_need_perf_global_ctrl_intercept() only > checks counter counts), the guest can write to bit 48 of > MSR_CORE_PERF_GLOBAL_CTRL. The hardware PMU would then start updating > MSR_PERF_METRICS. Good catch. It's a basic yet important issue and something I should have thought of earlier. If the host supports PerfMetrics but it is not exposed to the guest, MSR_CORE_PERF_GLOBAL_CTRL must be intercepted so the guest cannot enable bit 48. With the following code, the guest cannot activate MSR_PERF_METRICS, and the leak concern goes away. diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 9292407f0353..474207c6413d 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -819,6 +819,7 @@ static bool kvm_need_any_pmc_intercept(struct kvm_vcpu *vcpu) bool kvm_need_perf_global_ctrl_intercept(struct kvm_vcpu *vcpu) { return kvm_need_any_pmc_intercept(vcpu) || + kvm_need_perf_metrics_intercept(vcpu) || !kvm_pmu_has_perf_global_ctrl(vcpu_to_pmu(vcpu)); } EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_need_perf_global_ctrl_intercept); > Since kvm_vcpu_has_perf_metrics() returns false in this scenario, this path > would skip clearing the physical MSR_PERF_METRICS on VM-exit, leaving guest > state in the hardware register for the next VM or host to read. > >> + pmu->perf_metrics = rdpmc(INTEL_PMC_FIXED_RDPMC_METRICS); >> + /* >> + * The SDM requires restoring fixed counter 3 before >> + * PERF_METRICS. However, this path writes 0 to PERF_METRICS >> + * before fixed counter 3. For this all-zero case, the >> + * resulting hardware state is therefore the same regardless >> + * of write order. >> + */ >> + if (pmu->perf_metrics) >> + wrmsrq(MSR_PERF_METRICS, 0); >> + } >> } >