Re: [PATCH v5 11/16] x86/bugs: Restructure spectre_v2_user mitigation

[Borislav Petkov <bp@alien8.de>]

On Fri, Apr 18, 2025 at 11:17:16AM -0500, David Kaplan wrote:
> @@ -217,6 +214,11 @@ void __init cpu_select_mitigations(void)
>  	 * choices.
>  	 */
>  	retbleed_update_mitigation();
> +	/*
> +	 * spectre_v2_user_update_mitigation() depends on
> +	 * retbleed_update_mitigation().
> +	 */

Why aren't you keeping the reason for the dependency from the above comment?

That's important when we need to touch this code again...

> +	spectre_v2_user_update_mitigation();
>  	mds_update_mitigation();
>  	taa_update_mitigation();
>  	mmio_update_mitigation();
> @@ -224,6 +226,7 @@ void __init cpu_select_mitigations(void)
>  
>  	spectre_v1_apply_mitigation();
>  	retbleed_apply_mitigation();
> +	spectre_v2_user_apply_mitigation();
>  	mds_apply_mitigation();
>  	taa_apply_mitigation();
>  	mmio_apply_mitigation();
> @@ -1374,6 +1377,8 @@ enum spectre_v2_mitigation_cmd {
>  	SPECTRE_V2_CMD_IBRS,
>  };
>  
> +static enum spectre_v2_mitigation_cmd spectre_v2_cmd __ro_after_init = SPECTRE_V2_CMD_AUTO;
> +
>  enum spectre_v2_user_cmd {
>  	SPECTRE_V2_USER_CMD_NONE,
>  	SPECTRE_V2_USER_CMD_AUTO,
> @@ -1412,31 +1417,19 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
>  		pr_info("spectre_v2_user=%s forced on command line.\n", reason);
>  }
>  
> -static __ro_after_init enum spectre_v2_mitigation_cmd spectre_v2_cmd;
> -
>  static enum spectre_v2_user_cmd __init
>  spectre_v2_parse_user_cmdline(void)

Lemme unbreak that silly thing while here...

>  {
> -	enum spectre_v2_user_cmd mode;
>  	char arg[20];
>  	int ret, i;
>  
> -	mode = IS_ENABLED(CONFIG_MITIGATION_SPECTRE_V2) ?
> -		SPECTRE_V2_USER_CMD_AUTO : SPECTRE_V2_USER_CMD_NONE;
> -
> -	switch (spectre_v2_cmd) {
> -	case SPECTRE_V2_CMD_NONE:
> +	if (cpu_mitigations_off() || !IS_ENABLED(CONFIG_MITIGATION_SPECTRE_V2))
>  		return SPECTRE_V2_USER_CMD_NONE;
> -	case SPECTRE_V2_CMD_FORCE:
> -		return SPECTRE_V2_USER_CMD_FORCE;
> -	default:
> -		break;
> -	}
>  
>  	ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
>  				  arg, sizeof(arg));
>  	if (ret < 0)
> -		return mode;
> +		return SPECTRE_V2_USER_CMD_AUTO;
>  
>  	for (i = 0; i < ARRAY_SIZE(v2_user_options); i++) {
>  		if (match_option(arg, ret, v2_user_options[i].option)) {
> @@ -1447,7 +1440,7 @@ spectre_v2_parse_user_cmdline(void)
>  	}
>  
>  	pr_err("Unknown user space protection option (%s). Switching to default\n", arg);
> -	return mode;
> +	return SPECTRE_V2_USER_CMD_AUTO;
>  }
>  
>  static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
> @@ -1458,7 +1451,6 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
>  static void __init
>  spectre_v2_user_select_mitigation(void)

That too.

>  {
> -	enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
>  	enum spectre_v2_user_cmd cmd;

Might as well get rid of that one.

>  	if (!boot_cpu_has(X86_FEATURE_IBPB) && !boot_cpu_has(X86_FEATURE_STIBP))
> @@ -1467,48 +1459,65 @@ spectre_v2_user_select_mitigation(void)
>  	cmd = spectre_v2_parse_user_cmdline();
>  	switch (cmd) {
>  	case SPECTRE_V2_USER_CMD_NONE:
> -		goto set_mode;
> +		return;
>  	case SPECTRE_V2_USER_CMD_FORCE:
> -		mode = SPECTRE_V2_USER_STRICT;
> +		spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
> +		spectre_v2_user_stibp = SPECTRE_V2_USER_STRICT;

Those should be aligned at the '=' sign for better readability.

...

IOW, all the changes ontop:

---

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index afea9179acdd..dc75195760ca 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -214,9 +214,11 @@ void __init cpu_select_mitigations(void)
 	 * choices.
 	 */
 	retbleed_update_mitigation();
+
 	/*
 	 * spectre_v2_user_update_mitigation() depends on
-	 * retbleed_update_mitigation().
+	 * retbleed_update_mitigation(), specifically the STIBP
+	 * selection is forced for UNRET or IBPB.
 	 */
 	spectre_v2_user_update_mitigation();
 	mds_update_mitigation();
@@ -1422,8 +1424,7 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
 		pr_info("spectre_v2_user=%s forced on command line.\n", reason);
 }
 
-static enum spectre_v2_user_cmd __init
-spectre_v2_parse_user_cmdline(void)
+static enum spectre_v2_user_cmd __init spectre_v2_parse_user_cmdline(void)
 {
 	char arg[20];
 	int ret, i;
@@ -1453,29 +1454,25 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
 	return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS;
 }
 
-static void __init
-spectre_v2_user_select_mitigation(void)
+static void __init spectre_v2_user_select_mitigation(void)
 {
-	enum spectre_v2_user_cmd cmd;
-
 	if (!boot_cpu_has(X86_FEATURE_IBPB) && !boot_cpu_has(X86_FEATURE_STIBP))
 		return;
 
-	cmd = spectre_v2_parse_user_cmdline();
-	switch (cmd) {
+	switch (spectre_v2_parse_user_cmdline()) {
 	case SPECTRE_V2_USER_CMD_NONE:
 		return;
 	case SPECTRE_V2_USER_CMD_FORCE:
-		spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
+		spectre_v2_user_ibpb  = SPECTRE_V2_USER_STRICT;
 		spectre_v2_user_stibp = SPECTRE_V2_USER_STRICT;
 		break;
 	case SPECTRE_V2_USER_CMD_AUTO:
 	case SPECTRE_V2_USER_CMD_PRCTL:
-		spectre_v2_user_ibpb = SPECTRE_V2_USER_PRCTL;
+		spectre_v2_user_ibpb  = SPECTRE_V2_USER_PRCTL;
 		spectre_v2_user_stibp = SPECTRE_V2_USER_PRCTL;
 		break;
 	case SPECTRE_V2_USER_CMD_PRCTL_IBPB:
-		spectre_v2_user_ibpb = SPECTRE_V2_USER_STRICT;
+		spectre_v2_user_ibpb  = SPECTRE_V2_USER_STRICT;
 		spectre_v2_user_stibp = SPECTRE_V2_USER_PRCTL;
 		break;
 	case SPECTRE_V2_USER_CMD_SECCOMP:

-- 
Regards/Gruss,
    Boris.

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