x86/bugs: Adjust SRSO mitigation to new features
Subsystems (5)
v2 cover x86/bugs: Adjust SRSO mitigation to new features
From: "Borislav Petkov (AMD)" <[email protected]>
Hi,
here's the next revision, with hopefully all review feedback addressed.
Changelog:
v1:
https://lore.kernel.org/r/[email protected]
Thx.
Borislav Petkov (AMD) (4):
x86/bugs: Add SRSO_USER_KERNEL_NO support
KVM: x86: Advertise SRSO_USER_KERNEL_NO to userspace x86/bugs: KVM: Add support for SRSO_MSR_FIX Documentation/kernel-parameters: Fix a typo in kvm.enable_virt_at_load
text
Documentation/admin-guide/hw-vuln/srso.rst | 10 ++++++++++ Documentation/admin-guide/kernel-parameters.txt | 2 +- arch/x86/include/asm/cpufeatures.h | 2 ++ arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 16 +++++++++++++++- arch/x86/kernel/cpu/common.c | 1 + arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/svm/svm.c | 6 ++++++ arch/x86/lib/msr.c | 2 ++ 9 files changed, 39 insertions(+), 3 deletions(-)
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
Signature · 1 line
2.43.0
PATCH v2 1/4 x86/bugs: Add SRSO_USER_KERNEL_NO support
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
--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/kernel/cpu/bugs.c | 6 ++++++ arch/x86/kernel/cpu/common.c | 1 + 3 files changed, 8 insertions(+)
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -464,6 +464,7 @@#define X86_FEATURE_SBPB (20*32+27)/* Selective Branch Prediction Barrier */ #define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ +#define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/kernel/cpu/bugs.c +6 -0
@@ -2615,6 +2615,11 @@ static void __init srso_select_mitigation(void)break ;case SRSO_CMD_SAFE_RET : +if (boot_cpu_has (X86_FEATURE_SRSO_USER_KERNEL_NO )) { +pr_notice ("CPU user/kernel transitions protected, falling back to IBPB-on-VMEXIT\n" ); + goto ibpb_on_vmexit; + } +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {/* * Enable the return thunk for generated code
@@ -2658,6 +2663,7 @@ static void __init srso_select_mitigation(void) }break ; +ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT :if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {
arch/x86/kernel/cpu/common.c +1 -0
@@ -1270,6 +1270,7 @@ static const struct x86_cpu_id cpu_vuln_blacklist[] __initconst = {VULNBL_AMD (0x17 ,RETBLEED |SMT_RSB |SRSO ),VULNBL_HYGON (0x18 ,RETBLEED |SMT_RSB |SRSO ),VULNBL_AMD (0x19 ,SRSO ), + VULNBL_AMD(0x1a, SRSO), {} };
Signature · 1 line
2.43.0
Re: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2620
Quoted 8 lines · Borislav Petkov
+++
b/arch/x86/kernel/cpu/bugs.carch/x86/kernel/cpu/bugs.c +3
-0@@ -2615,6 +2615,11 @@ static void __init srso_select_mitigation(void)break ;case SRSO_CMD_SAFE_RET : +if (boot_cpu_has (X86_FEATURE_SRSO_USER_KERNEL_NO )) { +pr_notice ("CPU user/kernel transitions protected, falling back to IBPB-on-VMEXIT\n" ); + goto ibpb_on_vmexit;
The presence of SRSO_USER_KERNEL_NO should indeed change the default, but if the user requests "safe_ret" specifically, shouldn't we give it to them? That would be more consistent with how we handle requested mitigations.
Also it doesn't really make sense to add a printk here as the mitigation will be printed at the end of the function.
Signature · 1 line
Josh
Borislav Petkov
maintainerRe: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
Quoted 3 lines · Josh Poimboeuf
The presence of
SRSO_USER_KERNEL_NOshould indeed change the default, but if the user requests "safe_ret" specifically, shouldn't we give it to them?
Hardly a valid use case except for debugging but if you're doing that, you can change the kernel too.
Because we just fixed this and if some poor soul has left spec_rstack_overflow=safe-ret in her /etc/default/grub (it has happened to me a bunch on my test boxes), she'll never get her performance back and that would be a yucky situation.
Quoted 2 lines
That would be more consistent with how we handle requested mitigations.
Yeah, but if there were a point to this, I guess. We don't really need this mitigation as there's nothing to mitigate on the user/kernel transition anymore.
Quoted 2 lines
Also it doesn't really make sense to add a printk here as the mitigation will be printed at the end of the function.
This is us letting the user know that we don't need Safe-RET anymore and we're falling back. But I'm not that hung up on that printk...
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
Quoted 5 lines · Borislav Petkov
Quoted 2 lines
Also it doesn't really make sense to add a printk here as the mitigation will be printed at the end of the function.
This is us letting the user know that we don't need Safe-RET anymore and we're falling back. But I'm not that hung up on that printk...
The printk makes sense when it's actually a fallback from "spec_rstack_overflow=safe-ret", but if nothing was specified on the cmdline, it's the default rather than a fallback. In which case I think the printk would be confusing.
Signature · 1 line
Josh
Borislav Petkov
maintainerRe: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
Quoted 2 lines · Josh Poimboeuf
The printk makes sense when it's actually a fallback from "spec_rstack_overflow=safe-ret"
Well, it kinda is as safe-ret is the default and we're falling back from it...
Quoted 2 lines
but if nothing was specified on the cmdline, it's the default rather than a fallback. In which case I think the printk would be confusing.
... but as I said, I'm not hung on that printk - zapped it is.
Btw, Sean, how should we merge this?
Should I take it all through tip and give you an immutable branch?
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
Quoted 3 lines · Borislav Petkov
Btw, Sean, how should we merge this?
Should I take it all through tip and give you an immutable branch?
Hmm, that should work. I don't anticipate any conflicts other than patch 2 (Advertise SRSO_USER_KERNEL_NO to userspace), which is amusingly the most trivial patch.
Patch 2 is going to conflict with the CPUID/cpu_caps rework[*], but the conflict won't be hard to resolve, and I'm pretty sure that if I merge in your branch after applying the rework, the merge commit will show an "obviously correct" resolution. Or if I screw it up, an obviously wrong resolution :-)
Alternatively, take 1, 3, and 4 through tip, and 2 through my tree, but that seems unnecessarily convoluted.
Borislav Petkov
maintainerRe: [PATCH v2 1/4] x86/bugs: Add SRSO_USER_KERNEL_NO support
Quoted 9 lines · Sean Christopherson
Quoted 3 lines · Borislav Petkov
Btw, Sean, how should we merge this?
Should I take it all through tip and give you an immutable branch?
Hmm, that should work. I don't anticipate any conflicts other than patch 2 (Advertise
SRSO_USER_KERNEL_NOto userspace), which is amusingly the most trivial patch.
Quoted 7 lines
Patch 2 is going to conflict with the CPUID/cpu_caps rework[*], but the conflict won't be hard to resolve, and I'm pretty sure that if I merge in your branch after applying the rework, the merge commit will show an "obviously correct" resolution. Or if I screw it up, an obviously wrong resolution :-)
Alternatively, take 1, 3, and 4 through tip, and 2 through my tree, but that seems unnecessarily convoluted.
Ok, lemme queue them all through tip and we'll see what conflicts we encounter along the way and then sync again.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette[tip: x86/bugs] x86/bugs: Add SRSO_USER_KERNEL_NO support
In reply to Borislav PetkovThe following commit has been merged into the x86/bugs branch of tip:
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: 877818802c3e970f67ccb53012facc78bef5f97a Gitweb: https://git.kernel.org/tip/877818802c3e970f67ccb53012facc78bef5f97a Author: Borislav Petkov (AMD) <[email protected]> AuthorDate: Mon, 11 Nov 2024 17:22:08 +01:00 Committer: Borislav Petkov (AMD) <[email protected]> CommitterDate: Mon, 30 Dec 2024 17:48:33 +01:00
x86/bugs: Add SRSO_USER_KERNEL_NO support
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: Mitigation: IBPB on VMEXIT only
--- arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/kernel/cpu/bugs.c | 4 ++++ arch/x86/kernel/cpu/common.c | 1 + 3 files changed, 6 insertions(+)
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -465,6 +465,7 @@#define X86_FEATURE_SBPB (20*32+27)/* Selective Branch Prediction Barrier */ #define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ +#define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/kernel/cpu/bugs.c +4 -0
@@ -2615,6 +2615,9 @@ static void __init srso_select_mitigation(void)break ;case SRSO_CMD_SAFE_RET : +if (boot_cpu_has (X86_FEATURE_SRSO_USER_KERNEL_NO )) + goto ibpb_on_vmexit; +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {/* * Enable the return thunk for generated code
@@ -2658,6 +2661,7 @@ static void __init srso_select_mitigation(void) }break ; +ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT :if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {
arch/x86/kernel/cpu/common.c +1 -0
@@ -1270,6 +1270,7 @@ static const struct x86_cpu_id cpu_vuln_blacklist[] __initconst = {VULNBL_AMD (0x17 ,RETBLEED |SMT_RSB |SRSO ),VULNBL_HYGON (0x18 ,RETBLEED |SMT_RSB |SRSO ),VULNBL_AMD (0x19 ,SRSO ), + VULNBL_AMD(0x1a, SRSO), {} };
PATCH v2 2/4 KVM: x86: Advertise SRSO_USER_KERNEL_NO to userspace
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
SRSO_USER_KERNEL_NO denotes whether the CPU is affected by SRSO across user/kernel boundaries. Advertise it to guest userspace.
--- arch/x86/kvm/cpuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +1 -1
@@ -800,7 +800,7 @@ void kvm_set_cpu_caps(void)kvm_cpu_cap_mask (CPUID_8000_0021_EAX ,F (NO_NESTED_DATA_BP ) |F (LFENCE_RDTSC ) |0 /* SmmPgCfgLock */ |F (NULL_SEL_CLR_BASE ) |F (AUTOIBRS ) |0 /* PrefetchCtlMsr */ | -F (WRMSR_XX_BASE_NS ) +F (WRMSR_XX_BASE_NS ) |F (SRSO_USER_KERNEL_NO ) );kvm_cpu_cap_check_and_set (X86_FEATURE_SBPB );
Signature · 1 line
2.43.0
[tip: x86/bugs] KVM: x86: Advertise SRSO_USER_KERNEL_NO to userspace
The following commit has been merged into the x86/bugs branch of tip:
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: 716f86b523d8ec3c17015ee0b03135c7aa6f2f08 Gitweb: https://git.kernel.org/tip/716f86b523d8ec3c17015ee0b03135c7aa6f2f08 Author: Borislav Petkov (AMD) <[email protected]> AuthorDate: Wed, 13 Nov 2024 13:28:33 +01:00 Committer: Borislav Petkov (AMD) <[email protected]> CommitterDate: Mon, 30 Dec 2024 17:56:00 +01:00
KVM: x86: Advertise SRSO_USER_KERNEL_NO to userspace
SRSO_USER_KERNEL_NO denotes whether the CPU is affected by SRSO across user/kernel boundaries. Advertise it to guest userspace.
--- arch/x86/kvm/cpuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/cpuid.c +1 -1
@@ -821,7 +821,7 @@ void kvm_set_cpu_caps(void)kvm_cpu_cap_mask (CPUID_8000_0021_EAX ,F (NO_NESTED_DATA_BP ) |F (LFENCE_RDTSC ) |0 /* SmmPgCfgLock */ |F (NULL_SEL_CLR_BASE ) |F (AUTOIBRS ) |0 /* PrefetchCtlMsr */ | -F (WRMSR_XX_BASE_NS ) +F (WRMSR_XX_BASE_NS ) |F (SRSO_USER_KERNEL_NO ) );kvm_cpu_cap_check_and_set (X86_FEATURE_SBPB );
PATCH v2 3/4 x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
Add support for
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.
---
v2: Add some doc blurb about the modalities of the mitigation.
Documentation/admin-guide/hw-vuln/srso.rst | 10 ++++++++++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 10 +++++++++- arch/x86/kvm/svm/svm.c | 6 ++++++ arch/x86/lib/msr.c | 2 ++ 6 files changed, 29 insertions(+), 1 deletion(-)
Documentation/admin-guide/hw-vuln/srso.rst +10 -0
@@ -104,7 +104,17 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + Currently, the mitigation is automatically enabled when KVM enables + virtualization and can incur some cost. If no VMs will run on the system, + you can either disable virtualization or set kvm.enable_virt_at_load=0 to + enable it only when a VM gets started and thus when really needed. See the + text in Documentation/admin-guide/kernel-parameters.txt on this parameter + for more details. In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -465,6 +465,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_MSR_FIX (20*32+31)/* MSR BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -717,6 +717,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +9 -1
@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2540,7 +2541,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2665,6 +2667,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_MSR_FIX )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
arch/x86/kvm/svm/svm.c +6 -0
@@ -608,6 +608,9 @@ static void svm_disable_virtualization_cpu(void)kvm_cpu_svm_disable ();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 ); }static int svm_enable_virtualization_cpu (void )
@@ -685,6 +688,9 @@ 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_MSR_FIX )) +msr_set_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); +return 0 ; }
arch/x86/lib/msr.c +2 -0
@@ -103,6 +103,7 @@ 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.
@@ -118,6 +119,7 @@ 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)
Signature · 1 line
2.43.0
Sean Christopherson
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 5 lines · Borislav Petkov
Documentation/admin-guide/hw-vuln/srso.rst +0
-0
Documentation/admin-guide/hw-vuln/srso.rst The possible values in this file are: · line 113
Quoted 10 lines
Documentation/admin-guide/hw-vuln/srso.rst +6
-0@@ -104,7 +104,17 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + Currently, the mitigation is automatically enabled when KVM enables + virtualization and can incur some cost.
How much cost are we talking?
Quoted 29 lines
Documentation/admin-guide/hw-vuln/srso.rst +6
-0static enum srso_mitigation srso_mitigation __ro_after_init = SRSO_MITIGATION_NONE;@@ -2665,6 +2667,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit: case SRSO_CMD_IBPB_ON_VMEXIT: + if (boot_cpu_has(X86_FEATURE_SRSO_MSR_FIX)) { + pr_notice("Reducing speculation to address VM/HV SRSO attack vector.\n"); + srso_mitigation = SRSO_MITIGATION_BP_SPEC_REDUCE; + break; + } + if (IS_ENABLED(CONFIG_MITIGATION_SRSO)) { if (!boot_cpu_has(X86_FEATURE_ENTRY_IBPB) && has_microcode) { setup_force_cpu_cap(X86_FEATURE_IBPB_ON_VMEXIT);arch/x86/kvm/svm/svm.c +3
-0@@ -608,6 +608,9 @@ static void svm_disable_virtualization_cpu(void)kvm_cpu_svm_disable ();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 ); }static int svm_enable_virtualization_cpu (void )
arch/x86/kvm/svm/svm.c static int svm_enable_virtualization_cpu(void) · line 692
Quoted 5 lines
arch/x86/kvm/svm/svm.c +2
-0@@ -685,6 +688,9 @@ 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_MSR_FIX )) +msr_set_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT );
IIUC, this magic bit reduces how much the CPU is allowed to speculate in order to mitigate potential VM=>host attacks, and that reducing speculation also reduces overall performance.
If that's correct, then enabling the magic bit needs to be gated by an appropriate mitagation being enabled, not forced on automatically just because the CPU supports X86_FEATURE_SRSO_MSR_FIX.
And depending on the cost, it might also make sense to set the bit on-demand, and then clean up when KVM disables virtualization. E.g. wait to set the bit until entry to a guest is imminent.
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 1 line · Sean Christopherson
How much cost are we talking?
Likely 1-2%.
That's why I'm simply enabling it by default.
Quoted 7 lines
IIUC, this magic bit reduces how much the CPU is allowed to speculate in order to mitigate potential VM=>host attacks, and that reducing speculation also reduces overall performance.
If that's correct, then enabling the magic bit needs to be gated by an appropriate mitagation being enabled, not forced on automatically just because the CPU supports
X86_FEATURE_SRSO_MSR_FIX.
Well, in the default case we have safe-RET - the default - but since it is not needed anymore, it falls back to this thing which is needed when the mitigation is enabled.
That's why it also is in the SRSO_CMD_IBPB_ON_VMEXIT case as it is part of the spec_rstack_overflow=ibpb-vmexit mitigation option.
So it kinda already does that. When you disable the mitigation, this one won't get enabled either.
Quoted 3 lines
And depending on the cost, it might also make sense to set the bit on-demand, and then clean up when KVM disables virtualization. E.g. wait to set the bit until entry to a guest is imminent.
So the "when to set that bit" discussion kinda remained unfinished the last time. Here's gist:
You:
| "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()?"
I:
| "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."
So to answer your current question, it sounds like the user can control the on-demand thing with enable_virt_at_load=0, right?
Or do you mean something else different?
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 19 lines · Borislav Petkov
Quoted 1 line · Sean Christopherson
How much cost are we talking?
Likely 1-2%.
That's why I'm simply enabling it by default.
Quoted 7 lines
IIUC, this magic bit reduces how much the CPU is allowed to speculate in order to mitigate potential VM=>host attacks, and that reducing speculation also reduces overall performance.
If that's correct, then enabling the magic bit needs to be gated by an appropriate mitagation being enabled, not forced on automatically just because the CPU supports
X86_FEATURE_SRSO_MSR_FIX.Well, in the default case we have safe-RET - the default - but since it is not needed anymore, it falls back to this thing which is needed when the mitigation is enabled.
Quoted 5 lines
That's why it also is in the
SRSO_CMD_IBPB_ON_VMEXITcase as it is part of the spec_rstack_overflow=ibpb-vmexit mitigation option.So it kinda already does that. When you disable the mitigation, this one won't get enabled either.
But it's a hardware-defined feature flag, so won't it be set by this code?
if (c->extended_cpuid_level >= 0x8000001f) c->x86_capability[CPUID_8000_001F_EAX] = cpuid_eax(0x8000001f);
srso_select_mitigation() checks the flag for SRSO_CMD_IBPB_ON_VMEXIT
case SRSO_CMD_IBPB_ON_VMEXIT:
if (boot_cpu_has(X86_FEATURE_SRSO_MSR_FIX)) {
pr_notice("Reducing speculation to address VM/HV SRSO attack vector.\n");
srso_mitigation = SRSO_MITIGATION_BP_SPEC_REDUCE;
break;
}but I don't see any code that would clear X86_FEATURE_SRSO_MSR_FIX. Am I missing something?
Quoted 6 lines
Quoted 3 lines
And depending on the cost, it might also make sense to set the bit on-demand, and then clean up when KVM disables virtualization. E.g. wait to set the bit until entry to a guest is imminent.
So the "when to set that bit" discussion kinda remained unfinished the last time.
Gah, sorry. I suspect I got thinking about how best to "set it only when really needed", and got lost in analysis paralysis.
Quoted 19 lines
Here's gist:
You:
| "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()?"I:
| "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 2 lines
So to answer your current question, it sounds like the user can control the on-demand thing with enable_virt_at_load=0, right?
To some extent. But I strongly suspect that the vast, vast majority of end users will end up with systems that automatically load kvm.ko, but don't run VMs the majority of the time. Expecting non-KVM to users to detect a 1-2% regression and track down enable_virt_at_load doesn't seem like a winning strategy.
Quoted 1 line
Or do you mean something else different?
The other possibility would be to wait to set the bit until a CPU is actually going to do VMRUN. If we use KVM's "user-return MSR" framework, the bit would be cleared when the CPU returns to userspace. The only downside to that is KVM would toggle the bit on CPUs running vCPUs on every exit to userspace, e.g. to emulate MMIO/IO and other things.
But, userspace exits are relatively slow paths, so if the below is a wash for performance when running VMs, i.e. the cost of the WRMSRs is either in the noise or is offset by the regained 1-2% performance for userspace, then I think it's a no-brainer.
Enabling "full" speculation on return to usersepace means non-KVM tasks won't be affected, and there's no "sticky" behavior. E.g. another idea would be to defer setting the bit until VMRUN is imminent, but then wait to clear the bit until virtualization is disabled. But that has the downside of the bit being set on all CPUs over time, especially if enable_virt_at_load is true.
Compile tested only...
arch/x86/kvm/svm/svm.c +14 -6
@@ -256,6 +256,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -608,9 +609,6 @@ static void svm_disable_virtualization_cpu(void)kvm_cpu_svm_disable ();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 ); }static int svm_enable_virtualization_cpu (void )
@@ -688,9 +686,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_MSR_FIX )) -msr_set_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); -return 0 ; }
@@ -1547,6 +1542,11 @@ 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_MSR_FIX )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT , +MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); + svm->guest_state_loaded = true; }
@@ -5313,6 +5313,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_MSR_FIX )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot) <0 ) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 2 lines · Sean Christopherson
but I don't see any code that would clear
X86_FEATURE_SRSO_MSR_FIX. Am I missing something?
Ah, you want the toggles in svm_{enable,disable}_virtualization_cpu() to not happen when the mitigation is disabled. Yeah, I guess we should clear the flag when the mitigation is disabled...
Quoted 2 lines
Gah, sorry. I suspect I got thinking about how best to "set it only when really needed", and got lost in analysis paralysis.
I know *exactly* what you're talking about :-P
Quoted 4 lines
To some extent. But I strongly suspect that the vast, vast majority of end users will end up with systems that automatically load kvm.ko, but don't run VMs the majority of the time. Expecting non-KVM to users to detect a 1-2% regression and track down
enable_virt_at_loaddoesn't seem like a winning strategy.
Yap, that was my fear too. Frankly, I don't have a really good answer to that yet.
Quoted 11 lines
The other possibility would be to wait to set the bit until a CPU is actually going to do VMRUN. If we use KVM's "user-return MSR" framework, the bit would be cleared when the CPU returns to userspace. The only downside to that is KVM would toggle the bit on CPUs running vCPUs on every exit to userspace, e.g. to emulate MMIO/IO and other things.
But, userspace exits are relatively slow paths, so if the below is a wash for performance when running VMs, i.e. the cost of the WRMSRs is either in the noise or is offset by the regained 1-2% performance for userspace, then I think it's a no-brainer.
Quoted 5 lines
Enabling "full" speculation on return to usersepace means non-KVM tasks won't be affected, and there's no "sticky" behavior. E.g. another idea would be to defer setting the bit until VMRUN is imminent, but then wait to clear the bit until virtualization is disabled. But that has the downside of the bit being set on all CPUs over time, especially if
enable_virt_at_loadis true.
Yeah, I think we should keep it simple initially and only do anything more involved when it turns out that we really need it.
Quoted 1 line
Compile tested only...
Thanks, looks good, I'll run it after I get back from vacation to make sure we're good and then we'll talk again. :)
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Sean Christophersonarch/x86/kvm/svm/svm.c static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu) · line 1548
Quoted 8 lines · Sean Christopherson
arch/x86/kvm/svm/svm.c +4
-0@@ -1547,6 +1542,11 @@ 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_MSR_FIX )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT , +MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT );
I think this needs to be:
if (cpu_feature_enabled(X86_FEATURE_SRSO_MSR_FIX))
kvm_set_user_return_msr(zen4_bp_cfg_uret_slot,
BIT_ULL(MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT),
BIT_ULL(MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT));I.e., value and mask are both the 4th bit: (1 << 4)
arch/x86/kvm/svm/svm.c static __init int svm_hardware_setup(void) · line 5321
Quoted 13 lines
arch/x86/kvm/svm/svm.c +6
-0svm->guest_state_loaded = true; }@@ -5313,6 +5313,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_MSR_FIX )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot) <0 ) { + r = -EIO ; + goto err; + }
And this needs to be:
if (cpu_feature_enabled(X86_FEATURE_SRSO_MSR_FIX)) {
zen4_bp_cfg_uret_slot = kvm_add_user_return_msr(MSR_ZEN4_BP_CFG);
if (WARN_ON_ONCE(zen4_bp_cfg_uret_slot < 0)) {
r = -EIO;
goto err;
}
}Note the WARN_ON_ONCE bracketing. But I know you're doing this on purpose - to see if I'm paying attention and not taking your patch blindly :-P
With that fixed, this approach still doesn't look sane to me: before I start the guest I have all SPEC_REDUCE bits correctly clear:
# rdmsr -a 0xc001102e | uniq -c
128 420000... start a guest, shut it down cleanly, qemu exits properly...
# rdmsr -a 0xc001102e | uniq -c
1 420010
6 420000
1 420010
3 420000
1 420010
3 420000
1 420010
1 420000
1 420010
6 420000
1 420010
1 420000
1 420010
6 420000
1 420010
5 420000
1 420010
18 420000
1 420010
5 420000
1 420010
6 420000
1 420010
3 420000
1 420010
3 420000
1 420010
1 420000
1 420010
6 420000
1 420010
1 420000
1 420010
6 420000
1 420010
5 420000
1 420010
18 420000
1 420010
5 420000so SPEC_REDUCE remains set on some cores. Not good since I'm not running VMs anymore.
# rmmod kvm_amd kvm
# rdmsr -a 0xc001102e | uniq -c
128 420000that looks more like it.
Also, this user-return MSR toggling does show up higher in the profile:
4.31% qemu-system-x86 [kvm] [k] 0x000000000000d23f 2.44% qemu-system-x86 [kernel.kallsyms] [k] read_tsc 1.66% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 1.50% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safe
vs
1.01% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 0.81% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safe
so it really is noticeable.
So I wanna say, let's do the below and be done with it. My expectation is that this won't be needed in the future anymore either so it'll be a noop on most machines...
---
@@ -609,6 +609,9 @@ static void svm_disable_virtualization_cpu(void)
kvm_cpu_svm_disable();
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);
}
static int svm_enable_virtualization_cpu(void)
@@ -686,6 +689,9 @@ 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_MSR_FIX))
+ msr_set_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
+
return 0;
}
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 3 lines · Borislav Petkov
On Mon, Dec 16, 2024 at 10:51:13AM -0800, Sean Christopherson wrote:
Note theWARN_ON_ONCEbracketing. But I know you're doing this on purpose - to see if I'm paying attention and not taking your patch blindly :-P
LOL, yeah, totally on purpose.
Quoted 9 lines
With that fixed, this approach still doesn't look sane to me: before I start the guest I have all
SPEC_REDUCEbits correctly clear:# rdmsr -a 0xc001102e | uniq -c 128 420000... start a guest, shut it down cleanly, qemu exits properly...
# rdmsr -a 0xc001102e | uniq -c
...
Quoted 8 lines
so
SPEC_REDUCEremains set on some cores. Not good since I'm not running VMs anymore.# rmmod kvm_amd kvm # rdmsr -a 0xc001102e | uniq -c 128 420000that looks more like it.
The "host" value will only be restored when the CPU exits to userspace, so if there are no userspace tasks running on those CPUs, i.e. nothing that forces them back to userspace, then it's expected for them to have the "guest" value loaded, even after the guest is long gone. Unloading KVM effectively forces KVM to simulate a return to userspace and thus restore the host values.
It seems unlikely that someone would care deeply about the performance of a CPU that is only running kernel code, but I agree it's odd and not exactly desirable.
Quoted 7 lines
Also, this user-return MSR toggling does show up higher in the profile:
4.31% qemu-system-x86 [kvm] [k] 0x000000000000d23f 2.44% qemu-system-x86 [kernel.kallsyms] [k] read_tsc 1.66% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 1.50% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safe
Quoted 6 lines
vs
1.01% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 0.81% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safeso it really is noticeable.
Hmm, mostly out of curiosity, what's the "workload"? And do you know what 0xd23f corresponds to?
For most setups, exits all the way to userspace are relatively uncommon. There are scenarios where the number of userspace exits is quite high, e.g. if the guest is spamming its emulated serial console, but I wouldn't expect switching the MSR on user entry/exit to be that noticeable.
Quoted 3 lines
So I wanna say, let's do the below and be done with it. My expectation is that this won't be needed in the future anymore either so it'll be a noop on most machines...
Yeah, especially if this is all an improvement over the existing mitigation. Though since it can impact non-virtualization workloads, maybe it should be a separately selectable mitigation? I.e. not piggybacked on top of ibpb-vmexit?
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 5 lines · Sean Christopherson
The "host" value will only be restored when the CPU exits to userspace, so if there are no userspace tasks running on those CPUs, i.e. nothing that forces them back to userspace, then it's expected for them to have the "guest" value loaded, even after the guest is long gone. Unloading KVM effectively forces KVM to simulate a return to userspace and thus restore the host values.
Aha, makes sense.
Quoted 1 line
Hmm, mostly out of curiosity, what's the "workload"?
Oh, very very exciting: booting a guest! :-P
Quoted 1 line
And do you know what
0xd23fcorresponds to?
How's that:
$ objdump -D arch/x86/kvm/kvm.ko
...
000000000000d1a0 <kvm_vcpu_halt>:
d1a0: e8 00 00 00 00 call d1a5 <kvm_vcpu_halt+0x5>
d1a5: 55 push %rbp
... d232: e8 09 93 ff ff call 6540 <kvm_vcpu_check_block>
d237: 85 c0 test %eax,%eax
d239: 0f 88 f6 01 00 00 js d435 <kvm_vcpu_halt+0x295>
d23f: f3 90 pause
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
d241: e8 00 00 00 00 call d246 <kvm_vcpu_halt+0xa6>
d246: 48 89 c3 mov %rax,%rbx
d249: e8 00 00 00 00 call d24e <kvm_vcpu_halt+0xae>
d24e: 84 c0 test %al,%alWhich makes sense :-)
Quoted 3 lines
Yeah, especially if this is all an improvement over the existing mitigation. Though since it can impact non-virtualization workloads, maybe it should be a separately selectable mitigation? I.e. not piggybacked on top of ibpb-vmexit?
Well, ibpb-on-vmexit is your typical cloud provider scenario where you address the VM/VM attack vector by doing an IBPB on VMEXIT. This SRSO_MSR_FIX thing protects the *host* from a malicious guest so you need both enabled for full protection on the guest/host vector.
(And yeah, I'm talking about attack vectors because this way of thinking about
the mitigations will simplify stuff a lot:
https://lore.kernel.org/r/[email protected]
)
Ok, lemme send a proper patch...
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 23 lines · Borislav Petkov
Quoted 1 line
And do you know what
0xd23fcorresponds to?How's that:
$ objdump -D arch/x86/kvm/kvm.ko ... 000000000000d1a0 <kvm_vcpu_halt>: d1a0: e8 00 00 00 00 call d1a5 <kvm_vcpu_halt+0x5> d1a5: 55 push %rbp ...d232: e8 09 93 ff ff call 6540 <kvm_vcpu_check_block> d237: 85 c0 test %eax,%eax d239: 0f 88 f6 01 00 00 js d435 <kvm_vcpu_halt+0x295> d23f: f3 90 pause ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ d241: e8 00 00 00 00 call d246 <kvm_vcpu_halt+0xa6> d246: 48 89 c3 mov %rax,%rbx d249: e8 00 00 00 00 call d24e <kvm_vcpu_halt+0xae> d24e: 84 c0 test %al,%al
Quoted 1 line
Which makes sense :-)
Ooh, it's just the MSR writes that increased. I misinterpreted the profile statement and thought that something in KVM was jumping from ~0% to 4.31%. If the cost really is just this:
1.66% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 1.50% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safe
vs
1.01% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr 0.81% qemu-system-x86 [kernel.kallsyms] [k] native_write_msr_safe
then my vote is to go with the user_return approach. It's unfortunate that restoring full speculation may be delayed until a CPU exits to userspace or KVM is unloaded, but given that enable_virt_at_load is enabled by default, in practice it's likely still far better than effectively always running the host with reduced speculation.
Quoted 6 lines
Quoted 3 lines
Yeah, especially if this is all an improvement over the existing mitigation. Though since it can impact non-virtualization workloads, maybe it should be a separately selectable mitigation? I.e. not piggybacked on top of ibpb-vmexit?
Well, ibpb-on-vmexit is your typical cloud provider scenario where you address the VM/VM attack vector by doing an IBPB on VMEXIT.
No? svm_vcpu_load() emits IBPB when switching VMCBs, i.e. when switching between vCPUs that may live in separate security contexts. That IBPB is skipped when X86_FEATURE_IBPB_ON_VMEXIT is enabled, because the host is trusted to not attack its guests.
Quoted 2 lines
This
SRSO_MSR_FIXthing protects the *host* from a malicious guest so you need both enabled for full protection on the guest/host vector.
If reducing speculation protects the host, why wouldn't that also protect other guests? The CPU needs to bounce through the host before enterring a different guest.
And if for some reason reducing speculation doesn't suffice, wouldn't it be better to fall back to doing IBPB only when switching VMCBs?
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 5 lines · Sean Christopherson
then my vote is to go with the
user_returnapproach. It's unfortunate that restoring full speculation may be delayed until a CPU exits to userspace or KVM is unloaded, but given thatenable_virt_at_loadis enabled by default, in practice it's likely still far better than effectively always running the host with reduced speculation.
I guess. Kaplan just said something to that effect so I guess we can start with that and then see who complains and address it if she cries loud enough. :-P
Quoted 1 line
No?
svm_vcpu_load()emits IBPB when switching VMCBs, i.e. when switching between
Bah, nevermind. I got confused by our own whitepaper. /facepalm.
So here's the deal:
The machine has SRSO_USER_KERNEL_NO=1. Which means, you don't need safe-RET. So we fallback to ibpb-on-vmexit.
Now, if the machine sports BpSpecReduce, we do that and that covers all the vectors. Otherwise, IBPB-on-VMEXIT it is.
The VM/VM attack vector the paper is talking about and having to IBPB is for the Spectre v2 side of things. Not SRSO.
Yeah, lemme document that while it is fresh in my head. This is exactly why I wanted Josh to start mitigation documentation - exactly for such nasties.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 12 lines · Borislav Petkov
Quoted 5 lines · Sean Christopherson
then my vote is to go with the
user_returnapproach. It's unfortunate that restoring full speculation may be delayed until a CPU exits to userspace or KVM is unloaded, but given thatenable_virt_at_loadis enabled by default, in practice it's likely still far better than effectively always running the host with reduced speculation.I guess. Kaplan just said something to that effect so I guess we can start with that and then see who complains and address it if she cries loud enough. :-P
Quoted 8 lines
Quoted 1 line
No?
svm_vcpu_load()emits IBPB when switching VMCBs, i.e. when switching betweenBah, nevermind. I got confused by our own whitepaper. /facepalm.
So here's the deal:
The machine has SRSO_USER_KERNEL_NO=1. Which means, you don't need safe-RET. So we fallback to ibpb-on-vmexit.
Surely, IBPB-on-VMexit is worse for performance than safe-RET?!?
(But, maybe I have a warped perspective, since I only care about VM performance).
Quoted 17 lines
Now, if the machine sports BpSpecReduce, we do that and that covers all the vectors. Otherwise, IBPB-on-VMEXIT it is.
The VM/VM attack vector the paper is talking about and having to IBPB is for the Spectre v2 side of things. Not SRSO.
Yeah, lemme document that while it is fresh in my head. This is exactly why I wanted Josh to start mitigation documentation - exactly for such nasties.
Thx.
--
Regards/Gruss,
Boris.
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 1 line · Jim Mattson
Surely, IBPB-on-VMexit is worse for performance than safe-RET?!?
We don't need safe-RET with SRSO_USER_KERNEL_NO=1. And there's no safe-RET for virt only. So IBPB-on-VMEXIT is the next best thing. The good thing is, those machines have BpSpecReduce too so you won't be doing IBPB-on-VMEXIT either but what we're talking about here - BpSpecReduce.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 8 lines · Borislav Petkov
Quoted 1 line · Jim Mattson
Surely, IBPB-on-VMexit is worse for performance than safe-RET?!?
We don't need safe-RET with SRSO_USER_KERNEL_NO=1. And there's no safe-RET for virt only. So IBPB-on-VMEXIT is the next best thing. The good thing is, those machines have BpSpecReduce too so you won't be doing IBPB-on-VMEXIT either but what we're talking about here - BpSpecReduce.
I'm suggesting that IBPB-on-VMexit is probably the *worst* thing. If it weren't for BpSpecReduce, I would want safe-RET for virt only. (Well, if it weren't for ASI, I would want that, anyway.)
Quoted 5 lines
--
Regards/Gruss,
Boris.
Borislav Petkov
maintainerRe: [PATCH v2 3/4] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 6 lines · Jim Mattson
Quoted 4 lines
We don't need safe-RET with SRSO_USER_KERNEL_NO=1. And there's no safe-RET for virt only. So IBPB-on-VMEXIT is the next best thing. The good thing is, those machines have BpSpecReduce too so you won't be doing IBPB-on-VMEXIT either but what we're talking about here - BpSpecReduce.
I'm suggesting that IBPB-on-VMexit is probably the *worst* thing.
I know what you're suggesting and I don't think so but I'd need to look at the numbers.
Quoted 1 line
If it weren't for BpSpecReduce, I would want safe-RET for virt only.
Read my reply again.
Quoted 1 line
(Well, if it weren't for ASI, I would want that, anyway.)
I'm sure Brendan is waiting for reviews there.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerPATCH 1/1 x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Borislav PetkovOk,
here's a new version, I think I've addressed all outstanding review comments.
Lemme know how we should proceed here, you take it or I? Judging by the diffstat probably I should and you ack it or so.
Also, lemme know if I should add your Co-developed-by for the user_return portion.
Thx.
---
From: "Borislav Petkov (AMD)" <[email protected]>
Add support for
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.
--- Documentation/admin-guide/hw-vuln/srso.rst | 21 +++++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 1 + arch/x86/include/asm/processor.h | 1 + arch/x86/kernel/cpu/bugs.c | 16 +++++++++++++++- arch/x86/kvm/svm/svm.c | 14 ++++++++++++++ arch/x86/lib/msr.c | 2 ++ 7 files changed, 55 insertions(+), 1 deletion(-)
Documentation/admin-guide/hw-vuln/srso.rst +21 -0
@@ -104,6 +104,27 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. + In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -468,6 +468,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_MSR_FIX (20*32+31)/* MSR BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -719,6 +719,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/include/asm/processor.h +1 -0
@@ -762,6 +762,7 @@ enum mds_mitigations { };extern bool gds_ucode_mitigated (void ); +extern bool srso_spec_reduce_enabled (void );/* * Make previous memory operations globally visible before
arch/x86/kernel/cpu/bugs.c +15 -1
@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2540,12 +2541,19 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;static enum srso_mitigation_cmd srso_cmd__ro_after_init =SRSO_CMD_SAFE_RET ; +bool srso_spec_reduce_enabled (void ) +{ +return srso_mitigation ==SRSO_MITIGATION_BP_SPEC_REDUCE ; +} +EXPORT_SYMBOL_GPL (srso_spec_reduce_enabled); +static int __init srso_parse_cmdline (char *str) {if (!str)
@@ -2663,6 +2671,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_MSR_FIX )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
arch/x86/kvm/svm/svm.c +14 -0
@@ -256,6 +256,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1541,6 +1542,11 @@ 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 (srso_spec_reduce_enabled ()) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5298,6 +5304,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (srso_spec_reduce_enabled ()) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
arch/x86/lib/msr.c +2 -0
@@ -103,6 +103,7 @@ 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.
@@ -118,6 +119,7 @@ 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)
Signature · 8 lines
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 6 lines · Borislav Petkov
Ok,
here's a new version, I think I've addressed all outstanding review comments.
Lemme know how we should proceed here, you take it or I? Judging by the diffstat probably I should and you ack it or so.
No preference, either way works for me.
Quoted 2 lines
Also, lemme know if I should add your Co-developed-by for the
user_returnportion.
Heh, no preference again. In case you want to add Co-developed-by, here's my SoB:
arch/x86/include/asm/cpufeatures.h line 471
Quoted 10 lines
In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1
-0@@ -468,6 +468,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_MSR_FIX (20*32+31)/* MSR BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs */
Any objection to calling this X86_FEATURE_SRSO_BP_SPEC_REDUCE? More below.
Quoted 5 lines
arch/x86/include/asm/cpufeatures.h +0
-1@@ -2540,12 +2541,19 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT ] ="Mitigation: IBPB on VMEXIT only"
arch/x86/include/asm/cpufeatures.h static const char * const srso_strings[] = { · line 2553
Quoted 10 lines
arch/x86/include/asm/cpufeatures.h +5
-0+ [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;static enum srso_mitigation_cmd srso_cmd__ro_after_init =SRSO_CMD_SAFE_RET ; +bool srso_spec_reduce_enabled (void ) +{ +return srso_mitigation ==SRSO_MITIGATION_BP_SPEC_REDUCE ;
At the risk of getting too clever, what if we use the X86_FEATURE to communicate that KVM should toggle the magic MSR? That'd avoid the helper+export, and up to this point "srso_mitigation" has been used only for documentation purposes.
The flag just needs to be cleared in two locations, which doesn't seem too awful. Though if we go that route, SRSO_MSR_FIX is a pretty crappy name :-)
Quoted 13 lines
arch/x86/lib/msr.c +1
-0@@ -103,6 +103,7 @@ 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.
arch/x86/lib/msr.c int msr_clear_bit(u32 msr, u8 bit) · line 122
Quoted 4 lines
arch/x86/lib/msr.c +1
-0@@ -118,6 +119,7 @@ int msr_clear_bit(u32 msr, u8 bit) {return __flip_bit (msr, bit, false); } +EXPORT_SYMBOL_GPL (msr_clear_bit);
These exports are no longer necessary.
Compile tested only...
--- Documentation/admin-guide/hw-vuln/srso.rst | 21 +++++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 15 ++++++++++++++- arch/x86/kvm/svm/svm.c | 14 ++++++++++++++ 5 files changed, 51 insertions(+), 1 deletion(-)
Documentation/admin-guide/hw-vuln/srso.rst +21 -0
@@ -104,6 +104,27 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. + In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -466,6 +466,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* MSR BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -717,6 +717,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +14 -1
@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2540,7 +2541,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2577,6 +2579,8 @@ static void __init srso_select_mitigation(void)if (!boot_cpu_has_bug (X86_BUG_SRSO ) ||cpu_mitigations_off () || srso_cmd ==SRSO_CMD_OFF ) { +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); +if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ;return ;
@@ -2665,6 +2669,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2686,6 +2696,9 @@ static void __init srso_select_mitigation(void) } out: +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); +pr_info ("%s\n" , srso_strings[srso_mitigation]); }
arch/x86/kvm/svm/svm.c +14 -0
@@ -257,6 +257,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1540,6 +1541,11 @@ 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 )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5306,6 +5312,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
base-commit: 9bec4a1f4ea92b99f96894e0c51c192b5345aa91
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 1 line · Sean Christopherson
No preference, either way works for me.
Yeah, too late now. Will queue it after -rc1.
Quoted 3 lines
Heh, no preference again. In case you want to add Co-developed-by, here's my SoB:
Signed-off-by Sean Christopherson <[email protected]>
Thanks. Added.
Quoted 1 line
Any objection to calling this
X86_FEATURE_SRSO_BP_SPEC_REDUCE? More below.
It sure is a better name. Lets leave SRSO_MSR_FIX in the comment so that grepping can find it as SRSO_MSR_FIX is what is in the official doc. IOW:
+#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31) /*
+ * BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs.
+ * (SRSO_MSR_FIX in the official doc).
+
Quoted 2 lines
At the risk of getting too clever, what if we use the
X86_FEATUREto communicate that KVM should toggle the magic MSR?
Not too clever at all. I myself have used exactly this scheme to communicate settings to other code and actually I should've thought about it... :-\
Definitely better than the silly export.
Quoted 5 lines
That'd avoid the helper+export, and up to this point "
srso_mitigation" has been used only for documentation purposes.The flag just needs to be cleared in two locations, which doesn't seem too awful. Though if we go that route,
SRSO_MSR_FIXis a pretty crappy name :-)
Yap, most def.
Quoted 1 line
These exports are no longer necessary.
Doh.
Quoted 1 line
Compile tested only...
Tested on hw.
Thanks!
---
From: "Borislav Petkov (AMD)" <[email protected]>
Date: Wed, 13 Nov 2024 13:41:10 +0100
Subject: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Add support for
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.
--- Documentation/admin-guide/hw-vuln/srso.rst | 21 +++++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 4 ++++ arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 14 +++++++++++++- arch/x86/kvm/svm/svm.c | 14 ++++++++++++++ 5 files changed, 53 insertions(+), 1 deletion(-)
Documentation/admin-guide/hw-vuln/srso.rst +21 -0
@@ -104,6 +104,27 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. + In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +4 -0
@@ -468,6 +468,10 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* +* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs. +* (SRSO_MSR_FIX in the official doc). +*/ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -719,6 +719,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +13 -1
@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2540,7 +2541,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2663,6 +2665,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {if (!boot_cpu_has (X86_FEATURE_ENTRY_IBPB ) && has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2684,6 +2692,10 @@ static void __init srso_select_mitigation(void) } out: + +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); +pr_info ("%s\n" , srso_strings[srso_mitigation]); }
arch/x86/kvm/svm/svm.c +14 -0
@@ -256,6 +256,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1541,6 +1542,11 @@ 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 )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5298,6 +5304,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
Signature · 9 lines
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Review static void __init srso_select_mitigation(void)
Quoted 2 lines · Borislav Petkov
static enum
srso_mitigationsrso_mitigation__ro_after_init=SRSO_MITIGATION_NONE;(quoted diff) +0
-0@@ -2663,6 +2665,12 @@ static void __init srso_select_mitigation(void)
Unless I'm missing something, the cpu_mitigations_off() and "srso_cmd == SRSO_CMD_OFF" cases need to clear the feature
arch/x86/kernel/cpu/bugs.c +1 -0
@@ -2581,6 +2581,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE );return ; }
There's also the Zen1/Zen2 ucode+!SMT path, which I assume is irreveleant in practice:
if (boot_cpu_data.x86 < 0x19 && !cpu_smt_possible()) {
setup_force_cpu_cap(X86_FEATURE_SRSO_NO);
return;
}But if we wanted to catch all paths, wrap the guts and clear the feature in the outer layer?
arch/x86/kernel/cpu/bugs.c +7 -3
@@ -2572,7 +2572,7 @@ early_param("spec_rstack_overflow", srso_parse_cmdline);#define SRSO_NOTICE "WARNING: See https://kernel.org/doc/html/latest/admin-guide/hw-vuln/srso.html for mitigation options." -static void __init srso_select_mitigation (void ) +static void __init __srso_select_mitigation (void ) {bool has_microcode =boot_cpu_has (X86_FEATURE_IBPB_BRTYPE );
@@ -2692,11 +2692,15 @@ static void __init srso_select_mitigation(void) } out: +pr_info ("%s\n" , srso_strings[srso_mitigation]); +} + +static void __init srso_select_mitigation (void ) +{ +__srso_select_mitigation ();if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE )setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); - -pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
Quoted 12 lines
(quoted diff) +6
-0ibpb_on_vmexit: case SRSO_CMD_IBPB_ON_VMEXIT: + if (boot_cpu_has(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) { + pr_notice("Reducing speculation to address VM/HV SRSO attack vector.\n"); + srso_mitigation = SRSO_MITIGATION_BP_SPEC_REDUCE; + break; + } + if (IS_ENABLED(CONFIG_MITIGATION_SRSO)) { if (!boot_cpu_has(X86_FEATURE_ENTRY_IBPB) && has_microcode) { setup_force_cpu_cap(X86_FEATURE_IBPB_ON_VMEXIT);
Review static void __init srso_select_mitigation(void) · line 2695
Quoted 4 lines
(quoted diff) +1
-0@@ -2684,6 +2692,10 @@ static void __init srso_select_mitigation(void) } out: +
Spurious newlines.
Quoted 5 lines
(quoted diff) +3
-0+ if (srso_mitigation != SRSO_MITIGATION_BP_SPEC_REDUCE) + setup_clear_cpu_cap(X86_FEATURE_SRSO_BP_SPEC_REDUCE); + pr_info("%s\n", srso_strings[srso_mitigation]); }
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 2 lines · Sean Christopherson
But if we wanted to catch all paths, wrap the guts and clear the feature in the outer layer?
Yap, all valid points, thanks for catching those.
Review
Quoted 9 lines
(quoted diff) +3
-2+static void __init srso_select_mitigation(void) +{ + __srso_select_mitigation(); if (srso_mitigation != SRSO_MITIGATION_BP_SPEC_REDUCE) setup_clear_cpu_cap(X86_FEATURE_SRSO_BP_SPEC_REDUCE); - - pr_info("%s\n", srso_strings[srso_mitigation]); }
What I'd like, though, here is to not dance around this srso_mitigation variable setting in __srso_select_mitigation() and then know that the __ function did modify it and now we can eval it.
I'd like for the __ function to return it like __ssb_select_mitigation() does.
But then if we do that, we'll have to do the same changes and turn the returns to "goto out" where all the paths converge. And I'd prefer if those paths converged anyway and not have some "early escapes" like those returns which I completely overlooked. :-\
And that code is going to change soon anyway after David's attack vectors series.
So, long story short, I guess the simplest thing to do would be to simply do the below.
I *think*.
I'll stare at it later, on a clear head again and test all cases to make sure nothing's escaping anymore.
Thx!
arch/x86/kernel/cpu/bugs.c +4 -4
@@ -2581,7 +2581,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2593,7 +2593,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2692,11 +2692,11 @@ static void __init srso_select_mitigation(void) } out: -if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE )setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); -pr_info ("%s\n" , srso_strings[srso_mitigation]); +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 27 lines · Borislav Petkov
Quoted 2 lines · Sean Christopherson
But if we wanted to catch all paths, wrap the guts and clear the feature in the outer layer?
Yap, all valid points, thanks for catching those.
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2700
Quoted 9 lines
arch/x86/kernel/cpu/bugs.c +3
-2+static void __init srso_select_mitigation (void ) +{ +__srso_select_mitigation ();if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE )setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); - -pr_info ("%s\n" , srso_strings[srso_mitigation]); }What I'd like, though, here is to not dance around this
srso_mitigationvariable setting in__srso_select_mitigation()and then know that the __ function did modify it and now we can eval it.I'd like for the __ function to return it like
__ssb_select_mitigation()does.But then if we do that, we'll have to do the same changes and turn the returns to "goto out" where all the paths converge. And I'd prefer if those paths converged anyway and not have some "early escapes" like those returns which I completely overlooked. :-\
Quoted 5 lines
And that code is going to change soon anyway after David's attack vectors series.
So, long story short, I guess the simplest thing to do would be to simply do the below.
I almost proposed that as well, the only reason I didn't is because I wasn't sure what to do with the pr_info() at the end.
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 2 lines · Sean Christopherson
I almost proposed that as well, the only reason I didn't is because I wasn't sure what to do with the
pr_info()at the end.
Yeah, that came up too while looking at David's patches. We're not really consistent when issuing the mitigation strings depending on the mitigation or whether we're affected or not...
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Borislav,
What is the performance impact of BpSpecReduce on Turin?
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 1 line · Jim Mattson
What is the performance impact of BpSpecReduce on Turin?
See upthread:
https://lore.kernel.org/all/20241216173142.GDZ2Bj_uPBG3TTPYd_@fat_crate.local
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Borislav PetkovFWIW, this should be the updated version of the patch with all the review comments posted so far.
Posting here just to have an overall view of how the new patch should look like. This is also based on todays Linus's master branch.
Compile tested only...
Best,
Patrick
---
From: "Borislav Petkov (AMD)" <[email protected]>
Add support for
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.
--- Documentation/admin-guide/hw-vuln/srso.rst | 20 ++++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/kernel/cpu/bugs.c | 21 +++++++++++++++++---- arch/x86/kvm/svm/svm.c | 14 ++++++++++++++ tools/arch/x86/include/asm/msr-index.h | 1 + 5 files changed, 53 insertions(+), 4 deletions(-)
Documentation/admin-guide/hw-vuln/srso.rst +20 -0
@@ -104,6 +104,26 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -468,6 +468,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs (SRSO_MSR_FIX in AMD docs). */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/kernel/cpu/bugs.c +17 -4
@@ -2522,6 +2522,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2539,7 +2540,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2578,7 +2580,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2590,7 +2592,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2670,6 +2672,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2691,7 +2699,12 @@ static void __init srso_select_mitigation(void) } out: -pr_info ("%s\n" , srso_strings[srso_mitigation]); + +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); + +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
arch/x86/kvm/svm/svm.c +14 -0
@@ -257,6 +257,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1540,6 +1541,11 @@ 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 )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5306,6 +5312,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
tools/arch/x86/include/asm/msr-index.h +1 -0
@@ -717,6 +717,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
Signature · 1 line
2.48.1.601.g30ceb7b040-goog
Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
And, of course, this bit:
Quoted 9 lines
tools/arch/x86/include/asm/msr-index.h +0
-0@@ -717,6 +717,7 @@Quoted 1 line
/* Zen4 */
#defineMSR_ZEN4_BP_CFG0xc001102e
+#defineMSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT4
has to be added to arch/x86/include/asm/msr-index.h as well.
Following (yet another) updated versions accounting for this.
Best,
Patrick
---
From: Borislav Petkov <[email protected]>
Add support for
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.
--- Documentation/admin-guide/hw-vuln/srso.rst | 20 ++++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 1 + arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 21 +++++++++++++++++---- arch/x86/kvm/svm/svm.c | 14 ++++++++++++++ tools/arch/x86/include/asm/msr-index.h | 1 + 6 files changed, 54 insertions(+), 4 deletions(-)
Documentation/admin-guide/hw-vuln/srso.rst +20 -0
@@ -104,6 +104,26 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +1 -0
@@ -468,6 +468,7 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs (SRSO_MSR_FIX in AMD docs). */ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -720,6 +720,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +17 -4
@@ -2522,6 +2522,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2539,7 +2540,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2578,7 +2580,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2590,7 +2592,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2670,6 +2672,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2691,7 +2699,12 @@ static void __init srso_select_mitigation(void) } out: -pr_info ("%s\n" , srso_strings[srso_mitigation]); + +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); + +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
arch/x86/kvm/svm/svm.c +14 -0
@@ -257,6 +257,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1540,6 +1541,11 @@ 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 )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5306,6 +5312,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
tools/arch/x86/include/asm/msr-index.h +1 -0
@@ -717,6 +717,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
Signature · 1 line
2.48.1.601.g30ceb7b040-goog
Borislav Petkov
maintainerRe: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
+ bjackman.
Quoted 1 line · Patrick Bellasi
Following (yet another) updated versions accounting for this.
Here's a tested one. You could've simply waited as I told you I'm working on it.
I'm going to queue this one next week into tip once Patrick's fix becomes part of -rc3.
Thx.
---
From: "Borislav Petkov (AMD)" <[email protected]>
Date: Wed, 13 Nov 2024 13:41:10 +0100
Subject: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Add support for
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 BpSpecReduce to mitigate SRSO across guest/host boundaries.
--- Documentation/admin-guide/hw-vuln/srso.rst | 21 +++++++++++++++++++ arch/x86/include/asm/cpufeatures.h | 4 ++++ arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++---- arch/x86/kvm/svm/svm.c | 14 +++++++++++++ 5 files changed, 60 insertions(+), 4 deletions(-)
Documentation/admin-guide/hw-vuln/srso.rst +21 -0
@@ -104,6 +104,27 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary. + In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +4 -0
@@ -468,6 +468,10 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* +* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs. +* (SRSO_MSR_FIX in the official doc). +*/ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -721,6 +721,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +20 -4
@@ -2522,6 +2522,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2539,7 +2540,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2578,7 +2580,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2590,7 +2592,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2670,6 +2672,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2691,7 +2699,15 @@ static void __init srso_select_mitigation(void) } out: -pr_info ("%s\n" , srso_strings[srso_mitigation]); +/* +* Clear the feature flag if this mitigation is not selected as that +* feature flag controls the BpSpecReduce MSR bit toggling in KVM. +*/ +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); + +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
arch/x86/kvm/svm/svm.c +14 -0
@@ -257,6 +257,7 @@ DEFINE_PER_CPU(struct svm_cpu_data, svm_data);* defer the restoration of TSC_AUX until the CPU returns to userspace. */ static int tsc_aux_uret_slot__read_mostly =-1 ; +static int zen4_bp_cfg_uret_slot__ro_after_init =-1 ;static const u32 msrpm_ranges[] = {0 ,0xc0000000 ,0xc0010000 };
@@ -1540,6 +1541,11 @@ 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 )) +kvm_set_user_return_msr (zen4_bp_cfg_uret_slot, +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ), +BIT_ULL (MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT )); + svm->guest_state_loaded = true; }
@@ -5306,6 +5312,14 @@ static __init int svm_hardware_setup(void) tsc_aux_uret_slot =kvm_add_user_return_msr (MSR_TSC_AUX ); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { + zen4_bp_cfg_uret_slot =kvm_add_user_return_msr (MSR_ZEN4_BP_CFG ); +if (WARN_ON_ONCE (zen4_bp_cfg_uret_slot <0 )) { + r = -EIO ; + goto err; + } + } +if (boot_cpu_has (X86_FEATURE_AUTOIBRS ))kvm_enable_efer_bits (EFER_AUTOIBRS );
Signature · 7 lines
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 5 lines · Patrick Bellasi
Quoted 2 lines
Following (yet another) updated versions accounting for this.
Here's a tested one. You could've simply waited as I told you I'm working on it.
Thanks Borislav, no problems for me, I'm just looking into these SRSO fixes and what I did was mostly a way to ramp up.
Quoted 2 lines
I'm going to queue this one next week into tip once Patrick's fix becomes part of -rc3.
Thanks for all the efforts.
Meanwhile I've a couple of questions about the approach proposed here.
Quoted 10 lines
+ * 'Mitigation: Reduced Speculation':
Documentation/admin-guide/hw-vuln/srso.rst +9
-0+ + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. +
Documentation/admin-guide/hw-vuln/srso.rst The possible values in this file are: · line 126
Quoted 10 lines
Documentation/admin-guide/hw-vuln/srso.rst +10
-0+ After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. + + Currently, the mitigation uses KVM's user_return approach + (kvm_set_user_return_msr()) to set the BpSpecReduce bit when a vCPU runs + a guest and reset it upon return to host userspace or when the KVM module + is unloaded. The intent being, the small perf impact of BpSpecReduce should + be incurred only when really necessary.
According to [1], the AMD Whitepaper on SRSO says:
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.
The "should be set identically across all processors in the system" makes me wondering if using the "KVM's user_return approach" proposed here is robust enough. Could this not lead to the bit being possibly set only on some CPU but not others?
Am I missing something? Is the MSR used something "shared" across all the cores of a CPU, i.e. as long a vCPU is running the entire system is still protected? Maybe there is even just a more recent whitepaper (I did not find) online with a different recommendation?
Also, setting the MSR bit only while the guest is running, is it sufficient to prevent both speculation and training?
Think about this scenario:
(a) Guest runs with BpSpecReduce MSR set by KVM and performs training.
(b) We exit into userspace and clear BpSpecReduce.
(c) We go back into the kernel with BpSpecReduce cleared and the guest training intact.
If (a) can be done (i.e. the MSR does not prevent training), don't we end up in a vulnerable state in (c)?
If BpSpecReduce does not prevent training, but only the training from being used, should not we keep it consistently set after a guest has run, or until an IBPB is executed?
Best,
Patrick
Borislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 4 lines · Patrick Bellasi
The "should be set identically across all processors in the system" makes me wondering if using the "KVM's
user_returnapproach" proposed here is robust enough. Could this not lead to the bit being possibly set only on some CPU but not others?
That's fine, we should update that paper.
Quoted 3 lines
If BpSpecReduce does not prevent training, but only the training from being used, should not we keep it consistently set after a guest has run, or until an IBPB is executed?
After talking with folks internally, you're probably right. We should slap an IBPB before clearing. Which means, I cannot use the MSR return slots anymore. I will have to resurrect some of the other solutions we had lined up...
Stay tuned.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 12 lines · Borislav Petkov
Quoted 4 lines · Patrick Bellasi
The "should be set identically across all processors in the system" makes me wondering if using the "KVM's
user_returnapproach" proposed here is robust enough. Could this not lead to the bit being possibly set only on some CPU but not others?That's fine, we should update that paper.
Quoted 3 lines
If BpSpecReduce does not prevent training, but only the training from being used, should not we keep it consistently set after a guest has run, or until an IBPB is executed?
Quoted 5 lines
After talking with folks internally, you're probably right. We should slap an IBPB before clearing. Which means, I cannot use the MSR return slots anymore. I will have to resurrect some of the other solutions we had lined up...
Stay tuned.
Thanks for working on this!
Should this patch (and the two previously merged patches) be backported to stable? I noticed they did not have CC:stable.
Quoted 8 lines
Thx.
Signature · 4 lines
Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 2 lines · Yosry Ahmed
Should this patch (and the two previously merged patches) be backported to stable? I noticed they did not have CC:stable.
Because a stable backport would require manual backporting, depending on where it goes. And Brendan, Patrick or I are willing to do that - it's a question of who gets there first. :-)
And folks, whoever wants to backport, pls tell the others so that we don't duplicate work.
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 7 lines · Borislav Petkov
Quoted 2 lines · Yosry Ahmed
Should this patch (and the two previously merged patches) be backported to stable? I noticed they did not have CC:stable.
Because a stable backport would require manual backporting, depending on where it goes. And Brendan, Patrick or I are willing to do that - it's a question of who gets there first. :-)
Thanks for the context, didn't realize a manual backport was needed.
Borislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 1 line · Yosry Ahmed
Thanks for the context, didn't realize a manual backport was needed.
Feb 17 [email protected] (:9.1K|) FAILED: patch "[PATCH] x86/cpu/kvm: SRSO: Fix possible missing IBPB on VM-Exit" failed to apply to 6.6-stable tree Feb 17 [email protected] (:9.1K|) FAILED: patch "[PATCH] x86/cpu/kvm: SRSO: Fix possible missing IBPB on VM-Exit" failed to apply to 6.1-stable tree Feb 17 [email protected] (:9.1K|) FAILED: patch "[PATCH] x86/cpu/kvm: SRSO: Fix possible missing IBPB on VM-Exit" failed to apply to 5.15-stable tree Feb 17 [email protected] (:9.2K|) FAILED: patch "[PATCH] x86/cpu/kvm: SRSO: Fix possible missing IBPB on VM-Exit" failed to apply to 5.10-stable tree Feb 17 [email protected] (:9.5K|) FAILED: patch "[PATCH] x86/cpu/kvm: SRSO: Fix possible missing IBPB on VM-Exit" failed to apply to 5.4-stable tree
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Borislav PetkovQuoted 3 lines · Borislav Petkov
After talking with folks internally, you're probably right. We should slap an IBPB before clearing. Which means, I cannot use the MSR return slots anymore. I will have to resurrect some of the other solutions we had lined up...
So I'm thinking about this (totally untested ofc) but I'm doing it in the CLGI region so no need to worry about migration etc.
Sean, that ok this way?
arch/x86/kvm/svm/svm.c +8 -0
@@ -4272,8 +4272,16 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,if (!static_cpu_has (X86_FEATURE_V_SPEC_CTRL ))x86_spec_ctrl_set_guest (svm->virt_spec_ctrl); +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 ); +svm_vcpu_enter_exit (vcpu, spec_ctrl_intercepted); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +indirect_branch_prediction_barrier (); +msr_clear_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); + } +if (!static_cpu_has (X86_FEATURE_V_SPEC_CTRL ))x86_spec_ctrl_restore_host (svm->virt_spec_ctrl);
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 9 lines · Borislav Petkov
Quoted 3 lines · Borislav Petkov
After talking with folks internally, you're probably right. We should slap an IBPB before clearing. Which means, I cannot use the MSR return slots anymore. I will have to resurrect some of the other solutions we had lined up...
So I'm thinking about this (totally untested ofc) but I'm doing it in the CLGI region so no need to worry about migration etc.
Sean, that ok this way?
I am no Sean, but I have some questions about this approach if that's okay :)
First of all, the use of indirect_branch_prediction_barrier() is interesting to me because it only actually performs an IBPB if X86_FEATURE_USE_IBPB is set, which is set according to the spectre_v2 mitigation. It seems to me that indirect_branch_prediction_barrier() was originally intended for use only in switch_mm_irqs_off() -> cond_mitigation(), where the spectre_v2 mitigations are executed, then made its way to other places like KVM.
Second, and probably more importantly, it seems like with this approach we will essentially be doing an IBPB on every VM-exit AND running the guest with reduced speculation. At least on the surface, this looks worse than an IBPB on VM-exit. My understanding is that this MSR is intended to be a more efficient mitigation than IBPB on VM-exit.
This probably performs considerably worse than the previous approaches, so I am wondering which approach is the 1-2% regression figure associated with.
If 1-2% is the cost for keeping the MSR enabled at all times, I wonder if we should just do that for simplicitly, and have it its own mitigation option (chosen by the cmdline).
Alternatively, if that's too expensive, perhaps we can choose another boundary to clear the MSR at and perform an IBPB. I can think of two places:
- Upon return to userspace (similar to your previous proposal). In this case we run userspace with the MSR cleared, and only perform an IBPB in the exit to userspace pace.
- In the switch_mm() path (around cond_mitigation()). Basically we keep the MSR bit set until we go to a different process, at which point we clear it and do an IBPB. The MSR will bet set while the VMM is running, but other processes in the system won't take the hit. We can even be smarter and only do the MSR clear + IBPB if we're going from a process using KVM to process that isn't. We'll need more bookkeeping for that though.
Any thoughts? Am I completely off base?
Quoted 27 lines
arch/x86/kvm/svm/svm.c +8
-0@@ -4272,8 +4272,16 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,if (!static_cpu_has (X86_FEATURE_V_SPEC_CTRL ))x86_spec_ctrl_set_guest (svm->virt_spec_ctrl); +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 ); +svm_vcpu_enter_exit (vcpu, spec_ctrl_intercepted); +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +indirect_branch_prediction_barrier (); +msr_clear_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); + } +if (!static_cpu_has (X86_FEATURE_V_SPEC_CTRL ))x86_spec_ctrl_restore_host (svm->virt_spec_ctrl);Signature · 4 lines
Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 3 lines · Yosry Ahmed
If 1-2% is the cost for keeping the MSR enabled at all times, I wonder if we should just do that for simplicitly, and have it its own mitigation option (chosen by the cmdline).
Yes, that's what David and I think we should do initially.
Then we can chase some more involved scheme like setting the bit before the first VM runs and then clearing it when the last one exits. I *think* I've seen something like that in KVM but I need to stare in detail.
Quoted 3 lines
- Upon return to userspace (similar to your previous proposal). In this case we run userspace with the MSR cleared, and only perform an IBPB in the exit to userspace pace.
You want to IBPB before clearing the MSR as otherwise host kernel will be running with the mistrained gunk from the guest.
Quoted 1 line
Any thoughts?
Yes, let's keep it simple and do anything more involved *only* if it is really necessary.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 11 lines · Borislav Petkov
Quoted 3 lines · Yosry Ahmed
If 1-2% is the cost for keeping the MSR enabled at all times, I wonder if we should just do that for simplicitly, and have it its own mitigation option (chosen by the cmdline).
Yes, that's what David and I think we should do initially.
Then we can chase some more involved scheme like setting the bit before the first VM runs and then clearing it when the last one exits. I *think* I've seen something like that in KVM but I need to stare in detail.
Quoted 6 lines
Quoted 3 lines
- Upon return to userspace (similar to your previous proposal). In this case we run userspace with the MSR cleared, and only perform an IBPB in the exit to userspace pace.
You want to IBPB before clearing the MSR as otherwise host kernel will be running with the mistrained gunk from the guest.
I meant IBPB + MSR clear before going to userspace, or IBPB + MSR clear before a context switch.
Quoted 11 lines
Quoted 2 lines
Any thoughts?
Yes, let's keep it simple and do anything more involved *only* if it is really necessary.
Signature · 4 lines
Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov
maintainerRe: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 2 lines · Yosry Ahmed
I meant IBPB + MSR clear before going to userspace, or IBPB + MSR clear before a context switch.
Basically what I said already:
"Yes, let's keep it simple and do anything more involved *only* if it is really necessary."
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 4 lines · Borislav Petkov
Quoted 1 line · Yosry Ahmed
Quoted 8 lines
Quoted 3 lines
I meant IBPB + MSR clear before going to userspace, or IBPB + MSR clear
before a context switch.Basically what I said already:
"Yes, let's keep it simple and do anything more involved *only* if it is really necessary."
Right, I agree as I mentioned earlier (assuming the perf hit is 1-2%), just wanted to clarify what I meant.
Borislav Petkov
maintainerPATCH 1/1 x86/bugs: KVM: Add support for SRSO_MSR_FIX
So,
in the interest of finally making some progress here I'd like to commit this below (will test it one more time just in case but it should work :-P). It is simple and straight-forward and doesn't need an IBPB when the bit gets cleared.
A potential future improvement is David's suggestion that there could be a way for tracking when the first guest gets started, we set the bit then, we make sure the bit gets set on each logical CPU when the guests migrate across the machine and when the *last* guest exists, that bit gets cleared again.
It all depends on how much machinery is additionally needed and how much ugly it becomes and how much noticeable the perf impact even is from simply setting that bit blindly on every CPU...
But something we can chase later, once *some* fix is there first.
Thx.
---
Add support for
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 BpSpecReduce to mitigate SRSO across guest/host boundaries.
Switch back to enabling the bit when virtualization is enabled and to clear the bit when virtualization is disabled because using a MSR slot would clear the bit when the guest is exited and any training the guest has done, would potentially influence the host kernel when execution enters the kernel and hasn't VMRUN the guest yet.
More detail on the public thread in Link.
--- Documentation/admin-guide/hw-vuln/srso.rst | 13 ++++++++++++ arch/x86/include/asm/cpufeatures.h | 4 ++++ arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++---- arch/x86/kvm/svm/svm.c | 6 ++++++ arch/x86/lib/msr.c | 2 ++ 6 files changed, 46 insertions(+), 4 deletions(-)
Documentation/admin-guide/hw-vuln/srso.rst +13 -0
@@ -104,7 +104,20 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +4 -0
@@ -468,6 +468,10 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* +* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs. +* (SRSO_MSR_FIX in the official doc). +*/ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -721,6 +721,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +20 -4
@@ -2522,6 +2522,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2539,7 +2540,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2578,7 +2580,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2590,7 +2592,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2670,6 +2672,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2691,7 +2699,15 @@ static void __init srso_select_mitigation(void) } out: -pr_info ("%s\n" , srso_strings[srso_mitigation]); +/* +* Clear the feature flag if this mitigation is not selected as that +* feature flag controls the BpSpecReduce MSR bit toggling in KVM. +*/ +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); + +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
arch/x86/kvm/svm/svm.c +6 -0
@@ -607,6 +607,9 @@ 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 )
@@ -684,6 +687,9 @@ 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 ; }
arch/x86/lib/msr.c +2 -0
@@ -103,6 +103,7 @@ 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.
@@ -118,6 +119,7 @@ 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)
Signature · 8 lines
2.43.0
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 4 lines
in the interest of finally making some progress here I'd like to commit this below (will test it one more time just in case but it should work :-P). It is simple and straight-forward and doesn't need an IBPB when the bit gets cleared.
That's indeed simple and straight-forward for the time being.
Maybe a small improvement we could add on top is to have a separate and dedicated cmdline option?
Indeed, with X86_FEATURE_SRSO_USER_KERNEL_NO we are not effectively using an IBPB on VM-Exit anymore. Something like the diff down below?
Best,
Patrick
---
arch/x86/kernel/cpu/bugs.c +12 -8
@@ -2531,6 +2531,7 @@ enum srso_mitigation_cmd {SRSO_CMD_SAFE_RET ,SRSO_CMD_IBPB ,SRSO_CMD_IBPB_ON_VMEXIT , + SRSO_CMD_BP_SPEC_REDUCE, }; static const char * const srso_strings[] = {
@@ -2562,6 +2563,8 @@ static int __init srso_parse_cmdline(char *str) srso_cmd =SRSO_CMD_IBPB ; else if (!strcmp(str, "ibpb-vmexit")) srso_cmd =SRSO_CMD_IBPB_ON_VMEXIT ; + else if (!strcmp(str, "spec-reduce")) + srso_cmd =SRSO_CMD_BP_SPEC_REDUCE ; else pr_err("Ignoring unknown SRSO option (%s).", str);
@@ -2617,7 +2620,7 @@ static void __init srso_select_mitigation(void)case SRSO_CMD_SAFE_RET :if (boot_cpu_has (X86_FEATURE_SRSO_USER_KERNEL_NO )) - goto ibpb_on_vmexit; + goto spec_reduce;if (IS_ENABLED (CONFIG_MITIGATION_SRSO )) {/*
@@ -2670,14 +2673,7 @@ static void __init srso_select_mitigation(void) }break ; -ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : -if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { -pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); - srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; -break ; - } -if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2694,6 +2690,14 @@ static void __init srso_select_mitigation(void)pr_err ("WARNING: kernel not compiled with MITIGATION_IBPB_ENTRY.\n" ); }break ; + +spec_reduce: +case SRSO_CMD_BP_SPEC_REDUCE : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + }default :break ; }
Borislav Petkov
maintainerRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 5 lines · Patrick Bellasi
Maybe a small improvement we could add on top is to have a separate and dedicated cmdline option?
Indeed, with
X86_FEATURE_SRSO_USER_KERNEL_NOwe are not effectively using an IBPB on VM-Exit anymore. Something like the diff down below?
Except that I don't see the point of this yet one more cmdline option. Our mitigations options space is a nightmare. Why do we want to add another one?
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 9 lines · Patrick Bellasi
Quoted 5 lines
Maybe a small improvement we could add on top is to have a separate and dedicated cmdline option?
Indeed, with
X86_FEATURE_SRSO_USER_KERNEL_NOwe are not effectively using an IBPB on VM-Exit anymore. Something like the diff down below?Except that I don't see the point of this yet one more cmdline option. Our mitigations options space is a nightmare. Why do we want to add another one?
The changelog of the following patch provides the motivations.
Do you think something like the following self contained change can be added on top of your change?
Best,
Patrick
---
From 62bd6151cdb5f8e3322d8c91166cf89b6ed9f5b6 Mon Sep 17 00:00:00 2001
From: Patrick Bellasi <[email protected]>
Date: Mon, 24 Feb 2025 17:41:30 +0000
Subject: [PATCH] x86/bugs: Add explicit BP_SPEC_REDUCE cmdline
Some AMD CPUs are vulnerable to SRSO only limited to the Guest->Host attack vector. When no command line options are specified, the default SRSO mitigation on these CPUs is "safe-ret", which is optimized to use "ibpb-vmexit". A further optimization, introduced in [1], replaces IBPB on VM-Exits with the more efficient BP_SPEC_REDUCE mitigation when the CPU reports X86_FEATURE_SRSO_BP_SPEC_REDUCE support.
The current implementation in bugs.c automatically selects the best mitigation for a CPU when no command line options are provided. However, it lacks the ability to explicitly choose between IBPB and BP_SPEC_REDUCE.
In some scenarios it could be interesting to mitigate SRSO only when the low overhead of BP_SPEC_REDUCE is available, without overwise falling back to an IBPB at each VM-Exit.
More in general, an explicit control is valuable for testing, benchmarking, and comparing the behavior and overhead of IBPB versus BP_SPEC_REDUCE.
Add a new kernel cmdline option to explicitly select BP_SPEC_REDUCE. Do that with a minimal change that does not affect the current SafeRET "fallback logic". Do warn when reduced speculation is required but the support not available and properly report the vulnerability reason.
[1] https://lore.kernel.org/lkml/20250218111306.GFZ7RrQh3RD4JKj1lu@fat_crate.local/
--- arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
arch/x86/kernel/cpu/bugs.c +18 -6
@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT ,SRSO_MITIGATION_BP_SPEC_REDUCE , + SRSO_MITIGATION_BP_SPEC_REDUCE_NA, }; enum srso_mitigation_cmd {
@@ -2531,6 +2532,7 @@ enum srso_mitigation_cmd {SRSO_CMD_SAFE_RET ,SRSO_CMD_IBPB ,SRSO_CMD_IBPB_ON_VMEXIT , + SRSO_CMD_BP_SPEC_REDUCE, }; static const char * const srso_strings[] = {
@@ -2542,6 +2544,7 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation", + [SRSO_MITIGATION_BP_SPEC_REDUCE_NA] = "Vulnerable: Reduced Speculation, not available", };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2562,6 +2565,8 @@ static int __init srso_parse_cmdline(char *str) srso_cmd =SRSO_CMD_IBPB ; else if (!strcmp(str, "ibpb-vmexit")) srso_cmd =SRSO_CMD_IBPB_ON_VMEXIT ; + else if (!strcmp(str, "bp-spec-reduce")) + srso_cmd =SRSO_CMD_BP_SPEC_REDUCE ; else pr_err("Ignoring unknown SRSO option (%s).", str);
@@ -2672,12 +2677,8 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : -if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { -pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); - srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; -break ; - } - +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) + goto bp_spec_reduce;if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2694,6 +2695,17 @@ static void __init srso_select_mitigation(void)pr_err ("WARNING: kernel not compiled with MITIGATION_IBPB_ENTRY.\n" ); }break ; + +case SRSO_CMD_BP_SPEC_REDUCE : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +bp_spec_reduce: +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + }else { + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE_NA ; +pr_warn ("BP_SPEC_REDUCE not supported!\n" ); + }default :break ; }
--
2.48.1.711.g2feabab25a-goog
Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 31 lines · Patrick Bellasi
Quoted 9 lines · Patrick Bellasi
Quoted 5 lines
Maybe a small improvement we could add on top is to have a separate and dedicated cmdline option?
Indeed, with
X86_FEATURE_SRSO_USER_KERNEL_NOwe are not effectively using an IBPB on VM-Exit anymore. Something like the diff down below?Except that I don't see the point of this yet one more cmdline option. Our mitigations options space is a nightmare. Why do we want to add another one?
The changelog of the following patch provides the motivations.
Do you think something like the following self contained change can be added on top of your change?
Best,
Patrick---
From62bd6151cdb5f8e3322d8c91166cf89b6ed9f5b6Mon Sep 17 00:00:00 2001
From: Patrick Bellasi <[email protected]>
Date: Mon, 24 Feb 2025 17:41:30 +0000
Subject: [PATCH] x86/bugs: Add explicitBP_SPEC_REDUCEcmdlineSome AMD CPUs are vulnerable to SRSO only limited to the
Guest->Hostattack vector. When no command line options are specified, the default SRSO mitigation on these CPUs is "safe-ret", which is optimized to use "ibpb-vmexit". A further optimization, introduced in [1], replaces IBPB on VM-Exits with the more efficientBP_SPEC_REDUCEmitigation when the CPU reportsX86_FEATURE_SRSO_BP_SPEC_REDUCEsupport.
Quoted 7 lines
The current implementation in
bugs.cautomatically selects the best mitigation for a CPU when no command line options are provided. However, it lacks the ability to explicitly choose between IBPB andBP_SPEC_REDUCE.
In some scenarios it could be interesting to mitigate SRSO only when the low overhead ofBP_SPEC_REDUCEis available, without overwise falling back to an IBPB at each VM-Exit.
To add more details about this, we are using ASI as our main mitigation for SRSO. However, it's likely that bp-spec-reduce is cheaper, so we basically want to always use bp-spec-reduce if available, if not, we don't want the ibpb-on-vmexit or safe-ret as they are a lot more expensive than ASI.
So we want the cmdline option to basically say only use bp-spec-reduce if it's available, but don't fallback if it isn't. On the other hand we are enlighting ASI to skip mitigating SRSO if X86_FEATURE_SRSO_BP_SPEC_REDUCE is enabled
Quoted 69 lines
More in general, an explicit control is valuable for testing, benchmarking, and comparing the behavior and overhead of IBPB versus
BP_SPEC_REDUCE.Add a new kernel cmdline option to explicitly select
BP_SPEC_REDUCE. Do that with a minimal change that does not affect the current SafeRET "fallback logic". Do warn when reduced speculation is required but the support not available and properly report the vulnerability reason.[1] https://lore.kernel.org/lkml/20250218111306.GFZ7RrQh3RD4JKj1lu@fat_crate.local/
Signed-off-by Patrick Bellasi <[email protected]>--- arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)arch/x86/kernel/cpu/bugs.c +7
-6@@ -2523,6 +2523,7 @@ enum srso_mitigation {SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT ,SRSO_MITIGATION_BP_SPEC_REDUCE , + SRSO_MITIGATION_BP_SPEC_REDUCE_NA, }; enum srso_mitigation_cmd {@@ -2531,6 +2532,7 @@ enum srso_mitigation_cmd {SRSO_CMD_SAFE_RET ,SRSO_CMD_IBPB ,SRSO_CMD_IBPB_ON_VMEXIT , + SRSO_CMD_BP_SPEC_REDUCE, }; static const char * const srso_strings[] = {@@ -2542,6 +2544,7 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation", + [SRSO_MITIGATION_BP_SPEC_REDUCE_NA] = "Vulnerable: Reduced Speculation, not available", };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;@@ -2562,6 +2565,8 @@ static int __init srso_parse_cmdline(char *str) srso_cmd =SRSO_CMD_IBPB ; else if (!strcmp(str, "ibpb-vmexit")) srso_cmd =SRSO_CMD_IBPB_ON_VMEXIT ; + else if (!strcmp(str, "bp-spec-reduce")) + srso_cmd =SRSO_CMD_BP_SPEC_REDUCE ; else pr_err("Ignoring unknown SRSO option (%s).", str);@@ -2672,12 +2677,8 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : -if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { -pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); - srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; -break ; - } - +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) + goto bp_spec_reduce;if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2701
Quoted 7 lines
arch/x86/kernel/cpu/bugs.c +4
-0@@ -2694,6 +2695,17 @@ static void __init srso_select_mitigation(void)pr_err ("WARNING: kernel not compiled with MITIGATION_IBPB_ENTRY.\n" ); }break ; + +case SRSO_CMD_BP_SPEC_REDUCE : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +bp_spec_reduce:
We can put this label above 'case SRSO_CMD_BP_SPEC_REDUCE' for consistency with the ibpb_on_vmexit label. The X86_FEATURE_SRSO_BP_SPEC_REDUCE check will be repeated but it shouldn't matter in practice and the compiler will probably optimize it anyway.
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2706
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +5
-0+pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + }else { + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE_NA ;
Why do we need SRSO_MITIGATION_BP_SPEC_REDUCE_NA? It seems like in other cases, if some requirements are not met, we just leave srso_mitigation set SRSO_MITIGATION_NONE.
Quoted 7 lines
arch/x86/kernel/cpu/bugs.c +2
-1+pr_warn ("BP_SPEC_REDUCE not supported!\n" ); + }default :break ; } --2.48.1.711.g2feabab25a-goog
Borislav Petkov
maintainerRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 8 lines · Yosry Ahmed
To add more details about this, we are using ASI as our main mitigation for SRSO. However, it's likely that bp-spec-reduce is cheaper, so we basically want to always use bp-spec-reduce if available, if not, we don't want the ibpb-on-vmexit or safe-ret as they are a lot more expensive than ASI.
So we want the cmdline option to basically say only use bp-spec-reduce if it's available, but don't fallback if it isn't.
Yap, that should also be a part of the commit message.
Quoted 2 lines
On the other hand we are enlighting ASI to skip mitigating SRSO if
X86_FEATURE_SRSO_BP_SPEC_REDUCEis enabled
Yap, makes sense.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Patrick Bellasiarch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2702
Quoted 5 lines · Patrick Bellasi
arch/x86/kernel/cpu/bugs.c +5
-0+ +case SRSO_CMD_BP_SPEC_REDUCE : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +bp_spec_reduce: +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" );
Probably not needed anymore as that will be in srso_strings which is issued later.
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2708
Quoted 6 lines
arch/x86/kernel/cpu/bugs.c +6
-0+ srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + }else { + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE_NA ; +pr_warn ("BP_SPEC_REDUCE not supported!\n" ); + }
This is the part I'm worried about: user hears somewhere "bp-spec-reduce" is faster, sets it but doesn't know whether the hw even supports it. Machine boots, warns which is a single line and waaay buried in dmesg and continues unmitigated.
So *maybe* we can make this a lot more subtle and say:
srso=__dont_fall_back_to_ibpb_on_vmexit_if_bp_spec_reduce__
(joking about the name but that should be the gist of what it means)
and then act accordingly when that is specified along with a big fat:
WARN_ON(..."You should not use this as a mitigation option if you don't know what you're doing")
along with a big fat splat in dmesg.
Hmmm...?
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 9 lines · Patrick Bellasi
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2702
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +5
-0+ +case SRSO_CMD_BP_SPEC_REDUCE : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +bp_spec_reduce: +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" );Probably not needed anymore as that will be in
srso_stringswhich is issued later.
Good point, the above can certainly be dropped.
Quoted 11 lines
arch/x86/kernel/cpu/bugs.c static void __init srso_select_mitigation(void) · line 2708
Quoted 6 lines
arch/x86/kernel/cpu/bugs.c +6
-0+ srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + }else { + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE_NA ; +pr_warn ("BP_SPEC_REDUCE not supported!\n" ); + }This is the part I'm worried about: user hears somewhere "bp-spec-reduce" is faster, sets it but doesn't know whether the hw even supports it. Machine boots, warns which is a single line and waaay buried in dmesg and continues unmitigated.
That's why we are also going to detect this cases and set SRSO_MITIGATION_BP_SPEC_REDUCE_NA, so that we get a:
"Vulnerable: Reduced Speculation, not available"
from vulnerabilities/spec_rstack_overflow, which should be the only place users look for to assess the effective mitigation posture, ins't it?
Quoted 5 lines
So *maybe* we can make this a lot more subtle and say:
srso=__dont_fall_back_to_ibpb_on_vmexit_if_bp_spec_reduce__
(joking about the name but that should be the gist of what it means)
I can think about it, but this seems something different than the common practice, i.e. specify at cmdline what you want and be prepares on possibly not to get it.
Quoted 8 lines
and then act accordingly when that is specified along with a big fat:
WARN_ON(..."You should not use this as a mitigation option if you don't know what you're doing")along with a big fat splat in dmesg.
Hmmm...?
After all the above already happens, e.g. if we ask for ibpb-vmexit but the machine has not the ucode. In this case we still have to check the vulnerabilities report to know that we are:
"Vulnerable: No microcode"
--
#include <best/regards.h>
Patrick Bellasi (derkling@)
Borislav Petkov
maintainerRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 7 lines · Patrick Bellasi
That's why we are also going to detect this cases and set
SRSO_MITIGATION_BP_SPEC_REDUCE_NA, so that we get a:"Vulnerable: Reduced Speculation, not available"
from vulnerabilities/spec_rstack_overflow, which should be the only place users look for to assess the effective mitigation posture, ins't it?
If they even look. The strategy so far has been that the kernel should simply DTRT (it being the default) if the user doesn't know anything about mitigations etc.
So I have another idea: how about we upstream enough ASI bits - i.e., the function which checks whether ASI is enabled - and use that in the mitigation selection?
IOW:
case SRSO_CMD_BP_SPEC_REDUCE:
if ((boot_cpu_has(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) {
select it
} else {
if (ASI enabled)
do not fall back to IBPB;
else
fallback to IBPB;
}"ASI enabled" will return false upstream - at least initially only, until ASI is out-of-tree - and then it'll fall back.
On your kernels, it'll return true and there it won't fall back.
We just need to sync with Brendan what "ASI enabled" would be and then it should work and your backports would be easy in that respect.
Until ASI is not upstream, that is.
Hmmmm?
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 31 lines · Borislav Petkov
Quoted 7 lines · Patrick Bellasi
That's why we are also going to detect this cases and set
SRSO_MITIGATION_BP_SPEC_REDUCE_NA, so that we get a:"Vulnerable: Reduced Speculation, not available"
from vulnerabilities/spec_rstack_overflow, which should be the only place users look for to assess the effective mitigation posture, ins't it?
If they even look. The strategy so far has been that the kernel should simply DTRT (it being the default) if the user doesn't know anything about mitigations etc.
So I have another idea: how about we upstream enough ASI bits - i.e., the function which checks whether ASI is enabled - and use that in the mitigation selection?
IOW:
case SRSO_CMD_BP_SPEC_REDUCE: if ((boot_cpu_has(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) { select it } else { if (ASI enabled) do not fall back to IBPB; else fallback to IBPB; }"ASI enabled" will return false upstream - at least initially only, until ASI is out-of-tree - and then it'll fall back.
Quoted 8 lines
On your kernels, it'll return true and there it won't fall back.
We just need to sync with Brendan what "ASI enabled" would be and then it should work and your backports would be easy in that respect.
Until ASI is not upstream, that is.
Hmmmm?
This seems like a good idea to me, assuming we want ASI in the code eventually it seems worthwhile to make visible the places where we know we'll want to update the code when we get it in.
In RFCv2 this would be static_asi_enabled() [1] - I think in the current implementation it would be fine to use it directly, but in general we do need to be aware of initializion order.
[0] (first half of)
https://lore.kernel.org/all/DS0PR12MB9273553AE4096FCCBBB4000E94D62@DS0PR12MB9273.namprd12.prod.outlook.com/
[1]
https://lore.kernel.org/linux-mm/[email protected]/
Of course I'm biased here, from my perspective having such mentions of ASI in the code is unambiguously useful. But if others perceived it as useless noise I would understand!
Borislav Petkov
maintainerRe: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX
Quoted 7 lines · Brendan Jackman
This seems like a good idea to me, assuming we want ASI in the code eventually it seems worthwhile to make visible the places where we know we'll want to update the code when we get it in.
In RFCv2 this would be
static_asi_enabled()[1] - I think in the current implementation it would be fine to use it directly, but in general we do need to be aware of initializion order.
Right, I'd suggest you whack that thing and use cpu_feature_enabled() directly. No need for the indirection.
And I see you're setting X86_FEATURE_ASI in asi_check_boottime_disable() - I'm presuming that's early enough so that cpu_select_mitigations() in bugs.c can see it so that srso_select_mitigation() can act accordingly...
Quoted 3 lines
Of course I'm biased here, from my perspective having such mentions of ASI in the code is unambiguously useful. But if others perceived it as useless noise I would understand!
Yeah, well, it'll be a single feature check in srso_select_mitigation() with a big-fat comment in it explaining why so I think that should be ok...
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteBorislav Petkov
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
In reply to Borislav PetkovQuoted 11 lines · Borislav Petkov
So,
in the interest of finally making some progress here I'd like to commit this below (will test it one more time just in case but it should work :-P). It is simple and straight-forward and doesn't need an IBPB when the bit gets cleared.
A potential future improvement is David's suggestion that there could be a way for tracking when the first guest gets started, we set the bit then, we make sure the bit gets set on each logical CPU when the guests migrate across the machine and when the *last* guest exists, that bit gets cleared again.
Well, that "simplicity" was short-lived:
https://www.phoronix.com/review/linux-615-amd-regression
Sean, how about this below?
It is hacky and RFC-ish - i.e., don't look too hard at it - but basically I'm pushing down into arch code the decision whether to enable virt on load.
And it has no effects on anything else but machines which have this SRSO_MSR_FIX (Zen5).
And it seems to work here - the MSR is set only when I create a VM - i.e., as expected.
Thoughts? Better ideas?
Thx.
---
arch/x86/include/asm/kvm-x86-ops.h +1 -0
@@ -16,6 +16,7 @@ BUILD_BUG_ON(1) KVM_X86_OP(check_processor_compatibility) KVM_X86_OP(enable_virtualization_cpu) KVM_X86_OP(disable_virtualization_cpu) +KVM_X86_OP_OPTIONAL(enable_virt_on_load) KVM_X86_OP(hardware_unsetup) KVM_X86_OP(has_emulated_msr) KVM_X86_OP(vcpu_after_set_cpuid)
arch/x86/include/asm/kvm_host.h +1 -0
@@ -1664,6 +1664,7 @@ struct kvm_x86_ops {int (*enable_virtualization_cpu)(void );void (*disable_virtualization_cpu)(void ); +bool (*enable_virt_on_load)(void );cpu_emergency_virt_cb *emergency_disable_virtualization_cpu;void (*hardware_unsetup)(void );
arch/x86/kvm/svm/svm.c +11 -0
@@ -693,6 +693,16 @@ static int svm_enable_virtualization_cpu(void)return 0 ; } +static bool svm_enable_virt_on_load (void ) +{ +bool ret = true; + +if (cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) + ret = false; + +return ret; +} +static void svm_cpu_uninit (int cpu) {struct svm_cpu_data *sd =per_cpu_ptr (&svm_data, cpu);
@@ -5082,6 +5092,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = { .hardware_unsetup = svm_hardware_unsetup, .enable_virtualization_cpu = svm_enable_virtualization_cpu, .disable_virtualization_cpu = svm_disable_virtualization_cpu, + .enable_virt_on_load = svm_enable_virt_on_load, .emergency_disable_virtualization_cpu = svm_emergency_disable_virtualization_cpu, .has_emulated_msr = svm_has_emulated_msr,
arch/x86/kvm/x86.c +7 -1
@@ -12576,9 +12576,15 @@ void kvm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) }EXPORT_SYMBOL_GPL (kvm_vcpu_deliver_sipi_vector); -void kvm_arch_enable_virtualization (void ) +bool kvm_arch_enable_virtualization (bool allow_arch_override) { +if (allow_arch_override) +if (!kvm_x86_call (enable_virt_on_load)()) +return false; +cpu_emergency_register_virt_callback (kvm_x86_ops.emergency_disable_virtualization_cpu); + +return true; }void kvm_arch_disable_virtualization (void )
include/linux/kvm_host.h +1 -1
@@ -1599,7 +1599,7 @@ static inline void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu) {}* kvm_usage_count, i.e. at the beginning of the generic hardware enabling * sequence, and at the end of the generic hardware disabling sequence. */ -void kvm_arch_enable_virtualization (void ); +bool kvm_arch_enable_virtualization (bool );void kvm_arch_disable_virtualization (void );/* * kvm_arch_{enable,disable}_virtualization_cpu() are called on "every" CPU to
virt/kvm/kvm_main.c +19 -14
@@ -143,8 +143,8 @@ static int kvm_no_compat_open(struct inode *inode, struct file *file)#define KVM_COMPAT (c) .compat_ioctl = kvm_no_compat_ioctl, \ .open = kvm_no_compat_open#endif -static int kvm_enable_virtualization (void ); -static void kvm_disable_virtualization (void ); +static int kvm_enable_virtualization (bool allow_arch_override); +static void kvm_disable_virtualization (bool allow_arch_override);static void kvm_io_bus_destroy (struct kvm_io_bus *bus);
@@ -1187,7 +1187,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)if (r) goto out_err_no_arch_destroy_vm; - r =kvm_enable_virtualization (); + r =kvm_enable_virtualization (false);if (r) goto out_err_no_disable;
@@ -1224,7 +1224,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)mmu_notifier_unregister (&kvm->mmu_notifier, current->mm);#endif out_err_no_mmu_notifier: -kvm_disable_virtualization (); +kvm_disable_virtualization (false); out_err_no_disable:kvm_arch_destroy_vm (kvm); out_err_no_arch_destroy_vm:
@@ -1320,7 +1320,7 @@ static void kvm_destroy_vm(struct kvm *kvm)#endif kvm_arch_free_vm (kvm);preempt_notifier_dec (); -kvm_disable_virtualization (); +kvm_disable_virtualization (false);mmdrop (mm); }
@@ -5489,9 +5489,9 @@ static DEFINE_PER_CPU(bool, virtualization_enabled);static DEFINE_MUTEX (kvm_usage_lock);static int kvm_usage_count; -__weak void kvm_arch_enable_virtualization (void ) +__weak bool kvm_arch_enable_virtualization (bool ) { - +return false; }__weak void kvm_arch_disable_virtualization (void )
@@ -5589,8 +5589,9 @@ static struct syscore_ops kvm_syscore_ops = { .shutdown = kvm_shutdown, }; -static int kvm_enable_virtualization (void ) +static int kvm_enable_virtualization (bool allow_arch_override) { +bool do_init;int r;guard (mutex)(&kvm_usage_lock);
@@ -5598,7 +5599,9 @@ static int kvm_enable_virtualization(void)if (kvm_usage_count++)return 0 ; -kvm_arch_enable_virtualization (); + do_init =kvm_arch_enable_virtualization (allow_arch_override); +if (!do_init) + goto out; r =cpuhp_setup_state (CPUHP_AP_KVM_ONLINE ,"kvm/cpu:online" , kvm_online_cpu, kvm_offline_cpu);
@@ -5631,11 +5634,13 @@ static int kvm_enable_virtualization(void)cpuhp_remove_state (CPUHP_AP_KVM_ONLINE ); err_cpuhp:kvm_arch_disable_virtualization (); + +out: --kvm_usage_count;return r; } -static void kvm_disable_virtualization (void ) +static void kvm_disable_virtualization (bool allow_arch_override) {guard (mutex)(&kvm_usage_lock);
@@ -5650,7 +5655,7 @@ static void kvm_disable_virtualization(void)static int kvm_init_virtualization (void ) {if (enable_virt_at_load) -return kvm_enable_virtualization (); +return kvm_enable_virtualization (true);return 0 ; }
@@ -5658,10 +5663,10 @@ static int kvm_init_virtualization(void)static void kvm_uninit_virtualization (void ) {if (enable_virt_at_load) -kvm_disable_virtualization (); +kvm_disable_virtualization (true); }#else /* CONFIG_KVM_GENERIC_HARDWARE_ENABLING */ -static int kvm_enable_virtualization (void ) +static int kvm_enable_virtualization (bool allow_arch_override) {return 0 ; }
@@ -5671,7 +5676,7 @@ static int kvm_init_virtualization(void)return 0 ; } -static void kvm_disable_virtualization (void ) +static void kvm_disable_virtualization (bool allow_arch_override) { }
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 8 lines · Borislav Petkov
Quoted 7 lines · Borislav Petkov
So,
in the interest of finally making some progress here I'd like to commit this below (will test it one more time just in case but it should work :-P). It is simple and straight-forward and doesn't need an IBPB when the bit gets cleared.
Quoted 8 lines
Quoted 4 lines
A potential future improvement is David's suggestion that there could be a way for tracking when the first guest gets started, we set the bit then, we make sure the bit gets set on each logical CPU when the guests migrate across the machine and when the *last* guest exists, that bit gets cleared again.
Well, that "simplicity" was short-lived:
LOL.
Quoted 1 line
Sean, how about this below?
Eww. That's quite painful, and completely disallowing enable_virt_on_load is undesirable, e.g. for use cases where the host is (almost) exclusively running VMs.
Best idea I have is to throw in the towel on getting fancy, and just maintain a dedicated count in SVM.
Alternatively, we could plumb an arch hook into kvm_create_vm() and kvm_destroy_vm() that's called when KVM adds/deletes a VM from vm_list, and key off vm_list being empty. But that adds a lot of boilerplate just to avoid a mutex+count.
I haven't tested on a system with X86_FEATURE_SRSO_BP_SPEC_REDUCE, but did verify the mechanics by inverting the flag.
--
From: Sean Christopherson <[email protected]>
Date: Wed, 30 Apr 2025 15:34:50 -0700
Subject: [PATCH] KVM: SVM: Set/clear SRSO's BP_SPEC_REDUCE on 0 <=> 1 VM count
transitions
Set the magic BP_SPEC_REDUCE bit to mitigate SRSO when running VMs if and only if KVM has at least one active VM. Leaving the bit set at all times unfortunately degrades performance by a wee bit more than expected.
Use a dedicated mutex and counter instead of hooking virtualization enablement, as changing the behavior of kvm.enable_virt_at_load based on SRSO_BP_SPEC_REDUCE is painful, and has its own drawbacks, e.g. could result in performance issues for flows that are sensity to VM creation latency.
--- arch/x86/kvm/svm/svm.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
arch/x86/kvm/svm/svm.c +33 -6
@@ -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 ; }
@@ -5032,10 +5026,42 @@ static void svm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)sev_vcpu_deliver_sipi_vector (vcpu, vector); } +static DEFINE_MUTEX (srso_lock); +static int srso_nr_vms; + +static void svm_toggle_srso_spec_reduce (void *set) +{ +if (set) +msr_set_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); +else +msr_clear_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); +} + +static void svm_srso_add_remove_vm (int count) +{ +bool set; + +if (!cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) +return ; + +guard (mutex)(&srso_lock); + + set = !srso_nr_vms; + srso_nr_vms += count; + +WARN_ON_ONCE (srso_nr_vms <0 ); +if (!set && srso_nr_vms) +return ; + +on_each_cpu (svm_toggle_srso_spec_reduce, (void *)set,1 ); +} +static void svm_vm_destroy (struct kvm *kvm) {avic_vm_destroy (kvm);sev_vm_destroy (kvm); + +svm_srso_add_remove_vm (-1 ); }static int svm_vm_init (struct kvm *kvm)
@@ -5061,6 +5087,7 @@ static int svm_vm_init(struct kvm *kvm)return ret; } +svm_srso_add_remove_vm (1 );return 0 ; }
base-commit: f158e1b145f73aae1d3b7e756eb129a15b2b7a90 --
Re: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 26 lines · Sean Christopherson
Quoted 8 lines · Borislav Petkov
Quoted 7 lines · Borislav Petkov
So,
in the interest of finally making some progress here I'd like to commit this below (will test it one more time just in case but it should work :-P). It is simple and straight-forward and doesn't need an IBPB when the bit gets cleared.
Quoted 7 lines
Quoted 4 lines
A potential future improvement is David's suggestion that there could be a way for tracking when the first guest gets started, we set the bit then, we make sure the bit gets set on each logical CPU when the guests migrate across the machine and when the *last* guest exists, that bit gets cleared again.
Well, that "simplicity" was short-lived:
LOL.
Quoted 1 line
Sean, how about this below?
Eww. That's quite painful, and completely disallowing
enable_virt_on_loadis undesirable, e.g. for use cases where the host is (almost) exclusively running VMs.Best idea I have is to throw in the towel on getting fancy, and just maintain a dedicated count in SVM.
Quoted 6 lines
Alternatively, we could plumb an arch hook into
kvm_create_vm()andkvm_destroy_vm()that's called when KVM adds/deletes a VM fromvm_list, and key offvm_listbeing empty. But that adds a lot of boilerplate just to avoid a mutex+count.I haven't tested on a system with
X86_FEATURE_SRSO_BP_SPEC_REDUCE, but did verify the mechanics by inverting the flag.
Testing this patch on the same EPYC Turin server as my original tests, I can confirm that on a clean boot without any VMs running, the performance is back to where it was on v6.14. :)
Thanks,
Michael
Quoted 103 lines
--
From: Sean Christopherson <[email protected]>
Date: Wed, 30 Apr 2025 15:34:50 -0700
Subject: [PATCH] KVM: SVM: Set/clear SRSO'sBP_SPEC_REDUCEon 0 <=> 1 VM count
transitionsSet the magic
BP_SPEC_REDUCEbit to mitigate SRSO when running VMs if and only if KVM has at least one active VM. Leaving the bit set at all times unfortunately degrades performance by a wee bit more than expected.Use a dedicated mutex and counter instead of hooking virtualization enablement, as changing the behavior of kvm.enable_virt_at_load based on
SRSO_BP_SPEC_REDUCEis painful, and has its own drawbacks, e.g. could result in performance issues for flows that are sensity to VM creation latency.Fixes 8442df2b49ed ("x86/bugs: KVM: Add support for SRSO_MSR_FIX")Reported-by Michael Larabel <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/svm/svm.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)arch/x86/kvm/svm/svm.c +33
-6@@ -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 ; }@@ -5032,10 +5026,42 @@ static void svm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)sev_vcpu_deliver_sipi_vector (vcpu, vector); } +static DEFINE_MUTEX (srso_lock); +static int srso_nr_vms; + +static void svm_toggle_srso_spec_reduce (void *set) +{ +if (set) +msr_set_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); +else +msr_clear_bit (MSR_ZEN4_BP_CFG ,MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT ); +} + +static void svm_srso_add_remove_vm (int count) +{ +bool set; + +if (!cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) +return ; + +guard (mutex)(&srso_lock); + + set = !srso_nr_vms; + srso_nr_vms += count; + +WARN_ON_ONCE (srso_nr_vms <0 ); +if (!set && srso_nr_vms) +return ; + +on_each_cpu (svm_toggle_srso_spec_reduce, (void *)set,1 ); +} +static void svm_vm_destroy (struct kvm *kvm) {avic_vm_destroy (kvm);sev_vm_destroy (kvm); + +svm_srso_add_remove_vm (-1 ); }static int svm_vm_init (struct kvm *kvm)@@ -5061,6 +5087,7 @@ static int svm_vm_init(struct kvm *kvm)return ret; } +svm_srso_add_remove_vm (1 );return 0 ; }base-commit:
f158e1b145f73aae1d3b7e756eb129a15b2b7a90--
Borislav Petkov
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
In reply to Sean ChristophersonQuoted 3 lines · Sean Christopherson
Eww. That's quite painful, and completely disallowing
enable_virt_on_loadis undesirable, e.g. for use cases where the host is (almost) exclusively running VMs.
I wanted to stay generic... :-)
Quoted 6 lines
Best idea I have is to throw in the towel on getting fancy, and just maintain a dedicated count in SVM.
Alternatively, we could plumb an arch hook into
kvm_create_vm()andkvm_destroy_vm()that's called when KVM adds/deletes a VM fromvm_list, and key offvm_listbeing empty. But that adds a lot of boilerplate just to avoid a mutex+count.
FWIW, that was Tom's idea.
Quoted 5 lines
arch/x86/kvm/svm/svm.c +5
-0+static void svm_srso_add_remove_vm (int count) +{ +bool set; + +if (!cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE ))
arch/x86/kvm/svm/svm.c static void svm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) · line 5054
Quoted 10 lines
arch/x86/kvm/svm/svm.c +10
-0+return ; + +guard (mutex)(&srso_lock); + + set = !srso_nr_vms; + srso_nr_vms += count; + +WARN_ON_ONCE (srso_nr_vms <0 ); +if (!set && srso_nr_vms) +return ;
So instead of doing this "by-foot", I would've used any of those atomic_inc_return() and atomic_dec_and_test() and act upon the value when it becomes 0 or !0 instead of passing 1 and -1. Because the count is kinda implicit...
But yeah, not a biggie - that solves the issue too.
Thanks!
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 7 lines · Sean Christopherson
Quoted 3 lines
Eww. That's quite painful, and completely disallowing
enable_virt_on_loadis undesirable, e.g. for use cases where the host is (almost) exclusively running VMs.I wanted to stay generic... :-)
Quoted 8 lines
Quoted 6 lines
Best idea I have is to throw in the towel on getting fancy, and just maintain a dedicated count in SVM.
Alternatively, we could plumb an arch hook into
kvm_create_vm()andkvm_destroy_vm()that's called when KVM adds/deletes a VM fromvm_list, and key offvm_listbeing empty. But that adds a lot of boilerplate just to avoid a mutex+count.FWIW, that was Tom's idea.
FWIW, this could be helpful for ASI as well going forward, i.e. the set of ASI driven mitigations could be different whether there are VMs on a system or not, because the attack vectors are different.
So, having a first class and properly defined mechanisms to know if there are effectively VMs running on a system would be generically convenient.
But maybe that's something we can work on later on?
Best,
Patrick
Sean Christopherson
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
In reply to Borislav PetkovQuoted 13 lines · Borislav Petkov
Quoted 12 lines · Sean Christopherson
arch/x86/kvm/svm/svm.c +12
-0+static void svm_srso_add_remove_vm (int count) +{ +bool set; + +if (!cpu_feature_enabled (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) +return ; + +guard (mutex)(&srso_lock); + + set = !srso_nr_vms; + srso_nr_vms += count; +
Quoted 8 lines
arch/x86/kvm/svm/svm.c static void svm_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector) · line 5054
Quoted 3 lines
arch/x86/kvm/svm/svm.c +3
-0+WARN_ON_ONCE (srso_nr_vms <0 ); +if (!set && srso_nr_vms) +return ;So instead of doing this "by-foot", I would've used any of those
atomic_inc_return()andatomic_dec_and_test()and act upon the value when it becomes 0 or !0 instead of passing 1 and -1. Because the count is kinda implicit...
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 for very little gain. In practice, when srso_nr_vms >= 1, CPUs will hold the lock for only a handful of cycles. The latency of VM creation is somewhat important, but it's certainly not that latency sensitive, e.g. KVM already serializes VM creation on kvm_lock (which is a mutex) in order to add VMs to vm_list.
The only truly slow path is the 0<=>1 transitions, and those *must* be slow to get the ordering correct.
One thing I can/will do is change the mutex into a spinlock, so that on non-RT systems there's zero chance of VM creation getting bogged down because a task happens to get preempted at just the wrong time. I was thinking it's illegal to use on_each_cpu() in a spinlock, but that's fine; it's using it with IRQs disabled that's problematic.
I'll post a proper patch with the spinlock and CONFIG_CPU_MITIGATIONS #ifdef, assuming testing comes back clean.
As for all the problematic scenarios... If KVM increments the counter before sending IPIs, then reacing VM creation can result in the second VM skipping the mutex and doing KVM_RUN with BP_SPEC_REDUCE=0.
VMs MSR CPU-0 CPU-1
-----------------------------
0 0 CREATE 0 0 lock() 1 0 inc() 1 0 CREATE 2 0 inc() 2 0 KVM_RUN :-( 2 0 IPI 2 1 WRMSR WRMSR
But simply incrementing the count after sending IPIs obviously doesn't work.
VMs MSR CPU-0 CPU-1
-----------------------------
0 0 CREATE 0 0 lock() 0 0 IPI 0 0 WRMSR WRMSR 1 0 inc() 1 0 KVM_RUN :-(
And passing in set/clear (or using separate IPI handlers) doesn't handle the case where the IPI from destroy arrives after the IPI from create.
VMs MSR CPU-0 CPU-1
-----------------------------
1 1 DESTROY 0 1 CREATE dec() 0 1 lock() 0 1 IPI(1) 0 1 WRMSR WRMSR 0 1 IPI(0) 0 0 WRMSR WRMSR 1 0 inc() 1 0 KVM_RUN :-(
Addressing that by adding a global flag to track that SRSO needs to be set almost works, but there's still a race with a destroy IPI if the callback only checks the "set" flag.
VMs MSR CPU-0 CPU-1
-----------------------------
1 1 DESTROY 0 1 CREATE dec() 0 1 lock() 0 1 set=1 0 1 IPI 0 1 WRMSR WRMSR 1 0 inc() 1 1 set=0 1 1 IPI 1 0 WRMSR WRMSR 1 0 KVM_RUN :-(
To address all of those, I ended up with the below. It's not actually that much code, but amount of documentation needed to explain everything is ugly.
#ifndef CONFIG_CPU_MITIGATIONS
static DEFINE_MUTEX(srso_add_vm_lock);
static atomic_t srso_nr_vms;
static bool srso_set;
static void svm_toggle_srso_spec_reduce(void *ign)
{
/*
* Read both srso_set and the count (and don't pass in set/clear!) so
* that BP_SPEC_REDUCE isn't incorrectly cleared if IPIs from destroying
* he last VM arrive after IPIs from creating the first VM (in the new
* "generation").
*/
if (READ_ONCE(srso_set) || atomic_read(&srso_nr_vms))
msr_set_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
else
msr_clear_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
}static void svm_srso_vm_init(void)
{
if (!cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
return;
/*
* Acquire the mutex on a 0=>1 transition to ensure BP_SPEC_REDUCE is
* set before any VM is fully created.
*/
if (atomic_inc_not_zero(&srso_nr_vms))
return;
guard(mutex)(&srso_add_vm_lock);
/*
* Re-check the count before sending IPIs, only the first task needs to
* toggle BP_SPEC_REDUCE, other tasks just need to wait. For the 0=>1
* case, update the count *after* BP_SPEC_REDUCE is set on all CPUs to
* ensure creating multiple VMs concurrently doesn't result in a task
* skipping the mutex before BP_SPEC_REDUCE is set.
*
* Atomically increment the count in all cases as the mutex only guards
* 0=>1 transitions, e.g. another task can decrement the count if a VM
* was created (0=>1) *and* destroyed (1=>0) between observing a count
* of '0' and acquiring the mutex, and another task can increment the
* count if the count is already >= 1.
*/
if (!atomic_inc_not_zero(&srso_nr_vms)) {
WRITE_ONCE(srso_set, true);
on_each_cpu(svm_toggle_srso_spec_reduce, NULL, 1);
atomic_inc(&srso_nr_vms);
smp_mb__after_atomic();
WRITE_ONCE(srso_set, false);
}
}static void svm_srso_vm_destroy(void)
{
if (!cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
return;
/*
* If the last VM is being destroyed, clear BP_SPEC_REDUCE on all CPUs.
* Unlike the creation case, there is no need to wait on other CPUs as
* running code with BP_SPEC_REDUCE=1 is always safe, KVM just needs to
* ensure guest code never runs with BP_SPEC_REDUCE=0.
*/
if (atomic_dec_and_test(&srso_nr_vms))
on_each_cpu(svm_toggle_srso_spec_reduce, NULL, 0);
}#else
static void svm_srso_vm_init(void) { }
static void svm_srso_vm_destroy(void) { }
#endif /* CONFIG_CPU_MITIGATIONS */
Borislav Petkov
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 10 lines · Sean Christopherson
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.
You'd still need to do an IPI on VMEXIT when VM count does 1=>0 but that's easy.
Dunno, there probably already is a per-CPU setting in KVM so you could add that to it...
Anyway, something along those lines...
Thx.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteRE: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 33 lines
-----Original Message-----
From: Borislav Petkov <[email protected]>
Sent: Monday, May 5, 2025 10:26 AM
To: Sean Christopherson <[email protected]>
Cc: Yosry Ahmed <[email protected]>; Patrick Bellasi <[email protected]>; Paolo Bonzini <[email protected]>; Josh Poimboeuf <[email protected]>; Pawan Gupta <[email protected]>; [email protected]; [email protected]; [email protected]; Patrick Bellasi <[email protected]>; Brendan Jackman <[email protected]>; Kaplan, David <[email protected]>; Michael Larabel <[email protected]>
Subject: Re: x86/bugs: KVM: Add support forSRSO_MSR_FIX, back for moarCaution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
Quoted 3 lines · Sean Christopherson
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.
Quoted 4 lines
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.
Quoted 4 lines
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. :-\
Quoted 8 lines
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_REDUCEis 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.
Quoted 15 lines
You'd still need to do an IPI on VMEXIT when VM count does 1=>0 but that's easy.
Dunno, there probably already is a per-CPU setting in KVM so you could add that to it...
Anyway, something along those lines...
Thx.
--
Regards/Gruss,
Boris.
Borislav Petkov
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 6 lines · David
Almost. My thought was that
kvm_runcould do something like:If (!this_cpu_read(bp_spec_reduce_is_set)) {
wrmsrl to setBP_SEC_REDUCE
this_cpu_write(bp_spec_reduce_is_set, 1)
}
That's what I meant too. :-)
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteSean Christopherson
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
In reply to Kaplan, DavidQuoted 28 lines · David Kaplan
Quoted 17 lines · Sean Christopherson
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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. :-\
Quoted 8 lines
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_REDUCEis 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_runcould do something like:
Quoted 8 lines
If (!this_cpu_read(bp_spec_reduce_is_set)) {
wrmsrl to setBP_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(-)
arch/x86/include/asm/msr-index.h +1 -1
@@ -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 */
arch/x86/kvm/svm/svm.c +19 -7
@@ -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 );
arch/x86/kvm/x86.h +1 -0
@@ -51,6 +51,7 @@ struct kvm_host_values {u64 xcr0;u64 xss;u64 arch_capabilities; +u64 zen4_bp_cfg; };void kvm_spurious_fault (void );
arch/x86/lib/msr.c +0 -2
@@ -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
RE: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 80 lines
-----Original Message-----
From: Sean Christopherson <[email protected]>
Sent: Monday, May 5, 2025 11:30 AM
To: Kaplan, David <[email protected]>
Cc: Borislav Petkov <[email protected]>; Yosry Ahmed <[email protected]>; Patrick Bellasi <[email protected]>; Paolo Bonzini <[email protected]>; Josh Poimboeuf <[email protected]>; Pawan Gupta <[email protected]>; [email protected]; [email protected]; [email protected]; Patrick Bellasi <[email protected]>; Brendan Jackman <[email protected]>; Michael Larabel <[email protected]>
Subject: Re: x86/bugs: KVM: Add support forSRSO_MSR_FIX, back for moarCaution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
Quoted 31 lines · David Kaplan
Quoted 20 lines · Sean Christopherson
Quoted 4 lines
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.
Quoted 6 lines
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 isexecuted with BP_SPEC_REDUCE=0.
Quoted 4 lines
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. :-\
Quoted 8 lines
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_REDUCEis 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_runcould do something like:Quoted 7 lines
If (!this_cpu_read(bp_spec_reduce_is_set)) {
wrmsrl to setBP_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()andsvm_prepare_host_switch().switch_to_guest()is called when KVM is about to do VMRUN, andhost_switch()is called when the vCPU is put, i.e. when the task is scheduled out or whenKVM_RUNexits to userspace.The existing
svm->guest_state_loadedguard 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 withBP_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_REDUCEon- demand:: You want to IBPB before clearing the MSR as otherwise host kernel will be : running with the mistrained gunk from the guest.
Quoted 8 lines
[*]
https://lore.kernel.org/all/20250217160728.GFZ7NewJHpMaWdiX2M@fat_crate.loc
alAssuming that's the case...
Compile-tested only. If this looks/sounds sane, I'll test the mechanics and write a changelog.
I'm having trouble following the patch...where do you clear the MSR bit?
I thought a per-cpu "cache" of the MSR bit might be good to avoid having to issue slow RDMSRs, if these paths are 'hot'. I don't know if that's the case or not.
Also keep in mind this is a shared (per-core) MSR bit. You can't just clear it when you exit one guest because the sibling thread might be running another guest.
So I think you still want the global count of VMs. You clear the MSR bit when that count drops to 0. But you can set the bit on your core before you enter guest mode if needed.
There is still a race condition I suppose where the count drops to 0, and you go to send the IPI to clear the MSR bit, but in the meantime somebody else starts a new VM. However that probably works naturally because in order to process the IPI you exit guest mode and on the next VMRUN, you'd check and see your MSR bit isn't set and go and set it.
--David Kaplan
Quoted 117 lines
--- 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(-)arch/x86/include/asm/msr-index.h +1
-1@@ -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 */ arch/x86/kvm/svm/svm.c index renamed from arch/x86/kvm/svm/svm.c +0
-0
cc1c721ba067..2d87ec216811100644arch/x86/kvm/svm/svm.c +0
-3@@ -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)arch/x86/kvm/svm/svm.c +14
-4@@ -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__initintsvm_hardware_setup(void)init_msrpm_offsets();arch/x86/kvm/svm/svm.c +3
-0+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);arch/x86/kvm/svm/svm.c +2
-0+ } + kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |XFEATURE_MASK_BNDCSR );arch/x86/kvm/x86.h index renamed from arch/x86/kvm/x86.h +0
-0
88a9475899c8..629eae9e4f59100644arch/x86/kvm/x86.h +1
-0@@ -51,6 +51,7 @@ struct kvm_host_values {u64 xcr0;u64 xss;u64 arch_capabilities; +u64 zen4_bp_cfg; };void kvm_spurious_fault (void );arch/x86/lib/msr.c index renamed from arch/x86/lib/msr.c +0
-0
5a18ecc04a6c..4bf4fad5b148100644arch/x86/lib/msr.c +0
-0@@ -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--
Sean Christopherson
maintainerRe: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
Quoted 37 lines · David Kaplan
Quoted 37 lines
Quoted 9 lines
Almost. My thought was that
kvm_runcould do something like:If (!this_cpu_read(bp_spec_reduce_is_set)) {
wrmsrl to setBP_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()andsvm_prepare_host_switch().switch_to_guest()is called when KVM is about to do VMRUN, andhost_switch()is called when the vCPU is put, i.e. when the task is scheduled out or whenKVM_RUNexits to userspace.The existing
svm->guest_state_loadedguard 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 withBP_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_REDUCEon- 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.loc
al
Quoted 6 lines
Quoted 4 lines
Assuming that's the case...
Compile-tested only. If this looks/sounds sane, I'll test the mechanics and write a changelog.
I'm having trouble following the patch...where do you clear the MSR bit?
Gah, the rdmsrl() in svm_prepare_host_switch() should be a wrmsrl().
Quoted 3 lines
I thought a per-cpu "cache" of the MSR bit might be good to avoid having to issue slow RDMSRs, if these paths are 'hot'. I don't know if that's the case or not.
Oh, you were only proposing deferring setting BP_SPEC_REDUCE. I like it. That avoids setting BP_SPEC_REDUCE on CPUs that aren't running VMs, e.g. housekeeping CPUs, and makes it a heck of a lot easier to elide the lock on 1<=>N transitions.
I posted v2 using that approach (when lore catches up):
https://lore.kernel.org/all/[email protected]
Quoted 3 lines
Also keep in mind this is a shared (per-core) MSR bit. You can't just clear it when you exit one guest because the sibling thread might be running another guest.
Heh, well then never mind. I'm not touching SMT coordination.
Quoted 10 lines
arch/x86/kvm/svm/svm.c static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu) · line 1563
Quoted 10 lines
static void
svm_prepare_host_switch(struct kvm_vcpu *vcpu){arch/x86/kvm/svm/svm.c +8
-1-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);
As above, this is supposed to be wrmsrl().
Quoted 3 lines
Quoted 3 lines
arch/x86/kvm/svm/svm.c +2
-0+ } + svm->guest_state_loaded = false; }
RE: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
[AMD Official Use Only - AMD Internal Distribution Only]
Quoted 74 lines
-----Original Message-----
From: Sean Christopherson <[email protected]>
Sent: Monday, May 5, 2025 1:04 PM
To: Kaplan, David <[email protected]>
Cc: Borislav Petkov <[email protected]>; Yosry Ahmed <[email protected]>; Patrick Bellasi <[email protected]>; Paolo Bonzini <[email protected]>; Josh Poimboeuf <[email protected]>; Pawan Gupta <[email protected]>; [email protected]; [email protected]; [email protected]; Patrick Bellasi <[email protected]>; Brendan Jackman <[email protected]>; Michael Larabel <[email protected]>
Subject: Re: x86/bugs: KVM: Add support forSRSO_MSR_FIX, back for moarCaution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
Quoted 24 lines · David Kaplan
Quoted 24 lines
Quoted 9 lines
Almost. My thought was that
kvm_runcould do something like:If (!this_cpu_read(bp_spec_reduce_is_set)) {
wrmsrl to setBP_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()andsvm_prepare_host_switch().switch_to_guest()is called when KVM is about to do VMRUN, andhost_switch()is called when the vCPU is put, i.e. when the task is scheduled out or whenKVM_RUNexits to userspace.Quoted 4 lines
Quoted 4 lines
The existing
svm->guest_state_loadedguard 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 withBP_SPEC_REDUCE, e.g. if #NPF triggers swap-in, an IRQ arrives while handling the exit, etc., but that's all finefrom a security perspective.
Quoted 14 lines
Quoted 14 lines
IIUC, per Boris[*] an IBPB is needed when toggling
BP_SPEC_REDUCEon-
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_cr
ate.loc
alQuoted 6 lines
Quoted 4 lines
Assuming that's the case...
Compile-tested only. If this looks/sounds sane, I'll test the mechanics and write a changelog.
I'm having trouble following the patch...where do you clear the MSR bit?
Gah, the
rdmsrl()insvm_prepare_host_switch()should be awrmsrl().Quoted 3 lines
I thought a per-cpu "cache" of the MSR bit might be good to avoid having to issue slow RDMSRs, if these paths are 'hot'. I don't know if that's the case or not.
Quoted 9 lines
Oh, you were only proposing deferring setting
BP_SPEC_REDUCE. I like it. That avoids settingBP_SPEC_REDUCEon CPUs that aren't running VMs, e.g. housekeeping CPUs, and makes it a heck of a lot easier to elide the lock on 1<=>N transitions.I posted v2 using that approach (when lore catches up):
LGTM.
Thanks --David Kaplan
PATCH v2 4/4 Documentation/kernel-parameters: Fix a typo in kvm.enable_virt_at_load text
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
s/lode/load/
--- Documentation/admin-guide/kernel-parameters.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/admin-guide/kernel-parameters.txt +1 -1
@@ -2695,7 +2695,7 @@ VMs, i.e. on the 0=>1 and 1=>0 transitions of the number of VMs. - Enabling virtualization at module lode avoids potential + Enabling virtualization at module load avoids potential latency for creation of the 0=>1 VM, as KVM serializes virtualization enabling across all online CPUs. The "cost" of enabling virtualization when KVM is loaded,
Signature · 1 line
2.43.0
[tip: x86/cleanups] Documentation/kernel-parameters: Fix a typo in kvm.enable_virt_at_load text
The following commit has been merged into the x86/cleanups branch of tip:
The following commit has been merged into the x86/cleanups branch of tip:
Commit-ID: 1146f7429f610d51b886402f1f7a43faa08d814a Gitweb: https://git.kernel.org/tip/1146f7429f610d51b886402f1f7a43faa08d814a Author: Borislav Petkov (AMD) <[email protected]> AuthorDate: Mon, 02 Dec 2024 13:04:16 +01:00 Committer: Borislav Petkov (AMD) <[email protected]> CommitterDate: Mon, 30 Dec 2024 17:58:58 +01:00
Documentation/kernel-parameters: Fix a typo in kvm.enable_virt_at_load text
s/lode/load/
--- Documentation/admin-guide/kernel-parameters.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/admin-guide/kernel-parameters.txt +1 -1
@@ -2769,7 +2769,7 @@ VMs, i.e. on the 0=>1 and 1=>0 transitions of the number of VMs. - Enabling virtualization at module lode avoids potential + Enabling virtualization at module load avoids potential latency for creation of the 0=>1 VM, as KVM serializes virtualization enabling across all online CPUs. The "cost" of enabling virtualization when KVM is loaded,
Quoted 10 lines · Borislav Petkov
From: "Borislav Petkov (AMD)" <[email protected]>
Hi,
here's the next revision, with hopefully all review feedback addressed.
Changelog:
v1:
The series is pretty self-explanatory. So:
Quoted 23 lines
Thx.
Borislav Petkov (AMD) (4):
x86/bugs: AddSRSO_USER_KERNEL_NOsupport
KVM: x86: AdvertiseSRSO_USER_KERNEL_NOto userspace x86/bugs: KVM: Add support forSRSO_MSR_FIXDocumentation/kernel-parameters: Fix a typo in kvm.enable_virt_at_load
textDocumentation/admin-guide/hw-vuln/srso.rst | 10 ++++++++++ Documentation/admin-guide/kernel-parameters.txt | 2 +- arch/x86/include/asm/cpufeatures.h | 2 ++ arch/x86/include/asm/msr-index.h | 1 + arch/x86/kernel/cpu/bugs.c | 16 +++++++++++++++- arch/x86/kernel/cpu/common.c | 1 + arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/svm/svm.c | 6 ++++++ arch/x86/lib/msr.c | 2 ++ 9 files changed, 39 insertions(+), 3 deletions(-)base-commit:
40384c840ea1944d7c5a392e8975ed088ecf0b37
[tip: x86/bugs] x86/bugs: KVM: Add support for SRSO_MSR_FIX
In reply to Borislav PetkovThe following commit has been merged into the x86/bugs branch of tip:
The following commit has been merged into the x86/bugs branch of tip:
Commit-ID: 8442df2b49ed9bcd67833ad4f091d15ac91efd00 Gitweb: https://git.kernel.org/tip/8442df2b49ed9bcd67833ad4f091d15ac91efd00 Author: Borislav Petkov <[email protected]> AuthorDate: Tue, 18 Feb 2025 12:13:33 +01:00 Committer: Borislav Petkov (AMD) <[email protected]> CommitterDate: Wed, 26 Feb 2025 15:13:06 +01:00
x86/bugs: KVM: Add support for SRSO_MSR_FIX
Add support for
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 BpSpecReduce to mitigate SRSO across guest/host boundaries.
Switch back to enabling the bit when virtualization is enabled and to clear the bit when virtualization is disabled because using a MSR slot would clear the bit when the guest is exited and any training the guest has done, would potentially influence the host kernel when execution enters the kernel and hasn't VMRUN the guest yet.
More detail on the public thread in Link below.
--- Documentation/admin-guide/hw-vuln/srso.rst | 13 +++++++++++- arch/x86/include/asm/cpufeatures.h | 4 ++++- arch/x86/include/asm/msr-index.h | 1 +- arch/x86/kernel/cpu/bugs.c | 24 +++++++++++++++++---- arch/x86/kvm/svm/svm.c | 6 +++++- arch/x86/lib/msr.c | 2 ++- 6 files changed, 46 insertions(+), 4 deletions(-)
Documentation/admin-guide/hw-vuln/srso.rst +13 -0
@@ -104,7 +104,20 @@ The possible values in this file are: (spec_rstack_overflow=ibpb-vmexit) + * 'Mitigation: Reduced Speculation': + This mitigation gets automatically enabled when the above one "IBPB on + VMEXIT" has been selected and the CPU supports the BpSpecReduce bit. + + It gets automatically enabled on machines which have the + SRSO_USER_KERNEL_NO=1 CPUID bit. In that case, the code logic is to switch + to the above =ibpb-vmexit mitigation because the user/kernel boundary is + not affected anymore and thus "safe RET" is not needed. + + After enabling the IBPB on VMEXIT mitigation option, the BpSpecReduce bit + is detected (functionality present on all such machines) and that + practically overrides IBPB on VMEXIT as it has a lot less performance + impact and takes care of the guest->host attack vector too. In order to exploit vulnerability, an attacker needs to:
arch/x86/include/asm/cpufeatures.h +4 -0
@@ -468,6 +468,10 @@#define X86_FEATURE_IBPB_BRTYPE (20*32+28)/* MSR_PRED_CMD[IBPB] flushes all branch type predictions */ #define X86_FEATURE_SRSO_NO (20*32+29)/* CPU is not affected by SRSO */ #define X86_FEATURE_SRSO_USER_KERNEL_NO (20*32+30)/* CPU is not affected by SRSO across user/kernel boundaries */ +#define X86_FEATURE_SRSO_BP_SPEC_REDUCE (20*32+31)/* +* BP_CFG[BpSpecReduce] can be used to mitigate SRSO for VMs. +* (SRSO_MSR_FIX in the official doc). +*/ /* * Extended auxiliary flags: Linux defined - for features scattered in various
arch/x86/include/asm/msr-index.h +1 -0
@@ -721,6 +721,7 @@/* Zen4 */ #define MSR_ZEN4_BP_CFG 0xc001102e +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4#define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5/* Fam 19h MSRs */
arch/x86/kernel/cpu/bugs.c +20 -4
@@ -2522,6 +2522,7 @@ enum srso_mitigation {SRSO_MITIGATION_SAFE_RET ,SRSO_MITIGATION_IBPB ,SRSO_MITIGATION_IBPB_ON_VMEXIT , + SRSO_MITIGATION_BP_SPEC_REDUCE, }; enum srso_mitigation_cmd {
@@ -2539,7 +2540,8 @@ static const char * const srso_strings[] = { [SRSO_MITIGATION_MICROCODE] = "Vulnerable: Microcode, no safe RET", [SRSO_MITIGATION_SAFE_RET] = "Mitigation: Safe RET", [SRSO_MITIGATION_IBPB] = "Mitigation: IBPB", - [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only" + [SRSO_MITIGATION_IBPB_ON_VMEXIT] = "Mitigation: IBPB on VMEXIT only", + [SRSO_MITIGATION_BP_SPEC_REDUCE] = "Mitigation: Reduced Speculation" };static enum srso_mitigation srso_mitigation__ro_after_init =SRSO_MITIGATION_NONE ;
@@ -2578,7 +2580,7 @@ static void __init srso_select_mitigation(void) srso_cmd == SRSO_CMD_OFF) {if (boot_cpu_has (X86_FEATURE_SBPB )) x86_pred_cmd =PRED_CMD_SBPB ; -return ; + goto out; }if (has_microcode) {
@@ -2590,7 +2592,7 @@ static void __init srso_select_mitigation(void)*/ if (boot_cpu_data.x86 <0x19 && !cpu_smt_possible ()) {setup_force_cpu_cap (X86_FEATURE_SRSO_NO ); -return ; + goto out; }if (retbleed_mitigation ==RETBLEED_MITIGATION_IBPB ) {
@@ -2670,6 +2672,12 @@ static void __init srso_select_mitigation(void) ibpb_on_vmexit:case SRSO_CMD_IBPB_ON_VMEXIT : +if (boot_cpu_has (X86_FEATURE_SRSO_BP_SPEC_REDUCE )) { +pr_notice ("Reducing speculation to address VM/HV SRSO attack vector.\n" ); + srso_mitigation =SRSO_MITIGATION_BP_SPEC_REDUCE ; +break ; + } +if (IS_ENABLED (CONFIG_MITIGATION_IBPB_ENTRY )) {if (has_microcode) {setup_force_cpu_cap (X86_FEATURE_IBPB_ON_VMEXIT );
@@ -2691,7 +2699,15 @@ ibpb_on_vmexit: } out: -pr_info ("%s\n" , srso_strings[srso_mitigation]); +/* +* Clear the feature flag if this mitigation is not selected as that +* feature flag controls the BpSpecReduce MSR bit toggling in KVM. +*/ +if (srso_mitigation !=SRSO_MITIGATION_BP_SPEC_REDUCE ) +setup_clear_cpu_cap (X86_FEATURE_SRSO_BP_SPEC_REDUCE ); + +if (srso_mitigation !=SRSO_MITIGATION_NONE ) +pr_info ("%s\n" , srso_strings[srso_mitigation]); }#undef pr_fmt
arch/x86/kvm/svm/svm.c +6 -0
@@ -607,6 +607,9 @@ 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 )
@@ -684,6 +687,9 @@ 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 ; }
arch/x86/lib/msr.c +2 -0
@@ -103,6 +103,7 @@ 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.
@@ -118,6 +119,7 @@ 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)