lkml.org 
[lkml]   [2020]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP
From
Date
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;
> }
>
>

\
 
 \ /
  Last update: 2020-11-05 21:33    [W:0.058 / U:0.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site