[PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2

Breno Leitao posted 2 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2
Posted by Breno Leitao 1 month, 1 week ago
Change the default value of spectre v2 in user mode to respect the
CONFIG_MITIGATION_SPECTRE_V2 config option.

Currently, user mode spectre v2 is set to auto
(SPECTRE_V2_USER_CMD_AUTO) by default, even if
CONFIG_MITIGATION_SPECTRE_V2 is disabled.

Set the spectre_v2 value to auto (SPECTRE_V2_USER_CMD_AUTO) if the
Spectre v2 config (CONFIG_MITIGATION_SPECTRE_V2) is enabled, otherwise
set the value to none (SPECTRE_V2_USER_CMD_NONE).

Important to say the command line argument "spectre_v2_user" overwrites
the default value in both cases.

Signed-off-by: Breno Leitao <leitao@debian.org>
Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6d3b61d7be9c..c21ff072e048 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1277,9 +1277,13 @@ 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)
 {
+	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:
 		return SPECTRE_V2_USER_CMD_NONE;
@@ -1292,7 +1296,7 @@ spectre_v2_parse_user_cmdline(void)
 	ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
 				  arg, sizeof(arg));
 	if (ret < 0)
-		return SPECTRE_V2_USER_CMD_AUTO;
+		return mode;
 
 	for (i = 0; i < ARRAY_SIZE(v2_user_options); i++) {
 		if (match_option(arg, ret, v2_user_options[i].option)) {
@@ -1302,8 +1306,8 @@ spectre_v2_parse_user_cmdline(void)
 		}
 	}
 
-	pr_err("Unknown user space protection option (%s). Switching to AUTO select\n", arg);
-	return SPECTRE_V2_USER_CMD_AUTO;
+	pr_err("Unknown user space protection option (%s). Switching to default\n", arg);
+	return mode;
 }
 
 static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
-- 
2.43.5
Re: [PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2
Posted by Pawan Gupta 4 weeks, 1 day ago
On Tue, Oct 15, 2024 at 03:51:06AM -0700, Breno Leitao wrote:
> @@ -1277,9 +1277,13 @@ 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)
>  {
> +	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:
>  		return SPECTRE_V2_USER_CMD_NONE;
> @@ -1292,7 +1296,7 @@ spectre_v2_parse_user_cmdline(void)
>  	ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
>  				  arg, sizeof(arg));
>  	if (ret < 0)
> -		return SPECTRE_V2_USER_CMD_AUTO;
> +		return mode;

This doesn't look right to me, spectre_v2=eibrs|retpoline... will override
CONFIG_MITIGATION_SPECTRE_V2=n and enable the kernel mitigation, but the
user mitigation will stay disabled. If this is the intention it should be
clearly documented that enabling kernel mitigation does not enable user
mitigation. And an explicit spectre_v2_user= is required to enable user
mitigation.
Re: [PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2
Posted by Breno Leitao 4 weeks ago
Hello Pawan,

On Mon, Oct 28, 2024 at 07:34:53AM -0700, Pawan Gupta wrote:
> On Tue, Oct 15, 2024 at 03:51:06AM -0700, Breno Leitao wrote:
> > @@ -1277,9 +1277,13 @@ 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)
> >  {
> > +	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:
> >  		return SPECTRE_V2_USER_CMD_NONE;
> > @@ -1292,7 +1296,7 @@ spectre_v2_parse_user_cmdline(void)
> >  	ret = cmdline_find_option(boot_command_line, "spectre_v2_user",
> >  				  arg, sizeof(arg));
> >  	if (ret < 0)
> > -		return SPECTRE_V2_USER_CMD_AUTO;
> > +		return mode;
> 
> This doesn't look right to me, spectre_v2=eibrs|retpoline... will override
> CONFIG_MITIGATION_SPECTRE_V2=n and enable the kernel mitigation, but the
> user mitigation will stay disabled.

Correct. In the current configuration, if the user compiles the kernel
with CONFIG_MITIGATION_SPECTRE_V2=n explict, then the kernel will have
all spectre v2 mitigations disable by default, and can opt-in using
command line arguments.

In other words, if the user compiles the kernel with
CONFIG_MITIGATION_SPECTRE_V2=n, then it means all spectre v2 mitigations
will be disable. the user wants to opt-in mitigation by mitigation, tho.

For instance, if user sets `spectre_v2=eibrs|retpoline` in cmdline, then
it must not set `spectre_v2_user` automatically. User can decide to do
enable userspace mitigation so as well by passing userspace another
command line `spectre_v2_user=on|prctl|..`. I.e, they are independent
selectable.

> If this is the intention it should be
> clearly documented that enabling kernel mitigation does not enable user
> mitigation. And an explicit spectre_v2_user= is required to enable user
> mitigation.

That is fair. I didn't find a place where to document about diferent
behavior when CONFIG_MITIGATION_X is disabled. What would you suggest?

Thanks for review and good points.
--breno
Re: [PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2
Posted by Pawan Gupta 3 weeks, 6 days ago
On Tue, Oct 29, 2024 at 02:19:12AM -0700, Breno Leitao wrote:
> > If this is the intention it should be
> > clearly documented that enabling kernel mitigation does not enable user
> > mitigation. And an explicit spectre_v2_user= is required to enable user
> > mitigation.
> 
> That is fair. I didn't find a place where to document about diferent
> behavior when CONFIG_MITIGATION_X is disabled. What would you suggest?

You could describe the behavior in the commit message and update kernel
parameter documentation.

With that:

Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>

---
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..f8bc02cd10ec 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6241,6 +6241,8 @@
 
 			Selecting 'on' will also enable the mitigation
 			against user space to user space task attacks.
+			Selecting specific mitigation does not force enable
+			user mitigations.
 
 			Selecting 'off' will disable both the kernel and
 			the user space protections.
Re: [PATCH 2/2] x86/bugs: spectre user default must depend on MITIGATION_SPECTRE_V2
Posted by Breno Leitao 3 weeks, 5 days ago
Hello Pawan,

On Wed, Oct 30, 2024 at 11:40:53AM -0700, Pawan Gupta wrote:
> On Tue, Oct 29, 2024 at 02:19:12AM -0700, Breno Leitao wrote:
> > > If this is the intention it should be
> > > clearly documented that enabling kernel mitigation does not enable user
> > > mitigation. And an explicit spectre_v2_user= is required to enable user
> > > mitigation.
> > 
> > That is fair. I didn't find a place where to document about diferent
> > behavior when CONFIG_MITIGATION_X is disabled. What would you suggest?
> 
> You could describe the behavior in the commit message and update kernel
> parameter documentation.
> 
> With that:
> 
> Reviewed-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> 
> ---
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..f8bc02cd10ec 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6241,6 +6241,8 @@
>  
>  			Selecting 'on' will also enable the mitigation
>  			against user space to user space task attacks.
> +			Selecting specific mitigation does not force enable
> +			user mitigations.
>  
>  			Selecting 'off' will disable both the kernel and
>  			the user space protections.

Sure, I will update it.

Thanks for the review!
--breno