Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Patrick Bellasi]

> On Tue, Feb 18, 2025 at 02:42:57PM +0000, Patrick Bellasi wrote:
> > 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?
> 
> 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 <derkling@google.com>
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/

Signed-off-by: Patrick Bellasi <derkling@google.com>
---
 arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 7fafd98368b91..2d785b2ca4e2e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -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

[Yosry Ahmed]

On Wed, Feb 26, 2025 at 06:45:40PM +0000, Patrick Bellasi wrote:
> > On Tue, Feb 18, 2025 at 02:42:57PM +0000, Patrick Bellasi wrote:
> > > 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?
> > 
> > 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 <derkling@google.com>
> 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.

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

> 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 <derkling@google.com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 7fafd98368b91..2d785b2ca4e2e 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -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:

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.

> +			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.

> +			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

[Borislav Petkov]

On Wed, Feb 26, 2025 at 07:51:08PM +0000, Yosry Ahmed wrote:
> 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.

> On the other hand we are enlighting ASI to skip mitigating SRSO if
> X86_FEATURE_SRSO_BP_SPEC_REDUCE is enabled

Yap, makes sense.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Wed, Feb 26, 2025 at 06:45:40PM +0000, Patrick Bellasi wrote:
> +
> +	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.

> +			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...?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Patrick Bellasi]

> On Wed, Feb 26, 2025 at 06:45:40PM +0000, Patrick Bellasi wrote:
> > +
> > +	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.

Good point, the above can certainly be dropped.

> > +			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?

> 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.

> 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@)

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Mon, Mar 03, 2025 at 03:05:56PM +0000, Patrick Bellasi wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Brendan Jackman]

On Tue, Mar 11, 2025 at 01:03:40PM +0100, Borislav Petkov wrote:
> On Mon, Mar 03, 2025 at 03:05:56PM +0000, Patrick Bellasi wrote:
> > 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?

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/20250110-asi-rfc-v2-v2-4-8419288bc805@google.com/

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!

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Tue, Mar 11, 2025 at 01:36:54PM +0000, Brendan Jackman wrote:
> 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...

> 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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

+ bjackman.

On Thu, Feb 13, 2025 at 01:44:08PM +0000, Patrick Bellasi wrote:
> 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)" <bp@alien8.de>
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.

Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 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(-)

diff --git a/Documentation/admin-guide/hw-vuln/srso.rst b/Documentation/admin-guide/hw-vuln/srso.rst
index 2ad1c05b8c88..b856538083a2 100644
--- a/Documentation/admin-guide/hw-vuln/srso.rst
+++ b/Documentation/admin-guide/hw-vuln/srso.rst
@@ -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:
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 508c0dad116b..43653f2704c9 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -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
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 72765b2fe0d8..d35519b337ba 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index a5d0998d7604..1d7afc40f227 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -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
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7640a84e554a..6ea3632af580 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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);
 
-- 
2.43.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Patrick Bellasi]

> On Thu, Feb 13, 2025 at 01:44:08PM +0000, Patrick Bellasi wrote:
> > 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.

> 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.

> + * '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.

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

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

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Thu, Feb 13, 2025 at 05:50:57PM +0000, Patrick Bellasi wrote:
> 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?

That's fine, we should update that paper.

> 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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Fri, Feb 14, 2025 at 09:10:05PM +0100, Borislav Petkov wrote:
> 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?

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 6ea3632af580..dcc4e5935b82 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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);
 
-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Yosry Ahmed]

On Sat, Feb 15, 2025 at 01:53:07PM +0100, Borislav Petkov wrote:
> On Fri, Feb 14, 2025 at 09:10:05PM +0100, Borislav Petkov wrote:
> > 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?


> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6ea3632af580..dcc4e5935b82 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -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);
>  
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Sun, Feb 16, 2025 at 09:59:59PM -0800, Yosry Ahmed wrote:
> 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.

> - 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.

> Any thoughts? 

Yes, let's keep it simple and do anything more involved *only* if it is really
necessary.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Yosry Ahmed]

On Mon, Feb 17, 2025 at 05:07:28PM +0100, Borislav Petkov wrote:
> On Sun, Feb 16, 2025 at 09:59:59PM -0800, Yosry Ahmed wrote:
> > 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.
> 
> > - 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.

> 
> > Any thoughts? 
> 
> Yes, let's keep it simple and do anything more involved *only* if it is really
> necessary.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Re: Re: Re: Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Mon, Feb 17, 2025 at 11:56:22AM -0800, Yosry Ahmed wrote:
> 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."

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Yosry Ahmed]

February 17, 2025 at 12:20 PM, "Borislav Petkov" <bp@alien8.de> wrote:
> 
> On Mon, Feb 17, 2025 at 11:56:22AM -0800, Yosry Ahmed wrote:
> 
> > 
> > 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.

[PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[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.

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.

Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20241202120416.6054-1-bp@kernel.org
---
 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(-)

diff --git a/Documentation/admin-guide/hw-vuln/srso.rst b/Documentation/admin-guide/hw-vuln/srso.rst
index 2ad1c05b8c88..66af95251a3d 100644
--- a/Documentation/admin-guide/hw-vuln/srso.rst
+++ b/Documentation/admin-guide/hw-vuln/srso.rst
@@ -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:
 
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 508c0dad116b..43653f2704c9 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -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
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 72765b2fe0d8..d35519b337ba 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index a5d0998d7604..1d7afc40f227 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -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
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index a713c803a3a3..77ab66c5bb96 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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;
 }
 
diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c
index 4bf4fad5b148..5a18ecc04a6c 100644
--- a/arch/x86/lib/msr.c
+++ b/arch/x86/lib/msr.c
@@ -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)
-- 
2.43.0


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Patrick Bellasi]

> 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

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 1d7afc40f2272..7609d80eda123 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -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;
        }

Re: [PATCH final?] x86/bugs: KVM: Add support for SRSO_MSR_FIX

[Borislav Petkov]

On Tue, Feb 18, 2025 at 02:42:57PM +0000, Patrick Bellasi wrote:
> 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?

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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette