lkml.org 
[lkml]   [2020]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP
On Sun, 1 Nov 2020 at 02:05, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/29/20 1:51 AM, 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 is IBPB is
>
> s/STIBP is IBPB/STIBP or IBPB/

Oops. Will be fixed in v2.

>
> > 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>
> >
> > ---
> >
> > arch/x86/kernel/cpu/bugs.c | 41 +++++++++++++++++++++-----------------
> > 1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index d3f0db463f96..fb64e02eed6f 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -1254,6 +1254,11 @@ static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
> > return 0;
> > }
> >
> > +static bool is_spec_ib_user(enum spectre_v2_user_mitigation mode)
>
> Maybe something like is_spec_ib_user_controlled() would be a better name.

Changed in v2.

>
> > +{
> > + return mode == SPECTRE_V2_USER_PRCTL || mode == SPECTRE_V2_USER_SECCOMP;
> > +}
> > +
>
> I like the idea of passing in the mode you want to check, but it appears
> they are never used independently. The ibpb and stibp modes are always
> checked together in one of the if statements below, so you could make this
> a function that checks both modes and just have a single call. I'll leave
> that up to the maintainers to see what is preferred.

I can see both sides to this. Personally, I think I prefer it as-is
since I think it improves readability a bit by making the conditions
less complicated whilst not hiding too many details. I'll wait to see
what others say before changing this one.

>
> > static int ib_prctl_set(struct task_struct *task, unsigned long ctrl)
> > {
> > switch (ctrl) {
> > @@ -1262,13 +1267,16 @@ 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.
>
> You probably want to reference the STIBP always on mode that allows this
> situation.

Updated comment in v2 to mention the X86_FEATURE_AMD_STIBP_ALWAYS_ON case.

>
> > */
> > - 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(spectre_v2_user_ibpb) &&
> > + !is_spec_ib_user(spectre_v2_user_stibp)) ||
> > task_spec_ib_force_disable(task))
> > return -EPERM;
> > task_clear_spec_ib_disable(task);
> > @@ -1283,9 +1291,8 @@ 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(spectre_v2_user_ibpb) &&
> > + !is_spec_ib_user(spectre_v2_user_stibp))
>
> The set function seems reasonable to me.
>
> > return 0;
> > task_set_spec_ib_disable(task);
> > if (ctrl == PR_SPEC_FORCE_DISABLE)
> > @@ -1351,20 +1358,18 @@ 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(spectre_v2_user_ibpb) ||
> > + is_spec_ib_user(spectre_v2_user_stibp)) {
> > 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
>
> The get function also seems reasonable.
>
> Lets hear what some of the other folks that are familiar with this area
> have to say.
>
> Thanks,
> Tom
>
> > return PR_SPEC_NOT_AFFECTED;
> > }
> >
> >



--
Anand K. Mistry
Software Engineer
Google Australia

\
 
 \ /
  Last update: 2020-11-02 01:04    [W:0.089 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site