Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented

From: Anthony Steinhauser
Date: Tue Jan 21 2020 - 06:19:25 EST

Next message: Alexandru Ardelean: "[PATCH 2/3] dt-bindings: iio: frequency: Add docs for ADF4360 PLL"
Previous message: Pengcheng Yang: "[PATCH] kernel/relay.c: fix read_pos error when multiple readers"
In reply to: Thomas Gleixner: "Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented"
Next in thread: Borislav Petkov: "Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx> wrote:
>
> Anthony,
>
> Anthony Steinhauser <asteinhauser@xxxxxxxxxx> 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
From 561159a358e91ce4959f06836b0aa6147f8dd346 Mon Sep 17 00:00:00 2001
From: Anthony Steinhauser <asteinhauser@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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

Next message: Alexandru Ardelean: "[PATCH 2/3] dt-bindings: iio: frequency: Add docs for ADF4360 PLL"
Previous message: Pengcheng Yang: "[PATCH] kernel/relay.c: fix read_pos error when multiple readers"
In reply to: Thomas Gleixner: "Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented"
Next in thread: Borislav Petkov: "Re: [PATCH] Return ENXIO instead of EPERM when speculation control is unimplemented"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]