Re: [PATCH 1/1] x86/speculation: Allow IBPB to be conditionally enabled on CPUs with always-on STIBP

[Anand K. Mistry]

On Sun, 1 Nov 2020 at 02:05, Tom Lendacky <thomas.lendacky@xxxxxxx> 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@xxxxxxxxxx>
> >
> > ---
> >
> >   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