Messages in this thread | | | Subject | Re: [PATCH v2] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP | From | Tom Lendacky <> | Date | Thu, 5 Nov 2020 14:32:10 -0600 |
| |
On 11/4/20 11:33 PM, Anand K Mistry wrote: > On AMD CPUs which have the feature X86_FEATURE_AMD_STIBP_ALWAYS_ON, > STIBP is set to on and 'spectre_v2_user_stibp == > SPECTRE_V2_USER_STRICT_PREFERRED'. At the same time, IBPB can be set to > conditional. However, this leads to the case where it's impossible to > turn on IBPB for a process because in the PR_SPEC_DISABLE case in > ib_prctl_set, the (spectre_v2_user_stibp == > SPECTRE_V2_USER_STRICT_PREFERRED) condition leads to a return before the > task flag is set. Similarly, ib_prctl_get will return PR_SPEC_DISABLE > even though IBPB is set to conditional. > > More generally, the following cases are possible: > 1. STIBP = conditional && IBPB = on for spectre_v2_user=seccomp,ibpb > 2. STIBP = on && IBPB = conditional for AMD CPUs with > X86_FEATURE_AMD_STIBP_ALWAYS_ON > > The first case functions correctly today, but only because > spectre_v2_user_ibpb isn't updated to reflect the IBPB mode. > > At a high level, this change does one thing. If either STIBP or IBPB is > set to conditional, allow the prctl to change the task flag. Also, > reflect that capability when querying the state. This isn't perfect > since it doesn't take into account if only STIBP or IBPB is > unconditionally on. But it allows the conditional feature to work as > expected, without affecting the unconditional one. > > Signed-off-by: Anand K Mistry <amistry@google.com>
Does it need a Fixes: tag?
Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
> > --- > > Changes in v2: > - Fix typo in commit message > - s/is_spec_ib_user/is_spec_ib_user_controlled > - Update comment in ib_prctl_set() to reference X86_FEATURE_AMD_STIBP_ALWAYS_ON > - Have is_spec_ib_user_controlled() check both IBPB and STIBP modes > > arch/x86/kernel/cpu/bugs.c | 46 +++++++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index d3f0db463f96..534225afe832 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -1254,6 +1254,14 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl) > return 0; > } > > +static bool is_spec_ib_user_controlled(void) > +{ > + return spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || > + spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || > + spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || > + spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP; > +} > + > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > { > switch (ctrl) { > @@ -1262,13 +1270,20 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return 0; > /* > - * Indirect branch speculation is always disabled in strict > - * mode. It can neither be enabled if it was force-disabled > - * by a previous prctl call. > + * With strict mode for both IBPB and STIBP, the instruction > + * code paths avoid checking this task flag and instead, > + * unconditionally run the instruction. However, STIBP and IBPB > + * are independent and either can be set to conditionally > + * enabled regardless of the mode of the other. If either is set > + * to conditional, allow the task flag to be updated, unless it > + * was force-disabled by a previous prctl call. > + * Currently, this is possible on an AMD CPU which has the > + * feature X86_FEATURE_AMD_STIBP_ALWAYS_ON. In this case, if the > + * kernel is booted with 'spectre_v2_user=seccomp', then > + * spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP and > + * spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED. > */ > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED || > + if (!is_spec_ib_user_controlled() || > task_spec_ib_force_disable(task)) > return -EPERM; > task_clear_spec_ib_disable(task); > @@ -1283,9 +1298,7 @@ static int ib_prctl_set(struct task_struct *task, unsigned long ctrl) > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return -EPERM; > - if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > + if (!is_spec_ib_user_controlled()) > return 0; > task_set_spec_ib_disable(task); > if (ctrl == PR_SPEC_FORCE_DISABLE) > @@ -1351,20 +1364,17 @@ static int ib_prctl_get(struct task_struct *task) > if (spectre_v2_user_ibpb == SPECTRE_V2_USER_NONE && > spectre_v2_user_stibp == SPECTRE_V2_USER_NONE) > return PR_SPEC_ENABLE; > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > - spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > - return PR_SPEC_DISABLE; > - else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_PRCTL || > - spectre_v2_user_ibpb == SPECTRE_V2_USER_SECCOMP || > - spectre_v2_user_stibp == SPECTRE_V2_USER_PRCTL || > - spectre_v2_user_stibp == SPECTRE_V2_USER_SECCOMP) { > + else if (is_spec_ib_user_controlled()) { > if (task_spec_ib_force_disable(task)) > return PR_SPEC_PRCTL | PR_SPEC_FORCE_DISABLE; > if (task_spec_ib_disable(task)) > return PR_SPEC_PRCTL | PR_SPEC_DISABLE; > return PR_SPEC_PRCTL | PR_SPEC_ENABLE; > - } else > + } else if (spectre_v2_user_ibpb == SPECTRE_V2_USER_STRICT || > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT || > + spectre_v2_user_stibp == SPECTRE_V2_USER_STRICT_PREFERRED) > + return PR_SPEC_DISABLE; > + else > return PR_SPEC_NOT_AFFECTED; > } > >
| |