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

[Yosry Ahmed <yosry.ahmed@linux.dev>]

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