x86/bugs: Adjust SRSO mitigation to new features

From: "Borislav Petkov (AMD)" <[email protected]>

If the machine has:

CPUID Fn8000_0021_EAX[30] (SRSO_USER_KERNEL_NO) -- If this bit is 1, it indicates the CPU is not subject to the SRSO vulnerability across user/kernel boundaries.

have it fall back to IBPB on VMEXIT only, in the case it is going to run VMs:

Speculative Return Stack Overflow: CPU user/kernel transitions protected, falling back to IBPB-on-VMEXIT Speculative Return Stack Overflow: Mitigation: IBPB on VMEXIT only

Then, upon KVM module load

and in case the machine has

CPUID Fn8000_0021_EAX[31] (SRSO_MSR_FIX). If this bit is 1, it indicates that software may use MSR BP_CFG[BpSpecReduce] to mitigate SRSO.

enable this BpSpecReduce bit to mitigate SRSO across guest/host boundaries.

Signed-off-by Borislav Petkov (AMD) <[email protected]>

---

arch/x86/kvm/cpuid.c +0 -0

 	kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO);
 
 	kvm_cpu_cap_init_kvm_defined(CPUID_8000_0022_EAX,

arch/x86/kvm/svm/svm.c +0 -0

index 9df3e1e5ae81..03f29912a638 100644

scripts/get_maintainer.pl :-)

It's not strictly KVM module load, it's when KVM enables virtualization.

E.g. if userspace clears enable_virt_at_load,

the MSR will be toggled every time the number of VMs goes from 0=>1 and 1=>0.

But why do this in KVM? E.g. why not set-and-forget in init_amd_zen4()?

Shouldn't these be two separate patches? AFAICT, while the two are related, there are no strict dependencies between SRSO_USER_KERNEL_NO and SRSO_MSR_FIX.

If the expectation is that X86_FEATURE_SRSO_USER_KERNEL_NO will only ever come from hardware, i.e. won't be force-set by the kernel, then I would prefer to set the bit in the "standard" way

arch/x86/kvm/cpuid.c +0 -0

index 41786b834b16..eb65336c2168 100644

arch/x86/kvm/cpuid.c +0 -0

        );
 
        kvm_cpu_cap_check_and_set(X86_FEATURE_SBPB);

The kvm_cpu_cap_check_and_set() trickery is necessary only for features that are force-set by the kernel, in order to avoid kvm_cpu_cap_mask()'s masking of the features by actual CPUID. I'm trying to clean things up to make that more obvious; hopefully that'll land in 6.14[*].

And advertising X86_FEATURE_SRSO_USER_KERNEL_NO should also be a separate patch, no? I.e.

1. Use SRSO_USER_KERNEL_NO in the host
2. Update KVM to advertise SRSO_USER_KERNEL_NO to userspace, i.e. let userspace know that it can be enumerate to the guest.
3. Add support for SRSO_MSR_FIX.

[*] https://lore.kernel.org/all/[email protected]

Quoted 11 lines

(quoted diff) +0 -0

 	kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO);
 
 	kvm_cpu_cap_init_kvm_defined(CPUID_8000_0022_EAX,

arch/x86/kvm/svm/svm.c +0 -0

index 9df3e1e5ae81..03f29912a638 100644
@@ -608,6 +608,9 @@ static void svm_disable_virtualization_cpu(void)
 	kvm_cpu_svm_disable();
 

arch/x86/kvm/svm/svm.c static void svm_disable_virtualization_cpu(void) · line 613

Quoted 4 lines

arch/x86/kvm/svm/svm.c +3 -0

 	amd_pmu_disable_virt();
+
+	if (cpu_feature_enabled(X86_FEATURE_SRSO_MSR_FIX))
+		msr_clear_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);

I don't like assuming the state of hardware. E.g. if MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT was already set, then KVM shouldn't clear it.

KVM's usual method of restoring host MSRs is to snapshot the MSR into "struct kvm_host_values" on module load, and then restore from there as needed. But that assumes all CPUs have the same value, which might not be the case here?

All that said, I'd still prefer that MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT is set during boot, unless there's a good reason not to do so.

Quoted 1 line · Sean Christopherson

scripts/get_maintainer.pl :-)

That's what I used but I pruned the list.

Why, did I miss anyone?

Quoted 1 line

But why do this in KVM? E.g. why not set-and-forget in init_amd_zen4()?

Because there's no need to impose an unnecessary - albeit small - perf impact on users who don't do virt.

I'm currently gravitating towards the MSR toggling thing, i.e., only when the VMs number goes 0=>1 but I'm not sure. If udev rules *always* load kvm.ko then yes, the toggling thing sounds better. I.e., set it only when really needed.

Quoted 3 lines

Shouldn't these be two separate patches? AFAICT, while the two are related, there are no strict dependencies between SRSO_USER_KERNEL_NO and SRSO_MSR_FIX.

Meh, I can split them if you really want me to.

Quoted 7 lines

Quoted 7 lines

arch/x86/kvm/svm/svm.c +0 -0

index 9df3e1e5ae81..03f29912a638 100644
@@ -608,6 +608,9 @@ static void svm_disable_virtualization_cpu(void)
 	kvm_cpu_svm_disable();
 

Quoted 7 lines

arch/x86/kvm/svm/svm.c static void svm_disable_virtualization_cpu(void) · line 613

Quoted 4 lines

arch/x86/kvm/svm/svm.c +3 -0

 	amd_pmu_disable_virt();
+
+	if (cpu_feature_enabled(X86_FEATURE_SRSO_MSR_FIX))
+		msr_clear_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);

I don't like assuming the state of hardware. E.g. if MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT was already set, then KVM shouldn't clear it.

Right, I don't see that happening tho. If we have to sync the toggling of this bit between different places, we'll have to do some dance but so far its only user is KVM.

Quoted 4 lines

KVM's usual method of restoring host MSRs is to snapshot the MSR into "struct kvm_host_values" on module load, and then restore from there as needed. But that assumes all CPUs have the same value, which might not be the case here?

Yes, the default value is 0 out of reset and it should be set on each logical CPU whenever we run VMs on it. I'd love to make it part of the VMRUN microcode but... :-)

Quoted 2 lines

All that said, I'd still prefer that MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT is set during boot, unless there's a good reason not to do so.

Yeah, unnecessary penalty on machines not running virt.

All of the actual maintainers.

AFAIK, Paolo doesn't subscribe to kvm@.

Quoted 1 line

Meh, I can split them if you really want me to.

I do.

What does the bit actually do? I can't find any useful documentation, and the changelog is equally useless.

Quoted 1 line · Sean Christopherson

All of the actual maintainers.

Which maintainers do you mean? tip ones? If so, they're all shorted to [email protected].

Quoted 1 line

AFAIK, Paolo doesn't subscribe to kvm@.

Oh boy, srsly?! I thought I'd reach the proper crowd with [email protected]...

Quoted 2 lines

What does the bit actually do? I can't find any useful documentation, and the changelog is equally useless.

"Processors which set SRSO_MSR_FIX=1 support an MSR bit which mitigates SRSO across guest/host boundaries. Software may enable this by setting bit 4 (BpSpecReduce) of MSR C001_102E. This bit can be set once during boot and should be set identically across all processors in the system."

From: https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf

I think that's the only public info we have on that bit.

It gets there, usually (as evidenced by my response). But even for me, there's a non-zero chance I'll miss something that's only Cc'd to kvm@, largely because kvm@ is used by all things virt, i.e. it's a bit noisy:

$ git grep kvm@ MAINTAINERS | wc -l
29

Heh, I found that. Not very helpful.

If you can't document the specifics, can you at least describe the performance implications? It's practically impossible to give meaningful feedback without having any idea what the magic bit does.

Quoted 6 lines · Sean Christopherson

It gets there, usually (as evidenced by my response). But even for me, there's a non-zero chance I'll miss something that's only Cc'd to kvm@, largely because kvm@ is used by all things virt, i.e. it's a bit noisy:

$ git grep kvm@ MAINTAINERS | wc -l
29

Hm, ok, so what do you guys prefer to be CCed on? Everyone from get_maintainer.pl's output? commit signers, authors, everyone? Or?

I prefer to be To:/Cc:'d on any patches that touch files that are covered by relevant MAINTAINERS entries. IMO, pulling names/emails from git is useless noise the vast majority of the time.

Quoted 3 lines · Sean Christopherson

I prefer to be To:/Cc:'d on any patches that touch files that are covered by relevant MAINTAINERS entries. IMO, pulling names/emails from git is useless noise the vast majority of the time.

Huh, that's what I did!

Please run this patch through get_maintainer.pl and tell me who else I should have CCed.

You didn't though. The original mail Cc'd kvm@, but neither Paolo nor I.

  $ ./scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats --nofixes -- <patch>
  Thomas Gleixner <[email protected]>
  Ingo Molnar <[email protected]>
  Borislav Petkov <[email protected]>
  Dave Hansen <[email protected]>
  "H. Peter Anvin" <[email protected]>
  Peter Zijlstra <[email protected]>

Josh Poimboeuf <[email protected]>
Pawan Gupta <[email protected]>

Sean Christopherson <[email protected]>
Paolo Bonzini <[email protected]>
[email protected]

[email protected]

Quoted 1 line · Sean Christopherson

You didn't though. The original mail Cc'd kvm@, but neither Paolo nor I.

I think we established that tho - I didn't know that kvm@ doesn't CC you guys.

I do subscribe to kvm@, but it's a mailing list, not at alias like x86@. AFAIK, x86@ is unique in that regard. In other words, I don't see a need to document the kvm@ behavior, because that's the behavior for every L: entry in MAINTAINERS
except the few "L: [email protected]" cases.

Quoted 4 lines · Sean Christopherson

I do subscribe to kvm@, but it's a mailing list, not at alias like x86@. AFAIK, x86@ is unique in that regard. In other words, I don't see a need to document the kvm@ behavior, because that's the behavior for every L: entry in MAINTAINERS
except the few "L: [email protected]" cases.

I have had maintainers in the past not like to be CCed directly as long as the corresponding mailing list is CCed.

LOL, doesn't that kind of defeat the purpose of MAINTAINERS?