Re: [PATCH RESEND] x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS

[KP Singh]

On Mon, Feb 20, 2023 at 4:20 AM KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> >
> > On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
> > > +static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode)
> > > +{
> > > +     /* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
> > > +      *
> > > +      * However, With KERNEL_IBRS, the IBRS bit is cleared on return
> > > +      * to user and the user-mode code needs to be able to enable protection
> > > +      * from cross-thread training, either by always enabling STIBP or
> > > +      * by enabling it via prctl.
> > > +      */
> > > +     return (spectre_v2_in_ibrs_mode(mode) &&
> > > +             !cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));
> > > +}
> >
> > The comments and code confused me, they both seem to imply some
> > distinction between IBRS and KERNEL_IBRS, but in the kernel those are
> > functionally the same thing.  e.g., the kernel doesn't have a user IBRS
> > mode.
> >
> > And, unless I'm missing some subtlety here, it seems to be a convoluted
> > way of saying that eIBRS doesn't need STIBP in user space.

Actually, there is a subtlety, STIBP is not needed in IBRS and eIBRS
however, with KERNEL_IBRS we only enable IBRS (by setting and
unsetting the IBRS bit of SPEC_CTL) in the kernel context and this is
why we need to allow STIBP in userspace. If it were for pure IBRS, we
would not need it either (based on my reading). Now, with
spectre_v2_in_ibrs_mode, as per the current implementation implies
that KERNEL_IBRS is chosen, but this may change. Also, I would also
prefer to have it more readable in the sense that:

"If the kernel decides to write 0 to the IBRS bit on returning to the
user, STIBP needs to to be allowed in user space"

> >
> > It would be simpler to just call it spectre_v2_in_eibrs_mode().
>
> Thanks, yeah this would work too. I was just trying to ensure that, if
> somehow, KERNEL_IBRS gets enabled with SPECTRE_V2_EIBRS, but this does
> not seem to be the case currently. Maybe we should also add a BUG_ON
> to ensure that KERNEL_IBRS does not get enabled in EIBRS mode?
>
> >
> > static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
> > {
> >         return mode == SPECTRE_V2_EIBRS ||
> >                mode == SPECTRE_V2_EIBRS_RETPOLINE ||
> >                mode == SPECTRE_V2_EIBRS_LFENCE;
> > }
> >
> > And then spectre_v2_in_ibrs_mode() could be changed to call that:
> >
> > static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
> > {
> >         return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS;
> > }
> >
> > > @@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)
> > >               break;
> > >
> > >       case SPECTRE_V2_IBRS:
> > > +             pr_err("enabling KERNEL_IBRS");
> >
> > Why?
>
> Removed.
>
> >
> > > @@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
> > >
> > >  static char *stibp_state(void)
> > >  {
> > > -     if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> > > +     if (spectre_v2_user_no_stibp(spectre_v2_enabled))
> > >               return "";
> >
> > This seems like old cruft, can we just remove this check altogether?  In
> > the eIBRS case, spectre_v2_user_stibp will already have its default of
> > SPECTRE_V2_USER_NONE.
> >
> > --
> > Josh