[PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

Different AMD processors may have different implementations of STIBP.
When STIBP is conditionally enabled, some implementations would benefit
from having STIBP always on instead of toggling the STIBP bit through MSR
writes. This preference is advertised through a CPUID feature bit.

When conditional STIBP support is requested at boot and the CPU advertises
STIBP always-on mode as preferred, switch to STIBP "on" support. To show
that this transition has occurred, create a new spectre_v2_user_mitigation
value and a new spectre_v2_user_strings message. The new mitigation value
is used in spectre_v2_user_select_mitigation() to print the new mitigation
message as well as to return a new string from stibp_state().

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---

This patch is against the x86/pti branch of the tip tree:
  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/pti

 arch/x86/include/asm/cpufeatures.h   |    1 +
 arch/x86/include/asm/nospec-branch.h |    1 +
 arch/x86/kernel/cpu/bugs.c           |   28 ++++++++++++++++++++++------
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 28c4a50..df8e94e2 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -284,6 +284,7 @@
 #define X86_FEATURE_AMD_IBPB		(13*32+12) /* "" Indirect Branch Prediction Barrier */
 #define X86_FEATURE_AMD_IBRS		(13*32+14) /* "" Indirect Branch Restricted Speculation */
 #define X86_FEATURE_AMD_STIBP		(13*32+15) /* "" Single Thread Indirect Branch Predictors */
+#define X86_FEATURE_AMD_STIBP_ALWAYS_ON	(13*32+17) /* "" Single Thread Indirect Branch Predictors always-on preferred */
 #define X86_FEATURE_AMD_SSBD		(13*32+24) /* "" Speculative Store Bypass Disable */
 #define X86_FEATURE_VIRT_SSBD		(13*32+25) /* Virtualized Speculative Store Bypass Disable */
 #define X86_FEATURE_AMD_SSB_NO		(13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 032b600..dad12b7 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -232,6 +232,7 @@ enum spectre_v2_mitigation {
 enum spectre_v2_user_mitigation {
 	SPECTRE_V2_USER_NONE,
 	SPECTRE_V2_USER_STRICT,
+	SPECTRE_V2_USER_STRICT_PREFERRED,
 	SPECTRE_V2_USER_PRCTL,
 	SPECTRE_V2_USER_SECCOMP,
 };
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 58689ac..e53cefa 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -262,10 +262,11 @@ enum spectre_v2_user_cmd {
 };
 
 static const char * const spectre_v2_user_strings[] = {
-	[SPECTRE_V2_USER_NONE]		= "User space: Vulnerable",
-	[SPECTRE_V2_USER_STRICT]	= "User space: Mitigation: STIBP protection",
-	[SPECTRE_V2_USER_PRCTL]		= "User space: Mitigation: STIBP via prctl",
-	[SPECTRE_V2_USER_SECCOMP]	= "User space: Mitigation: STIBP via seccomp and prctl",
+	[SPECTRE_V2_USER_NONE]			= "User space: Vulnerable",
+	[SPECTRE_V2_USER_STRICT]		= "User space: Mitigation: STIBP protection",
+	[SPECTRE_V2_USER_STRICT_PREFERRED]	= "User space: Mitigation: STIBP always-on preferred protection",
+	[SPECTRE_V2_USER_PRCTL]			= "User space: Mitigation: STIBP via prctl",
+	[SPECTRE_V2_USER_SECCOMP]		= "User space: Mitigation: STIBP via seccomp and prctl",
 };
 
 static const struct {
@@ -355,6 +356,15 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
 		break;
 	}
 
+	/*
+	 * At this point, an STIBP mode other than "off" has been set.
+	 * If STIBP support is not being forced, check if STIBP always-on
+	 * is preferred.
+	 */
+	if (mode != SPECTRE_V2_USER_STRICT &&
+	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON))
+		mode = SPECTRE_V2_USER_STRICT_PREFERRED;
+
 	/* Initialize Indirect Branch Prediction Barrier */
 	if (boot_cpu_has(X86_FEATURE_IBPB)) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
@@ -610,6 +620,7 @@ void arch_smt_update(void)
 	case SPECTRE_V2_USER_NONE:
 		break;
 	case SPECTRE_V2_USER_STRICT:
+	case SPECTRE_V2_USER_STRICT_PREFERRED:
 		update_stibp_strict();
 		break;
 	case SPECTRE_V2_USER_PRCTL:
@@ -812,7 +823,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
 		 * Indirect branch speculation is always disabled in strict
 		 * mode.
 		 */
-		if (spectre_v2_user == SPECTRE_V2_USER_STRICT)
+		if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
+		    spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
 			return -EPERM;
 		task_clear_spec_ib_disable(task);
 		task_update_spec_tif(task);
@@ -825,7 +837,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
 		 */
 		if (spectre_v2_user == SPECTRE_V2_USER_NONE)
 			return -EPERM;
-		if (spectre_v2_user == SPECTRE_V2_USER_STRICT)
+		if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
+		    spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
 			return 0;
 		task_set_spec_ib_disable(task);
 		if (ctrl == PR_SPEC_FORCE_DISABLE)
@@ -896,6 +909,7 @@ static int ib_prctl_get(struct task_struct *task)
 			return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
 		return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
 	case SPECTRE_V2_USER_STRICT:
+	case SPECTRE_V2_USER_STRICT_PREFERRED:
 		return PR_SPEC_DISABLE;
 	default:
 		return PR_SPEC_NOT_AFFECTED;
@@ -1089,6 +1103,8 @@ static char *stibp_state(void)
 		return ", STIBP: disabled";
 	case SPECTRE_V2_USER_STRICT:
 		return ", STIBP: forced";
+	case SPECTRE_V2_USER_STRICT_PREFERRED:
+		return ", STIBP: always-on preferred";
 	case SPECTRE_V2_USER_PRCTL:
 	case SPECTRE_V2_USER_SECCOMP:
 		if (static_key_enabled(&switch_to_cond_stibp))

Re: [PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/11/2018 01:10 PM, Lendacky, Thomas wrote:
> Different AMD processors may have different implementations of STIBP.
> When STIBP is conditionally enabled, some implementations would benefit
> from having STIBP always on instead of toggling the STIBP bit through MSR
> writes. This preference is advertised through a CPUID feature bit.
> 
> When conditional STIBP support is requested at boot and the CPU advertises
> STIBP always-on mode as preferred, switch to STIBP "on" support. To show
> that this transition has occurred, create a new spectre_v2_user_mitigation
> value and a new spectre_v2_user_strings message. The new mitigation value
> is used in spectre_v2_user_select_mitigation() to print the new mitigation
> message as well as to return a new string from stibp_state().

Alternatively, I can reuse SPECTRE_V2_USER_STRICT and issue pr_info_once()
to show that the method has been switched.  This would reduce the changes
to the code, but then the sysfs information doesn't show the switch (which
may be just fine).

Also, I'm open to the wording of the messages if the decision is to stick
with SPECTRE_V2_USER_STRICT_PREFERRED.

Thanks,
Tom

> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> 
> This patch is against the x86/pti branch of the tip tree:
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/pti
> 
>  arch/x86/include/asm/cpufeatures.h   |    1 +
>  arch/x86/include/asm/nospec-branch.h |    1 +
>  arch/x86/kernel/cpu/bugs.c           |   28 ++++++++++++++++++++++------
>  3 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index 28c4a50..df8e94e2 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -284,6 +284,7 @@
>  #define X86_FEATURE_AMD_IBPB		(13*32+12) /* "" Indirect Branch Prediction Barrier */
>  #define X86_FEATURE_AMD_IBRS		(13*32+14) /* "" Indirect Branch Restricted Speculation */
>  #define X86_FEATURE_AMD_STIBP		(13*32+15) /* "" Single Thread Indirect Branch Predictors */
> +#define X86_FEATURE_AMD_STIBP_ALWAYS_ON	(13*32+17) /* "" Single Thread Indirect Branch Predictors always-on preferred */
>  #define X86_FEATURE_AMD_SSBD		(13*32+24) /* "" Speculative Store Bypass Disable */
>  #define X86_FEATURE_VIRT_SSBD		(13*32+25) /* Virtualized Speculative Store Bypass Disable */
>  #define X86_FEATURE_AMD_SSB_NO		(13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index 032b600..dad12b7 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -232,6 +232,7 @@ enum spectre_v2_mitigation {
>  enum spectre_v2_user_mitigation {
>  	SPECTRE_V2_USER_NONE,
>  	SPECTRE_V2_USER_STRICT,
> +	SPECTRE_V2_USER_STRICT_PREFERRED,
>  	SPECTRE_V2_USER_PRCTL,
>  	SPECTRE_V2_USER_SECCOMP,
>  };
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 58689ac..e53cefa 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -262,10 +262,11 @@ enum spectre_v2_user_cmd {
>  };
>  
>  static const char * const spectre_v2_user_strings[] = {
> -	[SPECTRE_V2_USER_NONE]		= "User space: Vulnerable",
> -	[SPECTRE_V2_USER_STRICT]	= "User space: Mitigation: STIBP protection",
> -	[SPECTRE_V2_USER_PRCTL]		= "User space: Mitigation: STIBP via prctl",
> -	[SPECTRE_V2_USER_SECCOMP]	= "User space: Mitigation: STIBP via seccomp and prctl",
> +	[SPECTRE_V2_USER_NONE]			= "User space: Vulnerable",
> +	[SPECTRE_V2_USER_STRICT]		= "User space: Mitigation: STIBP protection",
> +	[SPECTRE_V2_USER_STRICT_PREFERRED]	= "User space: Mitigation: STIBP always-on preferred protection",
> +	[SPECTRE_V2_USER_PRCTL]			= "User space: Mitigation: STIBP via prctl",
> +	[SPECTRE_V2_USER_SECCOMP]		= "User space: Mitigation: STIBP via seccomp and prctl",
>  };
>  
>  static const struct {
> @@ -355,6 +356,15 @@ static void __init spec_v2_user_print_cond(const char *reason, bool secure)
>  		break;
>  	}
>  
> +	/*
> +	 * At this point, an STIBP mode other than "off" has been set.
> +	 * If STIBP support is not being forced, check if STIBP always-on
> +	 * is preferred.
> +	 */
> +	if (mode != SPECTRE_V2_USER_STRICT &&
> +	    boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON))
> +		mode = SPECTRE_V2_USER_STRICT_PREFERRED;
> +
>  	/* Initialize Indirect Branch Prediction Barrier */
>  	if (boot_cpu_has(X86_FEATURE_IBPB)) {
>  		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
> @@ -610,6 +620,7 @@ void arch_smt_update(void)
>  	case SPECTRE_V2_USER_NONE:
>  		break;
>  	case SPECTRE_V2_USER_STRICT:
> +	case SPECTRE_V2_USER_STRICT_PREFERRED:
>  		update_stibp_strict();
>  		break;
>  	case SPECTRE_V2_USER_PRCTL:
> @@ -812,7 +823,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
>  		 * Indirect branch speculation is always disabled in strict
>  		 * mode.
>  		 */
> -		if (spectre_v2_user == SPECTRE_V2_USER_STRICT)
> +		if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
> +		    spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
>  			return -EPERM;
>  		task_clear_spec_ib_disable(task);
>  		task_update_spec_tif(task);
> @@ -825,7 +837,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
>  		 */
>  		if (spectre_v2_user == SPECTRE_V2_USER_NONE)
>  			return -EPERM;
> -		if (spectre_v2_user == SPECTRE_V2_USER_STRICT)
> +		if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
> +		    spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
>  			return 0;
>  		task_set_spec_ib_disable(task);
>  		if (ctrl == PR_SPEC_FORCE_DISABLE)
> @@ -896,6 +909,7 @@ static int ib_prctl_get(struct task_struct *task)
>  			return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
>  		return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
>  	case SPECTRE_V2_USER_STRICT:
> +	case SPECTRE_V2_USER_STRICT_PREFERRED:
>  		return PR_SPEC_DISABLE;
>  	default:
>  		return PR_SPEC_NOT_AFFECTED;
> @@ -1089,6 +1103,8 @@ static char *stibp_state(void)
>  		return ", STIBP: disabled";
>  	case SPECTRE_V2_USER_STRICT:
>  		return ", STIBP: forced";
> +	case SPECTRE_V2_USER_STRICT_PREFERRED:
> +		return ", STIBP: always-on preferred";
>  	case SPECTRE_V2_USER_PRCTL:
>  	case SPECTRE_V2_USER_SECCOMP:
>  		if (static_key_enabled(&switch_to_cond_stibp))
> 

Re: [PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Borislav Petkov]

On Tue, Dec 11, 2018 at 07:19:09PM +0000, Lendacky, Thomas wrote:
> Alternatively, I can reuse SPECTRE_V2_USER_STRICT and issue pr_info_once()
> to show that the method has been switched.  This would reduce the changes
> to the code, but then the sysfs information doesn't show the switch (which
> may be just fine).

You can still query X86_FEATURE_AMD_STIBP_ALWAYS_ON in stibp_state(),
no?

But yeah, forcing SPECTRE_V2_USER_STRICT on STIBP_ALWAYS_ON makes sense
to me, with the already gazillion options we have :-\

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/11/2018 03:19 PM, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 07:19:09PM +0000, Lendacky, Thomas wrote:
>> Alternatively, I can reuse SPECTRE_V2_USER_STRICT and issue pr_info_once()
>> to show that the method has been switched.  This would reduce the changes
>> to the code, but then the sysfs information doesn't show the switch (which
>> may be just fine).
> 
> You can still query X86_FEATURE_AMD_STIBP_ALWAYS_ON in stibp_state(),
> no?

If using just SPECTRE_V2_USER_STRICT then the code in stibp_state() would
have to be able differentiate between the case where the mode was switched
because of X86_FEATURE_AMD_STIBP_ALWAYS_ON vs a kernel command line with
"spectre_v2_user=on". I could always set and use a static variable in the
file just for the stibp_state() case.

> 
> But yeah, forcing SPECTRE_V2_USER_STRICT on STIBP_ALWAYS_ON makes sense
> to me, with the already gazillion options we have :-\

I'll give it a bit of time and see if there's any other discussion and
re-submit without the new SPECTRE_V2_USER_STRICT_PREFERRED value.

Thanks,
Tom

> 

Re: [PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Borislav Petkov]

On Tue, Dec 11, 2018 at 10:17:30PM +0000, Lendacky, Thomas wrote:
> If using just SPECTRE_V2_USER_STRICT then the code in stibp_state() would
> have to be able differentiate between the case where the mode was switched
> because of X86_FEATURE_AMD_STIBP_ALWAYS_ON vs a kernel command line with
> "spectre_v2_user=on". I could always set and use a static variable in the
> file just for the stibp_state() case.

Does it matter on X86_FEATURE_AMD_STIBP_ALWAYS_ON CPUs?

I mean, we want STIBP to be always on there so you can do:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 25e914f77bb8..d14860d1cf9c 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1089,7 +1089,10 @@ static char *stibp_state(void)
 	case SPECTRE_V2_USER_NONE:
 		return ", STIBP: disabled";
 	case SPECTRE_V2_USER_STRICT:
-		return ", STIBP: forced";
+		if (boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON))
+			return ", STIBP: always on";
+		else
+			return ", STIBP: forced";
 	case SPECTRE_V2_USER_PRCTL:
 	case SPECTRE_V2_USER_SECCOMP:
 		if (static_key_enabled(&switch_to_cond_stibp))

so if user has booted with spectre_v2_user=on, we say, "oh well, it is always
enabled anyway"... or?

> I'll give it a bit of time and see if there's any other discussion and
> re-submit without the new SPECTRE_V2_USER_STRICT_PREFERRED value.

Sure.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/speculation: Add support for STIBP always-on preferred mode

[Lendacky, Thomas]

On 12/11/2018 04:22 PM, Borislav Petkov wrote:
> On Tue, Dec 11, 2018 at 10:17:30PM +0000, Lendacky, Thomas wrote:
>> If using just SPECTRE_V2_USER_STRICT then the code in stibp_state() would
>> have to be able differentiate between the case where the mode was switched
>> because of X86_FEATURE_AMD_STIBP_ALWAYS_ON vs a kernel command line with
>> "spectre_v2_user=on". I could always set and use a static variable in the
>> file just for the stibp_state() case.
> 
> Does it matter on X86_FEATURE_AMD_STIBP_ALWAYS_ON CPUs?
> 
> I mean, we want STIBP to be always on there so you can do:
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 25e914f77bb8..d14860d1cf9c 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1089,7 +1089,10 @@ static char *stibp_state(void)
>  	case SPECTRE_V2_USER_NONE:
>  		return ", STIBP: disabled";
>  	case SPECTRE_V2_USER_STRICT:
> -		return ", STIBP: forced";
> +		if (boot_cpu_has(X86_FEATURE_AMD_STIBP_ALWAYS_ON))
> +			return ", STIBP: always on";
> +		else
> +			return ", STIBP: forced";
>  	case SPECTRE_V2_USER_PRCTL:
>  	case SPECTRE_V2_USER_SECCOMP:
>  		if (static_key_enabled(&switch_to_cond_stibp))
> 
> so if user has booted with spectre_v2_user=on, we say, "oh well, it is always
> enabled anyway"... or?

I'm ok with that. Having said that, we can probably just leave it as is
and return "forced" even for the ALWAYS_ON case, then.

But if we want to differentiate, though, a simple static bool that is set
when the mode is switched works just as well.

> 
>> I'll give it a bit of time and see if there's any other discussion and
>> re-submit without the new SPECTRE_V2_USER_STRICT_PREFERRED value.
> 
> Sure.

Eh, I'll just submit v2 with the changes now and we can decide between the
above or the bool.

Thanks,
Tom

>