Re: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar

Sean Christopherson
2025-05-05T16:30:07Z

On Mon, May 05, 2025, David Kaplan wrote:
> > On Thu, May 01, 2025 at 09:56:44AM -0700, Sean Christopherson wrote:
> > > Heh, I considered that, and even tried it this morning because I
> > > thought it wouldn't be as tricky as I first thought, but turns out,
> > > yeah, it's tricky. The complication is that KVM needs to ensure
> > BP_SPEC_REDUCE=1 on all CPUs before any VM is created.
> > >
> > > I thought it wouldn't be _that_ tricky once I realized the 1=>0 case
> > > doesn't require ordering, e.g. running host code while other CPUs have
> > > BP_SPEC_REDUCE=1 is totally fine, KVM just needs to ensure no guest code is
> > executed with BP_SPEC_REDUCE=0.
> > > But guarding against all the possible edge cases is comically difficult.
> > >
> > > For giggles, I did get it working, but it's a rather absurd amount of
> > > complexity
> >
> > Thanks for taking the time to explain - that's, well, funky. :-\
> >
> > Btw, in talking about this, David had this other idea which sounds
> > interesting:
> >
> > How about we do a per-CPU var which holds down whether BP_SPEC_REDUCE is
> > enabled on the CPU?
> >
> > It'll toggle the MSR bit before VMRUN on the CPU when num VMs goes 0=>1. This
> > way you avoid the IPIs and you set the bit on time.
>
> Almost. My thought was that kvm_run could do something like:
>
> If (!this_cpu_read(bp_spec_reduce_is_set)) {
> wrmsrl to set BP_SEC_REDUCE
> this_cpu_write(bp_spec_reduce_is_set, 1)
> }
>
> That ensures the bit is set for your core before VMRUN. And as noted below,
> you can clear the bit when the count drops to 0 but that one is safe from
> race conditions.

/facepalm

I keep inverting the scenario in my head. I'm so used to KVM needing to ensure
it doesn't run with guest state that I keep forgetting that running with
BP_SPEC_REDUCE=1 is fine, just a bit slower.

With that in mind, the best blend of simplicity and performance is likely to hook
svm_prepare_switch_to_guest() and svm_prepare_host_switch(). switch_to_guest()
is called when KVM is about to do VMRUN, and host_switch() is called when the
vCPU is put, i.e. when the task is scheduled out or when KVM_RUN exits to
userspace.

The existing svm->guest_state_loaded guard avoids toggling the bit when KVM
handles a VM-Exit and re-enters the guest. The kernel may run a non-trivial
amount of code with BP_SPEC_REDUCE, e.g. if #NPF triggers swap-in, an IRQ arrives
while handling the exit, etc., but that's all fine from a security perspective.

IIUC, per Boris[*] an IBPB is needed when toggling BP_SPEC_REDUCE on-demand:

: You want to IBPB before clearing the MSR as otherwise host kernel will be
: running with the mistrained gunk from the guest.

[*] https://lore.kernel.org/all/20250217160728.GFZ7NewJHpMaWdiX2M@fat_crate.local

Assuming that's the case...

Compile-tested only. If this looks/sounds sane, I'll test the mechanics and
write a changelog.

---
arch/x86/include/asm/msr-index.h | 2 +-
arch/x86/kvm/svm/svm.c | 26 +++++++++++++++++++-------
arch/x86/kvm/x86.h | 1 +
arch/x86/lib/msr.c | 2 --
4 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index e6134ef2263d..0cc9267b872e 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -725,7 +725,7 @@

/* Zen4 */
#define MSR_ZEN4_BP_CFG 0xc001102e
-#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4
+#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE BIT(4)
#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5

/* Fam 19h MSRs */
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cc1c721ba067..2d87ec216811 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -607,9 +607,6 @@ static void svm_disable_virtualization_cpu(void)
kvm_cpu_svm_disable();

amd_pmu_disable_virt();
-
- if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
- msr_clear_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
}

static int svm_enable_virtualization_cpu(void)
@@ -687,9 +684,6 @@ static int svm_enable_virtualization_cpu(void)
rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, msr_hi);
}

- if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
- msr_set_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
-
return 0;
}

@@ -1550,12 +1544,25 @@ static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
(!boot_cpu_has(X86_FEATURE_V_TSC_AUX) || !sev_es_guest(vcpu->kvm)))
kvm_set_user_return_msr(tsc_aux_uret_slot, svm->tsc_aux, -1ull);

+ if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
+ wrmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg |
+ MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE);
+
svm->guest_state_loaded = true;
}

static void svm_prepare_host_switch(struct kvm_vcpu *vcpu)
{
- to_svm(vcpu)->guest_state_loaded = false;
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ if (!svm->guest_state_loaded)
+ return;
+
+ if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) {
+ indirect_branch_prediction_barrier();
+ rdmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg);
+ }
+ svm->guest_state_loaded = false;
}

static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -5364,6 +5371,11 @@ static __init int svm_hardware_setup(void)

init_msrpm_offsets();

+ if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) {
+ rdmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg);
+ WARN_ON(kvm_host.zen4_bp_cfg & MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE);
+ }
+
kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |
XFEATURE_MASK_BNDCSR);

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 88a9475899c8..629eae9e4f59 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -51,6 +51,7 @@ struct kvm_host_values {
u64 xcr0;
u64 xss;
u64 arch_capabilities;
+ u64 zen4_bp_cfg;
};

void kvm_spurious_fault(void);
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 5a18ecc04a6c..4bf4fad5b148 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -103,7 +103,6 @@ int msr_set_bit(u32 msr, u8 bit)
{
return __flip_bit(msr, bit, true);
}
-EXPORT_SYMBOL_GPL(msr_set_bit);

/**
* msr_clear_bit - Clear @bit in a MSR @msr.
@@ -119,7 +118,6 @@ int msr_clear_bit(u32 msr, u8 bit)
{
return __flip_bit(msr, bit, false);
}
-EXPORT_SYMBOL_GPL(msr_clear_bit);

#ifdef CONFIG_TRACEPOINTS
void do_trace_write_msr(unsigned int msr, u64 val, int failed)

base-commit: 45eb29140e68ffe8e93a5471006858a018480a45
--