linux-kernel - Re: [PATCH 1/3] x86/bugs: Use early_param for spectre_v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250814031939.wkkmztyurgtnhjx6@desk>
Date: Wed, 13 Aug 2025 20:19:39 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: David Kaplan <david.kaplan@....com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] x86/bugs: Use early_param for spectre_v2_user

On Mon, Aug 11, 2025 at 09:26:57AM -0500, David Kaplan wrote:
> Most of the mitigations in bugs.c use early_param to parse their command
> line options.  Modify spectre_v2_user to use early_param for consistency.
> 
> Signed-off-by: David Kaplan <david.kaplan@....com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 62 ++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index b74bf937cd9f..6bfe199b9f3e 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1829,7 +1829,7 @@ enum spectre_v2_mitigation_cmd {
>  
>  static enum spectre_v2_mitigation_cmd spectre_v2_cmd __ro_after_init = SPECTRE_V2_CMD_AUTO;
>  
> -enum spectre_v2_user_cmd {
> +enum spectre_v2_user_mitigation_cmd {
>  	SPECTRE_V2_USER_CMD_NONE,
>  	SPECTRE_V2_USER_CMD_AUTO,
>  	SPECTRE_V2_USER_CMD_FORCE,
> @@ -1839,6 +1839,9 @@ enum spectre_v2_user_cmd {
>  	SPECTRE_V2_USER_CMD_SECCOMP_IBPB,
>  };
>  
> +static enum spectre_v2_user_mitigation_cmd spectre_v2_user_cmd __ro_after_init =
> +	SPECTRE_V2_USER_CMD_AUTO;
> +
>  static const char * const spectre_v2_user_strings[] = {
>  	[SPECTRE_V2_USER_NONE]			= "User space: Vulnerable",
>  	[SPECTRE_V2_USER_STRICT]		= "User space: Mitigation: STIBP protection",
> @@ -1847,50 +1850,45 @@ static const char * const spectre_v2_user_strings[] = {
>  	[SPECTRE_V2_USER_SECCOMP]		= "User space: Mitigation: STIBP via seccomp and prctl",
>  };
>  
> -static const struct {
> -	const char			*option;
> -	enum spectre_v2_user_cmd	cmd;
> -	bool				secure;
> -} v2_user_options[] __initconst = {
> -	{ "auto",		SPECTRE_V2_USER_CMD_AUTO,		false },
> -	{ "off",		SPECTRE_V2_USER_CMD_NONE,		false },
> -	{ "on",			SPECTRE_V2_USER_CMD_FORCE,		true  },
> -	{ "prctl",		SPECTRE_V2_USER_CMD_PRCTL,		false },
> -	{ "prctl,ibpb",		SPECTRE_V2_USER_CMD_PRCTL_IBPB,		false },
> -	{ "seccomp",		SPECTRE_V2_USER_CMD_SECCOMP,		false },
> -	{ "seccomp,ibpb",	SPECTRE_V2_USER_CMD_SECCOMP_IBPB,	false },
> -};
> -
>  static void __init spec_v2_user_print_cond(const char *reason, bool secure)
>  {
>  	if (boot_cpu_has_bug(X86_BUG_SPECTRE_V2) != 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 int __init spectre_v2_parse_user_cmdline(char *str)
>  {
> -	char arg[20];
> -	int ret, i;
> +	if (!str)
> +		return -EINVAL;
>  
>  	if (!IS_ENABLED(CONFIG_MITIGATION_SPECTRE_V2))
>  		return SPECTRE_V2_USER_CMD_NONE;
>  
> -	ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
> -				  arg, sizeof(arg));
> -	if (ret < 0)
> -		return SPECTRE_V2_USER_CMD_AUTO;
> +	if (!strcmp(str, "auto"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_AUTO;
> +	else if (!strcmp(str, "off"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_NONE;
> +	else if (!strcmp(str, "on"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_FORCE;
> +	else if (!strcmp(str, "prctl"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_PRCTL;
> +	else if (!strcmp(str, "prctl,ibpb"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_PRCTL_IBPB;
> +	else if (!strcmp(str, "seccomp"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_SECCOMP;
> +	else if (!strcmp(str, "seccomp,ibpb"))
> +		spectre_v2_user_cmd = SPECTRE_V2_USER_CMD_SECCOMP_IBPB;
> +	else
> +		pr_err("Ignoring unknown spectre_v2_user option (%s).", str);

Should return from here? Otherwise, spec_v2_user_print_cond() will print
the unknown option as forced:

	pr_info("spectre_v2_user=%s forced on command line.\n", reason);

>  
> -	for (i = 0; i < ARRAY_SIZE(v2_user_options); i++) {
> -		if (match_option(arg, ret, v2_user_options[i].option)) {
> -			spec_v2_user_print_cond(v2_user_options[i].option,
> -						v2_user_options[i].secure);
> -			return v2_user_options[i].cmd;
> -		}
> -	}
> +	if (spectre_v2_user_cmd == SPECTRE_V2_USER_CMD_FORCE)
> +		spec_v2_user_print_cond(str, true);
> +	else
> +		spec_v2_user_print_cond(str, false);

I don't see the need for spec_v2_user_print_cond(), should it be zapped?

And then just do:

	if (spectre_v2_user_cmd != SPECTRE_V2_USER_CMD_FORCE)
		pr_info("spectre_v2_user=%s forced on command line.\n", str);

I also feel that the original print is a bit confusing (code-wise), because
it prints "forced" when the user opts for anything other than
"on"(CMD_FORCE). I think the intent was to inform the user that a partially
secure option is chosen.

> -	pr_err("Unknown user space protection option (%s). Switching to default\n", arg);
> -	return SPECTRE_V2_USER_CMD_AUTO;
> +	return 0;
>  }