* [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
@ 2019-12-29 16:48 Anthony Steinhauser
2020-01-20 19:02 ` Thomas Gleixner
2020-01-21 11:33 ` Borislav Petkov
0 siblings, 2 replies; 5+ messages in thread
From: Anthony Steinhauser @ 2019-12-29 16:48 UTC (permalink / raw)
To: linux-kernel; +Cc: mingo, bp, tglx, Anthony Steinhauser
According to the documentation, the PR_GET_SPECULATION_CTRL call should
return ENXIO when the control of the selected speculation misfeature is
not possible. EPERM should be returned only when the speculation was
disabled with PR_SPEC_FORCE_DISABLE and caller tried to enable it again.
Instead, the current implementation returns EPERM when the control of
indirect branch speculation is not possible because it is unimplemented by
the CPU. This behavior is obviously not compatible with the current
documentation. ENXIO should be returned in this case.
This change is:
1) Explicitly document that the EPERM return value applies also to cases
when the speculative behavior is forced from the boot command line and the
caller tries to change it.
2) Distinguishing between the speculation control being unimplemented and
being disabled, returning ENXIO in the first case and EPERM in the second
case.
Signed-off-by: Anthony Steinhauser <asteinhauser@google.com>
---
Documentation/userspace-api/spec_ctrl.rst | 6 +++--
arch/x86/include/asm/nospec-branch.h | 3 ++-
arch/x86/kernel/cpu/bugs.c | 29 +++++++++++++++--------
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/Documentation/userspace-api/spec_ctrl.rst b/Documentation/userspace-api/spec_ctrl.rst
index 7ddd8f667459..3ff6316207f1 100644
--- a/Documentation/userspace-api/spec_ctrl.rst
+++ b/Documentation/userspace-api/spec_ctrl.rst
@@ -83,8 +83,10 @@ ERANGE arg3 is incorrect, i.e. it's neither PR_SPEC_ENABLE nor
ENXIO Control of the selected speculation misfeature is not possible.
See PR_GET_SPECULATION_CTRL.
-EPERM Speculation was disabled with PR_SPEC_FORCE_DISABLE and caller
- tried to enable it again.
+EPERM Caller tried to enable speculation when it was disabled with
+ PR_SPEC_FORCE_DISABLE or force-disabled on the boot command line.
+ Caller tried to disable speculation when it was force-enabled on
+ the boot command line.
======= =================================================================
Speculation misfeature controls
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 5c24a7b35166..1e2caccac89e 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -220,7 +220,8 @@ enum spectre_v2_mitigation {
/* The indirect branch speculation control variants */
enum spectre_v2_user_mitigation {
- SPECTRE_V2_USER_NONE,
+ SPECTRE_V2_USER_UNAVAILABLE,
+ SPECTRE_V2_USER_DISABLED,
SPECTRE_V2_USER_STRICT,
SPECTRE_V2_USER_STRICT_PREFERRED,
SPECTRE_V2_USER_PRCTL,
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 8bf64899f56a..a6556483b136 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -489,7 +489,7 @@ static enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init =
SPECTRE_V2_NONE;
static enum spectre_v2_user_mitigation spectre_v2_user __ro_after_init =
- SPECTRE_V2_USER_NONE;
+ SPECTRE_V2_USER_UNAVAILABLE;
#ifdef CONFIG_RETPOLINE
static bool spectre_v2_bad_module;
@@ -540,7 +540,8 @@ enum spectre_v2_user_cmd {
};
static const char * const spectre_v2_user_strings[] = {
- [SPECTRE_V2_USER_NONE] = "User space: Vulnerable",
+ [SPECTRE_V2_USER_UNAVAILABLE] = "User space: Vulnerable: STIBP unavailable",
+ [SPECTRE_V2_USER_DISABLED] = "User space: Vulnerable: STIBP disabled",
[SPECTRE_V2_USER_STRICT] = "User space: Mitigation: STIBP protection",
[SPECTRE_V2_USER_STRICT_PREFERRED] = "User space: Mitigation: STIBP always-on protection",
[SPECTRE_V2_USER_PRCTL] = "User space: Mitigation: STIBP via prctl",
@@ -602,7 +603,7 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
static void __init
spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
{
- enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
+ enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_UNAVAILABLE;
bool smt_possible = IS_ENABLED(CONFIG_SMP);
enum spectre_v2_user_cmd cmd;
@@ -616,6 +617,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
cmd = spectre_v2_parse_user_cmdline(v2_cmd);
switch (cmd) {
case SPECTRE_V2_USER_CMD_NONE:
+ mode = SPECTRE_V2_USER_DISABLED;
goto set_mode;
case SPECTRE_V2_USER_CMD_FORCE:
mode = SPECTRE_V2_USER_STRICT;
@@ -676,7 +678,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
* mode.
*/
if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
- mode = SPECTRE_V2_USER_NONE;
+ mode = SPECTRE_V2_USER_UNAVAILABLE;
set_mode:
spectre_v2_user = mode;
/* Only print the STIBP mode when SMT possible */
@@ -915,7 +917,8 @@ void cpu_bugs_smt_update(void)
mutex_lock(&spec_ctrl_mutex);
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_DISABLED:
+ case SPECTRE_V2_USER_UNAVAILABLE:
break;
case SPECTRE_V2_USER_STRICT:
case SPECTRE_V2_USER_STRICT_PREFERRED:
@@ -1157,7 +1160,8 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
{
switch (ctrl) {
case PR_SPEC_ENABLE:
- if (spectre_v2_user == SPECTRE_V2_USER_NONE)
+ if (spectre_v2_user == SPECTRE_V2_USER_UNAVAILABLE ||
+ spectre_v2_user == SPECTRE_V2_USER_DISABLED)
return 0;
/*
* Indirect branch speculation is always disabled in strict
@@ -1173,9 +1177,11 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
case PR_SPEC_FORCE_DISABLE:
/*
* Indirect branch speculation is always allowed when
- * mitigation is force disabled.
+ * mitigation is unavailable or force disabled.
*/
- if (spectre_v2_user == SPECTRE_V2_USER_NONE)
+ if (spectre_v2_user == SPECTRE_V2_USER_UNAVAILABLE)
+ return -ENXIO;
+ if (spectre_v2_user == SPECTRE_V2_USER_DISABLED)
return -EPERM;
if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
@@ -1241,7 +1247,8 @@ static int ib_prctl_get(struct task_struct *task)
return PR_SPEC_NOT_AFFECTED;
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_UNAVAILABLE:
+ case SPECTRE_V2_USER_DISABLED:
return PR_SPEC_ENABLE;
case SPECTRE_V2_USER_PRCTL:
case SPECTRE_V2_USER_SECCOMP:
@@ -1495,7 +1502,9 @@ static char *stibp_state(void)
return "";
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_UNAVAILABLE:
+ return ", STIBP: unavailable";
+ case SPECTRE_V2_USER_DISABLED:
return ", STIBP: disabled";
case SPECTRE_V2_USER_STRICT:
return ", STIBP: forced";
--
2.24.1.735.g03f4e72817-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
2019-12-29 16:48 [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented Anthony Steinhauser
@ 2020-01-20 19:02 ` Thomas Gleixner
2020-01-21 11:19 ` Anthony Steinhauser
2020-01-21 11:33 ` Borislav Petkov
1 sibling, 1 reply; 5+ messages in thread
From: Thomas Gleixner @ 2020-01-20 19:02 UTC (permalink / raw)
To: Anthony Steinhauser, linux-kernel; +Cc: mingo, bp, Anthony Steinhauser
Anthony,
Anthony Steinhauser <asteinhauser@google.com> writes:
> return "";
>
> switch (spectre_v2_user) {
> - case SPECTRE_V2_USER_NONE:
> + case SPECTRE_V2_USER_UNAVAILABLE:
> + return ", STIBP: unavailable";
Shouldn't this for correctness differentiate between the case where the
STIBP mitigation feature is not available and the case where STIBP is
not used because SMT is not possible?
Thanks,
tglx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
2020-01-20 19:02 ` Thomas Gleixner
@ 2020-01-21 11:19 ` Anthony Steinhauser
0 siblings, 0 replies; 5+ messages in thread
From: Anthony Steinhauser @ 2020-01-21 11:19 UTC (permalink / raw)
To: Thomas Gleixner; +Cc: linux-kernel, mingo, bp
[-- Attachment #1: Type: text/plain, Size: 660 bytes --]
You're right, Thomas,
thanks for pointing that out. Please, see attached a corrected version.
On Mon, Jan 20, 2020 at 11:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Anthony,
>
> Anthony Steinhauser <asteinhauser@google.com> writes:
> > return "";
> >
> > switch (spectre_v2_user) {
> > - case SPECTRE_V2_USER_NONE:
> > + case SPECTRE_V2_USER_UNAVAILABLE:
> > + return ", STIBP: unavailable";
>
> Shouldn't this for correctness differentiate between the case where the
> STIBP mitigation feature is not available and the case where STIBP is
> not used because SMT is not possible?
>
> Thanks,
>
> tglx
[-- Attachment #2: 0001-Return-ENXIO-instead-of-EPERM-when-speculation-contr.patch --]
[-- Type: text/x-patch, Size: 7995 bytes --]
From 561159a358e91ce4959f06836b0aa6147f8dd346 Mon Sep 17 00:00:00 2001
From: Anthony Steinhauser <asteinhauser@google.com>
Date: Tue, 21 Jan 2020 03:09:11 -0800
Subject: [PATCH] Return ENXIO instead of EPERM when speculation control is
unimplemented
According to the documentation, the PR_GET_SPECULATION_CTRL call should
return ENXIO when the control of the selected speculation misfeature is
not possible. EPERM should be returned only when the speculation was
disabled with PR_SPEC_FORCE_DISABLE and caller tried to enable it again.
Instead, the current implementation returns EPERM when the control of
indirect branch speculation is not possible because it is unimplemented by
the CPU. This behavior is obviously not compatible with the current
documentation. ENXIO should be returned in this case.
This change is:
1) Explicitly document that the EPERM return value applies also to cases
when the speculative behavior is forced from the boot command line and the
caller tries to change it.
2) Distinguishing between the speculation control being unimplemented and
being disabled, returning ENXIO in the first case and EPERM in the second
case.
Signed-off-by: Anthony Steinhauser <asteinhauser@google.com>
---
Documentation/userspace-api/spec_ctrl.rst | 6 ++-
arch/x86/include/asm/nospec-branch.h | 4 +-
arch/x86/kernel/cpu/bugs.c | 49 +++++++++++++++++------
3 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/Documentation/userspace-api/spec_ctrl.rst b/Documentation/userspace-api/spec_ctrl.rst
index 7ddd8f667459..23fa3e9337cf 100644
--- a/Documentation/userspace-api/spec_ctrl.rst
+++ b/Documentation/userspace-api/spec_ctrl.rst
@@ -83,8 +83,10 @@ ERANGE arg3 is incorrect, i.e. it's neither PR_SPEC_ENABLE nor
ENXIO Control of the selected speculation misfeature is not possible.
See PR_GET_SPECULATION_CTRL.
-EPERM Speculation was disabled with PR_SPEC_FORCE_DISABLE and caller
- tried to enable it again.
+EPERM Caller tried to enable speculation when it was disabled with
+ PR_SPEC_FORCE_DISABLE or force-disabled on the boot command line.
+ Caller tried to disable speculation when it was force-enabled on
+ the boot command line.
======= =================================================================
Speculation misfeature controls
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 5c24a7b35166..8b04ec6cf2da 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -220,7 +220,9 @@ enum spectre_v2_mitigation {
/* The indirect branch speculation control variants */
enum spectre_v2_user_mitigation {
- SPECTRE_V2_USER_NONE,
+ SPECTRE_V2_USER_UNAVAILABLE,
+ SPECTRE_V2_USER_DISABLED,
+ SPECTRE_V2_USER_SMT_IMPOSSIBLE,
SPECTRE_V2_USER_STRICT,
SPECTRE_V2_USER_STRICT_PREFERRED,
SPECTRE_V2_USER_PRCTL,
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 8bf64899f56a..6c8c016dba61 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -489,7 +489,7 @@ static enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init =
SPECTRE_V2_NONE;
static enum spectre_v2_user_mitigation spectre_v2_user __ro_after_init =
- SPECTRE_V2_USER_NONE;
+ SPECTRE_V2_USER_UNAVAILABLE;
#ifdef CONFIG_RETPOLINE
static bool spectre_v2_bad_module;
@@ -540,7 +540,8 @@ enum spectre_v2_user_cmd {
};
static const char * const spectre_v2_user_strings[] = {
- [SPECTRE_V2_USER_NONE] = "User space: Vulnerable",
+ [SPECTRE_V2_USER_UNAVAILABLE] = "User space: Vulnerable: STIBP unavailable",
+ [SPECTRE_V2_USER_DISABLED] = "User space: Vulnerable: STIBP disabled",
[SPECTRE_V2_USER_STRICT] = "User space: Mitigation: STIBP protection",
[SPECTRE_V2_USER_STRICT_PREFERRED] = "User space: Mitigation: STIBP always-on protection",
[SPECTRE_V2_USER_PRCTL] = "User space: Mitigation: STIBP via prctl",
@@ -602,7 +603,7 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
static void __init
spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
{
- enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
+ enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_UNAVAILABLE;
bool smt_possible = IS_ENABLED(CONFIG_SMP);
enum spectre_v2_user_cmd cmd;
@@ -616,6 +617,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
cmd = spectre_v2_parse_user_cmdline(v2_cmd);
switch (cmd) {
case SPECTRE_V2_USER_CMD_NONE:
+ mode = SPECTRE_V2_USER_DISABLED;
goto set_mode;
case SPECTRE_V2_USER_CMD_FORCE:
mode = SPECTRE_V2_USER_STRICT;
@@ -672,11 +674,14 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
return;
/*
- * If SMT is not possible or STIBP is not available clear the STIBP
+ * If SMT is not possible or STIBP is not available reset the STIBP
* mode.
*/
- if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
- mode = SPECTRE_V2_USER_NONE;
+ if (!smt_possible)
+ mode = SPECTRE_V2_USER_SMT_IMPOSSIBLE;
+ else if (!boot_cpu_has(X86_FEATURE_STIBP))
+ mode = SPECTRE_V2_USER_UNAVAILABLE;
+
set_mode:
spectre_v2_user = mode;
/* Only print the STIBP mode when SMT possible */
@@ -915,7 +920,9 @@ void cpu_bugs_smt_update(void)
mutex_lock(&spec_ctrl_mutex);
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_DISABLED:
+ case SPECTRE_V2_USER_UNAVAILABLE:
+ case SPECTRE_V2_USER_SMT_IMPOSSIBLE:
break;
case SPECTRE_V2_USER_STRICT:
case SPECTRE_V2_USER_STRICT_PREFERRED:
@@ -1157,8 +1164,12 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
{
switch (ctrl) {
case PR_SPEC_ENABLE:
- if (spectre_v2_user == SPECTRE_V2_USER_NONE)
+ if (spectre_v2_user == SPECTRE_V2_USER_UNAVAILABLE ||
+ spectre_v2_user == SPECTRE_V2_USER_DISABLED)
return 0;
+
+ if (spectre_v2_user == SPECTRE_V2_USER_SMT_IMPOSSIBLE)
+ return -ENXIO;
/*
* Indirect branch speculation is always disabled in strict
* mode.
@@ -1173,12 +1184,17 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
case PR_SPEC_FORCE_DISABLE:
/*
* Indirect branch speculation is always allowed when
- * mitigation is force disabled.
+ * mitigation is unavailable or force disabled.
*/
- if (spectre_v2_user == SPECTRE_V2_USER_NONE)
+ if (spectre_v2_user == SPECTRE_V2_USER_UNAVAILABLE)
+ return -ENXIO;
+
+ if (spectre_v2_user == SPECTRE_V2_USER_DISABLED)
return -EPERM;
+
if (spectre_v2_user == SPECTRE_V2_USER_STRICT ||
- spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED)
+ spectre_v2_user == SPECTRE_V2_USER_STRICT_PREFERRED ||
+ spectre_v2_user == SPECTRE_V2_USER_SMT_IMPOSSIBLE)
return 0;
task_set_spec_ib_disable(task);
if (ctrl == PR_SPEC_FORCE_DISABLE)
@@ -1241,7 +1257,8 @@ static int ib_prctl_get(struct task_struct *task)
return PR_SPEC_NOT_AFFECTED;
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_UNAVAILABLE:
+ case SPECTRE_V2_USER_DISABLED:
return PR_SPEC_ENABLE;
case SPECTRE_V2_USER_PRCTL:
case SPECTRE_V2_USER_SECCOMP:
@@ -1252,6 +1269,7 @@ static int ib_prctl_get(struct task_struct *task)
return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
case SPECTRE_V2_USER_STRICT:
case SPECTRE_V2_USER_STRICT_PREFERRED:
+ case SPECTRE_V2_USER_SMT_IMPOSSIBLE:
return PR_SPEC_DISABLE;
default:
return PR_SPEC_NOT_AFFECTED;
@@ -1495,8 +1513,13 @@ static char *stibp_state(void)
return "";
switch (spectre_v2_user) {
- case SPECTRE_V2_USER_NONE:
+ case SPECTRE_V2_USER_UNAVAILABLE:
+ return ", STIBP: unavailable";
+ case SPECTRE_V2_USER_DISABLED:
return ", STIBP: disabled";
+ /* Do not display STIBP state if SMT is not possible. */
+ case SPECTRE_V2_USER_SMT_IMPOSSIBLE:
+ return "";
case SPECTRE_V2_USER_STRICT:
return ", STIBP: forced";
case SPECTRE_V2_USER_STRICT_PREFERRED:
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
2019-12-29 16:48 [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented Anthony Steinhauser
2020-01-20 19:02 ` Thomas Gleixner
@ 2020-01-21 11:33 ` Borislav Petkov
2020-01-21 11:57 ` Anthony Steinhauser
1 sibling, 1 reply; 5+ messages in thread
From: Borislav Petkov @ 2020-01-21 11:33 UTC (permalink / raw)
To: Anthony Steinhauser, Thomas Gleixner; +Cc: linux-kernel, mingo
On Sun, Dec 29, 2019 at 08:48:30AM -0800, Anthony Steinhauser wrote:
> @@ -602,7 +603,7 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
> static void __init
> spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> {
> - enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
> + enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_UNAVAILABLE;
> bool smt_possible = IS_ENABLED(CONFIG_SMP);
> enum spectre_v2_user_cmd cmd;
So here in the code, right under this line we check IBPB and STIBP and
whether SMT is force_disabled/possible and set smt_possible if not. We
parse cmdline, pick apart selection, etc...
> @@ -616,6 +617,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> cmd = spectre_v2_parse_user_cmdline(v2_cmd);
> switch (cmd) {
> case SPECTRE_V2_USER_CMD_NONE:
> + mode = SPECTRE_V2_USER_DISABLED;
> goto set_mode;
> case SPECTRE_V2_USER_CMD_FORCE:
> mode = SPECTRE_V2_USER_STRICT;
> @@ -676,7 +678,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> * mode.
> */
> if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> - mode = SPECTRE_V2_USER_NONE;
> + mode = SPECTRE_V2_USER_UNAVAILABLE;
... but here we do that evaluation again. But I think that *if* the
required hw support is not there - either SMT is not possible or STIBP
not present - then there's no real need to parse the cmdline and do all
that.
IOW, the filtering out of the cases where the user can't do any changes
due to not present hw should be concentrated at the function entry and
mode left at SPECTRE_V2_USER_UNAVAILABLE.
IOW 2, unless I'm not missing some of the gazillion use cases with this
;-\ I think that check needs to be moved up and integrated into the
entry checks. I.e., this ontop or a separate patch...:
---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 2e9299816530..ffe5e4fa4611 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -618,8 +618,10 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
return;
if (cpu_smt_control == CPU_SMT_FORCE_DISABLED ||
- cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
+ cpu_smt_control == CPU_SMT_NOT_SUPPORTED) {
smt_possible = false;
+ return;
+ }
cmd = spectre_v2_parse_user_cmdline(v2_cmd);
switch (cmd) {
@@ -679,13 +681,6 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
/* If enhanced IBRS is enabled no STIBP required */
if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED)
return;
-
- /*
- * If SMT is not possible or STIBP is not available clear the STIBP
- * mode.
- */
- if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
- mode = SPECTRE_V2_USER_UNAVAILABLE;
set_mode:
spectre_v2_user = mode;
/* Only print the STIBP mode when SMT possible */
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented
2020-01-21 11:33 ` Borislav Petkov
@ 2020-01-21 11:57 ` Anthony Steinhauser
0 siblings, 0 replies; 5+ messages in thread
From: Anthony Steinhauser @ 2020-01-21 11:57 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Thomas Gleixner, linux-kernel, mingo
Your change seems to remove exactly the distinction which Thomas
pointed out because SPECTRE_V2_USER_UNAVAILABLE would not
differentiate between STIBP mitigation not available and STIBP not
used because SMT is not possible. Otherwise your modification looks
fine to me.
On Tue, Jan 21, 2020 at 3:33 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Sun, Dec 29, 2019 at 08:48:30AM -0800, Anthony Steinhauser wrote:
> > @@ -602,7 +603,7 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
> > static void __init
> > spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> > {
> > - enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE;
> > + enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_UNAVAILABLE;
> > bool smt_possible = IS_ENABLED(CONFIG_SMP);
> > enum spectre_v2_user_cmd cmd;
>
> So here in the code, right under this line we check IBPB and STIBP and
> whether SMT is force_disabled/possible and set smt_possible if not. We
> parse cmdline, pick apart selection, etc...
>
> > @@ -616,6 +617,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> > cmd = spectre_v2_parse_user_cmdline(v2_cmd);
> > switch (cmd) {
> > case SPECTRE_V2_USER_CMD_NONE:
> > + mode = SPECTRE_V2_USER_DISABLED;
> > goto set_mode;
> > case SPECTRE_V2_USER_CMD_FORCE:
> > mode = SPECTRE_V2_USER_STRICT;
> > @@ -676,7 +678,7 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> > * mode.
> > */
> > if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> > - mode = SPECTRE_V2_USER_NONE;
> > + mode = SPECTRE_V2_USER_UNAVAILABLE;
>
> ... but here we do that evaluation again. But I think that *if* the
> required hw support is not there - either SMT is not possible or STIBP
> not present - then there's no real need to parse the cmdline and do all
> that.
>
> IOW, the filtering out of the cases where the user can't do any changes
> due to not present hw should be concentrated at the function entry and
> mode left at SPECTRE_V2_USER_UNAVAILABLE.
>
> IOW 2, unless I'm not missing some of the gazillion use cases with this
> ;-\ I think that check needs to be moved up and integrated into the
> entry checks. I.e., this ontop or a separate patch...:
>
> ---
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 2e9299816530..ffe5e4fa4611 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -618,8 +618,10 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> return;
>
> if (cpu_smt_control == CPU_SMT_FORCE_DISABLED ||
> - cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
> + cpu_smt_control == CPU_SMT_NOT_SUPPORTED) {
> smt_possible = false;
> + return;
> + }
>
> cmd = spectre_v2_parse_user_cmdline(v2_cmd);
> switch (cmd) {
> @@ -679,13 +681,6 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
> /* If enhanced IBRS is enabled no STIBP required */
> if (spectre_v2_enabled == SPECTRE_V2_IBRS_ENHANCED)
> return;
> -
> - /*
> - * If SMT is not possible or STIBP is not available clear the STIBP
> - * mode.
> - */
> - if (!smt_possible || !boot_cpu_has(X86_FEATURE_STIBP))
> - mode = SPECTRE_V2_USER_UNAVAILABLE;
> set_mode:
> spectre_v2_user = mode;
> /* Only print the STIBP mode when SMT possible */
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-21 11:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-29 16:48 [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented Anthony Steinhauser
2020-01-20 19:02 ` Thomas Gleixner
2020-01-21 11:19 ` Anthony Steinhauser
2020-01-21 11:33 ` Borislav Petkov
2020-01-21 11:57 ` Anthony Steinhauser
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox