+Mingwei
On Thu, Aug 24, 2023, Dapeng Mi wrote:
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 7d9ba301c090..ffda2ecc3a22 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -12,7 +12,8 @@
MSR_IA32_MISC_ENABLE_BTS_UNAVAIL)
/* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
-#define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4))
& 0xf)
+#define fixed_ctrl_field(ctrl_reg, idx) \
+ (((ctrl_reg) >> ((idx) * INTEL_FIXED_BITS_STRIDE)) &
INTEL_FIXED_BITS_MASK)
#define VMWARE_BACKDOOR_PMC_HOST_TSC 0x10000
#define VMWARE_BACKDOOR_PMC_REAL_TIME 0x10001
@@ -165,7 +166,8 @@ static inline bool pmc_speculative_in_use(struct
kvm_pmc *pmc)
if (pmc_is_fixed(pmc))
return fixed_ctrl_field(pmu->fixed_ctr_ctrl,
- pmc->idx - INTEL_PMC_IDX_FIXED) & 0x3;
+ pmc->idx - INTEL_PMC_IDX_FIXED) &
+ (INTEL_FIXED_0_KERNEL | INTEL_FIXED_0_USER);
return pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE;
}
diff --git a/arch/x86/kvm/vmx/pmu_intel.c
b/arch/x86/kvm/vmx/pmu_intel.c
index f2efa0bf7ae8..b0ac55891cb7 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -548,8 +548,13 @@ static void intel_pmu_refresh(struct kvm_vcpu
*vcpu)
setup_fixed_pmc_eventsel(pmu);
}
- for (i = 0; i < pmu->nr_arch_fixed_counters; i++)
- pmu->fixed_ctr_ctrl_mask &= ~(0xbull << (i * 4));
+ for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
+ pmu->fixed_ctr_ctrl_mask &=
+ ~intel_fixed_bits_by_idx(i,
+ INTEL_FIXED_0_KERNEL |
+ INTEL_FIXED_0_USER |
+ INTEL_FIXED_0_ENABLE_PMI);
+ }
counter_mask = ~(((1ull << pmu->nr_arch_gp_counters) - 1) |
(((1ull << pmu->nr_arch_fixed_counters) - 1) <<
INTEL_PMC_IDX_FIXED));
pmu->global_ctrl_mask = counter_mask;
@@ -595,7 +600,7 @@ static void intel_pmu_refresh(struct kvm_vcpu
*vcpu)
pmu->reserved_bits &= ~ICL_EVENTSEL_ADAPTIVE;
for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
pmu->fixed_ctr_ctrl_mask &=
- ~(1ULL << (INTEL_PMC_IDX_FIXED + i * 4));
OMG, this might just win the award for most obfuscated PMU code in
KVM, which is
saying something. The fact that INTEL_PMC_IDX_FIXED happens to be
32, the same
bit number as ICL_FIXED_0_ADAPTIVE, is 100% coincidence. Good riddance.
Argh, and this goofy code helped introduce a real bug.
reprogram_fixed_counters()
doesn't account for the upper 32 bits of IA32_FIXED_CTR_CTRL.
Wait, WTF? Nothing in KVM accounts for the upper bits. This can't
possibly work.
IIUC, because KVM _always_ sets precise_ip to a non-zero bit for PEBS
events,
perf will _always_ generate an adaptive record, even if the guest
requested a
basic record. Ugh, and KVM will always generate adaptive records
even if the
guest doesn't support them. This is all completely broken. It
probably kinda
sorta works because the Basic info is always stored in the record,
and generating
more info requires a non-zero MSR_PEBS_DATA_CFG, but ugh.
Oh great, and it gets worse. intel_pmu_disable_fixed() doesn't clear
the upper
bits either, i.e. leaves ICL_FIXED_0_ADAPTIVE set. Unless I'm
misreading the code,
intel_pmu_enable_fixed() effectively doesn't clear
ICL_FIXED_0_ADAPTIVE either,
as it only modifies the bit when it wants to set ICL_FIXED_0_ADAPTIVE.
*sigh*
I'm _very_ tempted to disable KVM PEBS support for the current PMU,
and make it
available only when the so-called passthrough PMU is available[*].
Because I
don't see how this is can possibly be functionally correct, nor do I
see a way
to make it functionally correct without a rather large and invasive
series.
Ouch. And after chatting with Mingwei, who asked the very good
question of
"can this leak host state?", I am pretty sure that yes, this can leak
host state.
When PERF_CAP_PEBS_BASELINE is enabled for the guest, i.e. when the
guest has
access to adaptive records, KVM gives the guest full access to
MSR_PEBS_DATA_CFG
pmu->pebs_data_cfg_mask = ~0xff00000full;
which makes sense in a vacuum, because AFAICT the architecture
doesn't allow
exposing a subset of the four adaptive controls.
GPRs and XMMs are always context switched and thus benign, but IIUC,
Memory Info
provides data that might now otherwise be available to the guest,
e.g. if host
userspace has disallowed equivalent events via KVM_SET_PMU_EVENT_FILTER.
And unless I'm missing something, LBRs are a full leak of host
state. Nothing
in the SDM suggests that PEBS records honor MSR intercepts, so unless
KVM is
also passing through LBRs, i.e. is context switching all LBR MSRs,
the guest can
use PEBS to read host LBRs at will.
