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

  With the introduction of KERNEL_IBRS, STIBP is no longer needed
to prevent cross thread training in the kernel space. When KERNEL_IBRS
was added, it also disabled the user-mode protections for spectre_v2.
KERNEL_IBRS does not mitigate cross thread training in the userspace.

In order to demonstrate the issue, one needs to avoid syscalls in the
victim as syscalls can shorten the window size due to
a user -> kernel -> user transition which sets the
IBRS bit when entering kernel space and clearing any training the
attacker may have done.

Allow users to select a spectre_v2_user mitigation (STIBP always on,
opt-in via prctl) when KERNEL_IBRS is enabled.

Reported-by: José Oliveira <joseloliveira11@gmail.com>
Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
Reviewed-by: Alexandra Sandulescu <aesa@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
Cc: stable@vger.kernel.org
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)
  

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.

It would be simpler to just call it spectre_v2_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?

> @@ -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.
  

On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <jpoimboe@kernel.org> 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.
>
> 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
  

On Mon, Feb 20, 2023 at 4:20 AM KP Singh <kpsingh@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <jpoimboe@kernel.org> 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
  

Drop stable@.

On Mon, Feb 20, 2023 at 04:34:02AM -0800, KP Singh 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"

From:

https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/indirect-branch-restricted-speculation.html

"If IA32_SPEC_CTRL.IBRS is already 1 before a transition to a more
privileged predictor mode, some processors may allow the predicted
targets of indirect branches executed in that predictor mode to be
controlled by software that executed before the transition. Software can
avoid this by using WRMSR on the IA32_SPEC_CTRL MSR to set the IBRS bit
to 1 after any such transition, regardless of the bit’s previous value.
It is not necessary to clear the bit first; writing it with a value of
1 after the transition suffices, regardless of the bit’s original
value."

I'd love it if we could simplify our code by not caring of the IBRS bit
when returning to user but I'm afraid that it ain't that simple.

This above probably wants to say that you need to write 1 on CPL change
because it has a flushing behavior of killing user prediction entries.

Lemme add Andy and dhansen for clarification.
  

On 2/20/23 06:31, Borislav Petkov wrote:
> This above probably wants to say that you need to write 1 on CPL change
> because it has a flushing behavior of killing user prediction entries.

Right.  The naive way of looking at IBRS is that setting IBRS=1
mitigates spectre-v2.  But, as the documentation says, just _leaving_ it
set to 1 is not good enough.  It must be actively rewritten in order to
get the strongest semantics.

It's still rather early in the morning, but I'm also quite confused
about what exactly the problem is here.  The patch and the changelog
aren't especially clear.  I'll need to stare at it with a cup of coffee
before I can give any coherent better suggestions, though.
  

On Mon, Feb 20, 2023 at 04:34:02AM -0800, KP Singh wrote:
> On Mon, Feb 20, 2023 at 4:20 AM KP Singh <kpsingh@kernel.org> wrote:
> >
> > On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <jpoimboe@kernel.org> 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"

We will never enable IBRS in user space.  We tried that years ago and it
was very slow.
  

On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
> We will never enable IBRS in user space.  We tried that years ago and it
> was very slow.

Then I don't know what this is trying to fix.

We'd need a proper bug statement in the commit message what the problem
is. As folks have established, the hw vuln mess is a whole universe of
crazy. So without proper documenting, this spaghetti madness will be
completely unreadable.

Thx.
  

On Mon, Feb 20, 2023 at 06:46:04PM +0100, Borislav Petkov wrote:
> On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
> > We will never enable IBRS in user space.  We tried that years ago and it
> > was very slow.
> 
> Then I don't know what this is trying to fix.
> 
> We'd need a proper bug statement in the commit message what the problem
> is. As folks have established, the hw vuln mess is a whole universe of
> crazy. So without proper documenting, this spaghetti madness will be
> completely unreadable.

Agreed, and there seems to be a lot of confusion around this patch.  I
think I understand the issue, let me write up a new patch which is
hopefully clearer.
  

On Mon, Feb 20, 2023 at 9:59 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 06:46:04PM +0100, Borislav Petkov wrote:
> > On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
> > > We will never enable IBRS in user space.  We tried that years ago and it
> > > was very slow.
> >
> > Then I don't know what this is trying to fix.
> >
> > We'd need a proper bug statement in the commit message what the problem
> > is. As folks have established, the hw vuln mess is a whole universe of
> > crazy. So without proper documenting, this spaghetti madness will be
> > completely unreadable.
>
> Agreed, and there seems to be a lot of confusion around this patch.  I
> think I understand the issue, let me write up a new patch which is
> hopefully clearer.

Feel free to write a patch, but I don't get the confusion.

Well, we disable IBRS userspace (this is KENREL_IBRS), because it is
slow. Now if a user space process wants to protect itself from cross
thread training, it should be able to do it, either by turning STIBP
always on or using a prctl to enable. With the current logic, it's
unable to do so. All of this is mentioned in the commit message.

>
> --
> Josh
  

On Mon, Feb 20, 2023 at 10:01:57AM -0800, KP Singh wrote:
> Well, we disable IBRS userspace (this is KENREL_IBRS), because it is
> slow. Now if a user space process wants to protect itself from cross
> thread training, it should be able to do it, either by turning STIBP
> always on or using a prctl to enable. With the current logic, it's
> unable to do so.

Ofcourse it can:

	[SPECTRE_V2_USER_PRCTL]                 = "User space: Mitigation: STIBP via prctl",

we did this at the time so that a userspace process can control it via
prctl().

So, maybe you should explain what you're trying to accomplish in detail
and where it fails...
  

On Mon, Feb 20, 2023 at 10:22 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Feb 20, 2023 at 10:01:57AM -0800, KP Singh wrote:
> > Well, we disable IBRS userspace (this is KENREL_IBRS), because it is
> > slow. Now if a user space process wants to protect itself from cross
> > thread training, it should be able to do it, either by turning STIBP
> > always on or using a prctl to enable. With the current logic, it's
> > unable to do so.
>
> Ofcourse it can:
>
>         [SPECTRE_V2_USER_PRCTL]                 = "User space: Mitigation: STIBP via prctl",
>
> we did this at the time so that a userspace process can control it via
> prctl().

No it cannot with IBRS which is really just KERNEL_IBRS enabled, we
bail out if spectre_v2_in_inbrs_mode and ignore any selections:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/cpu/bugs.c#n1202

The whole confusion spews from the fact that while the user thinks
they are enabling spectre_v2=ibrs, they only really get KERNEL_IBRS
and IBRS is dropped in userspace. This in itself seems like a decision
the kernel implicitly took on behalf of the user. Now it also took
their ability to enable spectre_v2_user in this case, which is what
this patch is fixing.


>
> So, maybe you should explain what you're trying to accomplish in detail
> and where it fails...
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
  

On Mon, Feb 20, 2023 at 10:44:21AM -0800, KP Singh wrote:
> No it cannot with IBRS which is really just KERNEL_IBRS enabled, we

See my other reply. The intent is there to be able to do it. What needs
to be figured out now is *why* we said no STIBP with IBRS? Was it an
omission or was there some intent behind it.
  

On Mon, Feb 20, 2023 at 10:52 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Feb 20, 2023 at 10:44:21AM -0800, KP Singh wrote:
> > No it cannot with IBRS which is really just KERNEL_IBRS enabled, we
>
> See my other reply. The intent is there to be able to do it. What needs
> to be figured out now is *why* we said no STIBP with IBRS? Was it an
> omission or was there some intent behind it.
>

Sure, it looks like an omission to me, we wrote a POC on Skylake that
was able to do cross-thread training with the current set of
mitigations.

STIBP with IBRS is still correct if spectre_v2=ibrs had really meant
IBRS everywhere, but just means KERNEL_IBRS, which means only kernel
is protected, userspace is still unprotected.

> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
  

On Mon, Feb 20, 2023 at 10:56:38AM -0800, KP Singh wrote:
> Sure, it looks like an omission to me, we wrote a POC on Skylake that
> was able to do cross-thread training with the current set of
> mitigations.

Right.

> STIBP with IBRS is still correct if spectre_v2=ibrs had really meant
> IBRS everywhere,

Yeah, IBRS everywhere got shot down as a no-no very early in the game,
for apparent reasons.

> but just means KERNEL_IBRS, which means only kernel is protected,
> userspace is still unprotected.

Yes, that was always the intent with IBRS: enable on kernel entry and
disable on exit.

Thx.
  

On Mon, Feb 20, 2023 at 11:02 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Feb 20, 2023 at 10:56:38AM -0800, KP Singh wrote:
> > Sure, it looks like an omission to me, we wrote a POC on Skylake that
> > was able to do cross-thread training with the current set of
> > mitigations.
>
> Right.
>
> > STIBP with IBRS is still correct if spectre_v2=ibrs had really meant
> > IBRS everywhere,
>
> Yeah, IBRS everywhere got shot down as a no-no very early in the game,
> for apparent reasons.

As you said in the other thread, this needs to be documented both in
the code and the kernel documentation.

>
> > but just means KERNEL_IBRS, which means only kernel is protected,
> > userspace is still unprotected.
>
> Yes, that was always the intent with IBRS: enable on kernel entry and
> disable on exit.
>
> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
  

On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> "When IBRS or enhanced IBRS is enabled, STIBP is not needed."
> 
> This is misleading, if not strictly wrong.  The IBRS bit being set
> implies STIBP, which reads differently to "not needed".
> 
> 
> Now - eIBRS is "set once at start of day" which ends up becoming a
> global implicit STIBP.

Right.

> I think we're discussing the legacy IBRS case here.  i.e. what was
> retrofitted in microcode for existing parts?

Any IBRS actually. The one which is *not* the automatic, fire'n'forget
thing.

> The reason why it is "write 1 on each privilege increase, 0 on privilege
> decrease" is because on some CPUs its an inhibit control, and on some
> CPUs is a flush (i.e. its actually IBPB).
> 
> But these same CPUs don't actually have an ability to thread-tag the
> indirect predictor nicely so STIBP is also horribly expensive under the
> hood - so much so that we were firmly recommended to clear STIBP/IBRS
> when going idle so as to reduce the impact on the sibling.

Yap, we do that. And we do the write to 0 for IBRS on exit to
luserspace, probably for very similar reasons.

> IMO the proper way to do this is to set STIBP uniformly depending on
> whether you want it in userspace or not, and treat it logically
> separately to IBRS.  It doesn't hurt (any more) to have both bits set.

So we have this thing:

        /*
         * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
         * STIBP is not required.
         */
        if (!boot_cpu_has(X86_FEATURE_STIBP) ||
            !smt_possible ||
            spectre_v2_in_ibrs_mode(spectre_v2_enabled))
                return;

What you propose sounds cleaner but would definitely need more massaging
of this madness code. So I guess we could do only the
enable-STIBP-when-IBRS-enabled thing first and do more cleanups later.

Thx.
  

On Mon, Feb 20, 2023 at 1:10 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> > "When IBRS or enhanced IBRS is enabled, STIBP is not needed."
> >
> > This is misleading, if not strictly wrong.  The IBRS bit being set
> > implies STIBP, which reads differently to "not needed".
> >
> >
> > Now - eIBRS is "set once at start of day" which ends up becoming a
> > global implicit STIBP.
>
> Right.
>
> > I think we're discussing the legacy IBRS case here.  i.e. what was
> > retrofitted in microcode for existing parts?
>
> Any IBRS actually. The one which is *not* the automatic, fire'n'forget
> thing.
>
> > The reason why it is "write 1 on each privilege increase, 0 on privilege
> > decrease" is because on some CPUs its an inhibit control, and on some
> > CPUs is a flush (i.e. its actually IBPB).
> >
> > But these same CPUs don't actually have an ability to thread-tag the
> > indirect predictor nicely so STIBP is also horribly expensive under the
> > hood - so much so that we were firmly recommended to clear STIBP/IBRS
> > when going idle so as to reduce the impact on the sibling.
>
> Yap, we do that. And we do the write to 0 for IBRS on exit to
> luserspace, probably for very similar reasons.
>
> > IMO the proper way to do this is to set STIBP uniformly depending on
> > whether you want it in userspace or not, and treat it logically
> > separately to IBRS.  It doesn't hurt (any more) to have both bits set.
>
> So we have this thing:
>
>         /*
>          * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
>          * STIBP is not required.
>          */
>         if (!boot_cpu_has(X86_FEATURE_STIBP) ||
>             !smt_possible ||
>             spectre_v2_in_ibrs_mode(spectre_v2_enabled))
>                 return;
>
> What you propose sounds cleaner but would definitely need more massaging
> of this madness code. So I guess we could do only the
> enable-STIBP-when-IBRS-enabled thing first and do more cleanups later.
>

I do like the idea of decoupling, but yeah this is a bit tangled and
abstracted away from the user. The user currently just selects one of
(note the absence of STIBP in the choices here).

SPECTRE_V2_USER_CMD_NONE,
SPECTRE_V2_USER_CMD_AUTO,
SPECTRE_V2_USER_CMD_FORCE,
SPECTRE_V2_USER_CMD_PRCTL,
SPECTRE_V2_USER_CMD_PRCTL_IBPB,
SPECTRE_V2_USER_CMD_SECCOMP,
SPECTRE_V2_USER_CMD_SECCOMP_IBPB,

and the STIBP mode is selected implicitly based on the kernel's choice
of spectre v2 mitigations. I will fix the default case and we can
eventually decouple STIBP from the v2 kernel mitigation choice.

> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
  

On Mon, Feb 20, 2023 at 3:31 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 20/02/2023 9:10 pm, Borislav Petkov wrote:
> > On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> >> I think we're discussing the legacy IBRS case here.  i.e. what was
> >> retrofitted in microcode for existing parts?
> > Any IBRS actually. The one which is *not* the automatic, fire'n'forget
> > thing.
>
> /sigh so we're still talking about 3 different things then.
>
> 1) Intel's legacy IBRS
> 2) AMD's regular IBRS
> 3) AMD's AutoIBRS
>
> which all have different relevant behaviours for userspace.  Just so
> it's written out coherently in at least one place...
>
> When SEV-SNP is enabled in firmware, whether or not it's being used by
> software, AutoIBRS keeps indirect predictions inhibited in all of
> ASID0.  That's all host userspace to the non-hypervisor devs reading
> this thread.
>
> For any AMD configuration setting STIBP, there must be an IBPB after
> having set STIBP.   Setting STIBP alone does not evict previously
> created shared predictions.  This one can go subtly wrong for anyone who
> assumes that Intel STIBP and AMD STIBP have the same behaviour.

This is very useful, but I think this is also why the STIBP and IBPB's
conditionals seemed to be tangled together. The prctl / seccomp code
should set STIBP and trigger an IBPB.

I took a stab at the documentation piece, Andrew and others could you
help me with a review and suggestions?

diff --git a/Documentation/admin-guide/hw-vuln/spectre.rst
b/Documentation/admin-guide/hw-vuln/spectre.rst
index c4dcdb3d0d45..d7003bbc82f6 100644
--- a/Documentation/admin-guide/hw-vuln/spectre.rst
+++ b/Documentation/admin-guide/hw-vuln/spectre.rst
@@ -479,8 +479,17 @@ Spectre variant 2
    On Intel Skylake-era systems the mitigation covers most, but not all,
    cases. See :ref:`[3] <spec_ref3>` for more details.

-   On CPUs with hardware mitigation for Spectre variant 2 (e.g. Enhanced
-   IBRS on x86), retpoline is automatically disabled at run time.
+   On CPUs with hardware mitigation for Spectre variant 2 (e.g. IBRS
+   or enhanced IBRS on x86), retpoline is automatically disabled at run time.
+
+   Setting the IBRS bit implicitly enables STIBP which guards against
+   cross-thread branch target injection on SMT systems. On systems
with enhanced
+   IBRS, the kernel sets the bit once, which keeps cross-thread protections
+   always enabled, obviating the need for an explicit STIBP. On CPUs
with legacy
+   IBRS, the kernel clears the IBRS bit on returning to user-space, thus also
+   disabling the implicit STIBP. Consequently, STIBP needs to be explicitly
+   enabled to guard against cross-thread attacks in userspace.
+

    The retpoline mitigation is turned on by default on vulnerable
    CPUs. It can be forced on or off by the administrator
@@ -504,9 +513,12 @@ Spectre variant 2
    For Spectre variant 2 mitigation, individual user programs
    can be compiled with return trampolines for indirect branches.
    This protects them from consuming poisoned entries in the branch
-   target buffer left by malicious software.  Alternatively, the
-   programs can disable their indirect branch speculation via prctl()
-   (See :ref:`Documentation/userspace-api/spec_ctrl.rst <set_spec_ctrl>`).
+   target buffer left by malicious software.
+
+   On legacy IBRS systems where the IBRS bit is cleared and thus disabling the
+   implicit STIBP on returning to userspace, the programs can disable their
+   indirect branch speculation via prctl() (See
+   :ref:`Documentation/userspace-api/spec_ctrl.rst <set_spec_ctrl>`).
    On x86, this will turn on STIBP to guard against attacks from the
    sibling thread when the user program is running, and use IBPB to
    flush the branch target buffer when switching to/from the program.

>
> Furthermore, extra care needs taking on vmexit because transitioning
> from the guest STIBP setting to the host STIBP setting can leave shared
> predictions intact.



>
> ~Andrew
  

On Mon, Feb 20, 2023 at 11:30:46PM +0000, Andrew Cooper wrote:
> 1) Intel's legacy IBRS
> 2) AMD's regular IBRS

Yeah, we don't do that in the kernel.

> 3) AMD's AutoIBRS
> 
> which all have different relevant behaviours for userspace.  Just so
> it's written out coherently in at least one place...
> 
> When SEV-SNP is enabled in firmware, whether or not it's being used by
> software, AutoIBRS keeps indirect predictions inhibited in all of
> ASID0.  That's all host userspace to the non-hypervisor devs reading
> this thread.

Yap.

> For any AMD configuration setting STIBP, there must be an IBPB after
> having set STIBP.   Setting STIBP alone does not evict previously
> created shared predictions.  This one can go subtly wrong for anyone who
> assumes that Intel STIBP and AMD STIBP have the same behaviour.

We will IBPB eventually... on the next context switch.

> Furthermore, extra care needs taking on vmexit because transitioning
> from the guest STIBP setting to the host STIBP setting can leave shared
> predictions intact.

From what I can tell from looking at the SVM code, we don't do anything
special there when restoring SPEC_CTRL.
  

On Mon, Feb 20, 2023 at 3:45 PM KP Singh <kpsingh@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 3:31 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > On 20/02/2023 9:10 pm, Borislav Petkov wrote:
> > > On Mon, Feb 20, 2023 at 07:57:25PM +0000, Andrew Cooper wrote:
> > >> I think we're discussing the legacy IBRS case here.  i.e. what was
> > >> retrofitted in microcode for existing parts?
> > > Any IBRS actually. The one which is *not* the automatic, fire'n'forget
> > > thing.
> >
> > /sigh so we're still talking about 3 different things then.
> >
> > 1) Intel's legacy IBRS
> > 2) AMD's regular IBRS
> > 3) AMD's AutoIBRS
> >
> > which all have different relevant behaviours for userspace.  Just so
> > it's written out coherently in at least one place...
> >
> > When SEV-SNP is enabled in firmware, whether or not it's being used by
> > software, AutoIBRS keeps indirect predictions inhibited in all of
> > ASID0.  That's all host userspace to the non-hypervisor devs reading
> > this thread.
> >
> > For any AMD configuration setting STIBP, there must be an IBPB after
> > having set STIBP.   Setting STIBP alone does not evict previously
> > created shared predictions.  This one can go subtly wrong for anyone who
> > assumes that Intel STIBP and AMD STIBP have the same behaviour.
>
> This is very useful, but I think this is also why the STIBP and IBPB's
> conditionals seemed to be tangled together. The prctl / seccomp code
> should set STIBP and trigger an IBPB.
>
> I took a stab at the documentation piece, Andrew and others could you
> help me with a review and suggestions?
>
> diff --git a/Documentation/admin-guide/hw-vuln/spectre.rst
> b/Documentation/admin-guide/hw-vuln/spectre.rst
> index c4dcdb3d0d45..d7003bbc82f6 100644
> --- a/Documentation/admin-guide/hw-vuln/spectre.rst
> +++ b/Documentation/admin-guide/hw-vuln/spectre.rst
> @@ -479,8 +479,17 @@ Spectre variant 2
>     On Intel Skylake-era systems the mitigation covers most, but not all,
>     cases. See :ref:`[3] <spec_ref3>` for more details.
>
> -   On CPUs with hardware mitigation for Spectre variant 2 (e.g. Enhanced
> -   IBRS on x86), retpoline is automatically disabled at run time.
> +   On CPUs with hardware mitigation for Spectre variant 2 (e.g. IBRS
> +   or enhanced IBRS on x86), retpoline is automatically disabled at run time.
> +
> +   Setting the IBRS bit implicitly enables STIBP which guards against
> +   cross-thread branch target injection on SMT systems. On systems
> with enhanced
> +   IBRS, the kernel sets the bit once, which keeps cross-thread protections
> +   always enabled, obviating the need for an explicit STIBP. On CPUs
> with legacy
> +   IBRS, the kernel clears the IBRS bit on returning to user-space, thus also
> +   disabling the implicit STIBP. Consequently, STIBP needs to be explicitly
> +   enabled to guard against cross-thread attacks in userspace.
> +
>
>     The retpoline mitigation is turned on by default on vulnerable
>     CPUs. It can be forced on or off by the administrator
> @@ -504,9 +513,12 @@ Spectre variant 2
>     For Spectre variant 2 mitigation, individual user programs
>     can be compiled with return trampolines for indirect branches.
>     This protects them from consuming poisoned entries in the branch
> -   target buffer left by malicious software.  Alternatively, the
> -   programs can disable their indirect branch speculation via prctl()
> -   (See :ref:`Documentation/userspace-api/spec_ctrl.rst <set_spec_ctrl>`).
> +   target buffer left by malicious software.
> +
> +   On legacy IBRS systems where the IBRS bit is cleared and thus disabling the
> +   implicit STIBP on returning to userspace, the programs can disable their
> +   indirect branch speculation via prctl() (See
> +   :ref:`Documentation/userspace-api/spec_ctrl.rst <set_spec_ctrl>`).
>     On x86, this will turn on STIBP to guard against attacks from the
>     sibling thread when the user program is running, and use IBPB to
>     flush the branch target buffer when switching to/from the program.
>

I sent a new revision in
https://lore.kernel.org/lkml/20230221184908.2349578-1-kpsingh@kernel.org/T/#t
(I forgot to Cc Andrew, I would appreciate if you can have a look) and
I should not have added stable yet (mentioning it before it gets
called out)