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 <[email protected]>
---
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
Anthony,
Anthony Steinhauser <[email protected]> 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
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 <[email protected]> wrote:
>
> Anthony,
>
> Anthony Steinhauser <[email protected]> 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
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
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 <[email protected]> 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