x86/bugs: Allow STIBP with IBRS

  IBRS is only enabled in kernel space.  Since it's not enabled in user
space, user space isn't protected from indirect branch prediction
attacks from a sibling CPU thread.

Allow STIBP to be enabled to protect against such attacks.

Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
Reported-by: José Oliveira <joseloliveira11@gmail.com>
Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
  

On Mon, Feb 20, 2023 at 10:27 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> IBRS is only enabled in kernel space.  Since it's not enabled in user
> space, user space isn't protected from indirect branch prediction
> attacks from a sibling CPU thread.
>
> Allow STIBP to be enabled to protect against such attacks.
>
> Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> Reported-by: José Oliveira <joseloliveira11@gmail.com>
> Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> ---
>  arch/x86/kernel/cpu/bugs.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 85168740f76a..b97c0d28e573 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1124,14 +1124,19 @@ spectre_v2_parse_user_cmdline(void)
>         return SPECTRE_V2_USER_CMD_AUTO;
>  }
>
> -static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
> +static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
>  {
> -       return mode == SPECTRE_V2_IBRS ||
> -              mode == SPECTRE_V2_EIBRS ||
> +       return mode == SPECTRE_V2_EIBRS ||
>                mode == SPECTRE_V2_EIBRS_RETPOLINE ||
>                mode == SPECTRE_V2_EIBRS_LFENCE;
>  }

There are no comments here, this code is in dire need for some
comments and explanation, I was trying something like:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bca0bd8f4846..3e04f9fa68a8 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1124,14 +1124,31 @@ spectre_v2_parse_user_cmdline(void)
        return SPECTRE_V2_USER_CMD_AUTO;
 }

-static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
+static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
 {
-       return mode == SPECTRE_V2_IBRS ||
-              mode == SPECTRE_V2_EIBRS ||
+       return mode == SPECTRE_V2_EIBRS ||
               mode == SPECTRE_V2_EIBRS_RETPOLINE ||
               mode == SPECTRE_V2_EIBRS_LFENCE;
 }

+/*
+ * In IBRS mode, the spectre_v2 mitigation is enabled only in kernel space with
+ * the IBRS bit being cleared on return to userspace due to performance
+ * overhead.
+ */
+static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
+{
+       return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS;
+}
+
+/*
+ * User mode protections are only available in eIBRS mode.
+ */
+static inline bool spectre_v2_user_needs_stibp(enum spectre_v2_mitigation mode)
+{
+       return !spectre_v2_in_eibrs_mode(mode);
+}
+
 static void __init
 spectre_v2_user_select_mitigation(void)
 {
@@ -1193,13 +1210,8 @@ spectre_v2_user_select_mitigation(void)
                        "always-on" : "conditional");
        }

-       /*
-        * 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))
+       if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible ||
+           !spectre_v2_user_needs_stibp(spectre_v2_enabled))
                return;

        /*
@@ -2327,7 +2339,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_needs_stibp(spectre_v2_enabled))
                return "";

        switch (spectre_v2_user_stibp) {

Also Josh, is it okay for us to have a discussion and have me write
the patch as a v2? Your current patch does not even credit me at all.
Seems a bit unfair, but I don't really care. I was going to rev up the
patch with your suggestions.

>
> +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
> +{
> +       return spectre_v2_in_eibrs_mode(mode) ||
> +              mode == SPECTRE_V2_IBRS;
> +}
> +
>  static void __init
>  spectre_v2_user_select_mitigation(void)
>  {
> @@ -1194,12 +1199,12 @@ spectre_v2_user_select_mitigation(void)
>         }
>
>         /*
> -        * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
> +        * If no STIBP, 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))
> +           spectre_v2_in_eibrs_mode(spectre_v2_enabled))
>                 return;
>
>         /*
> @@ -2327,9 +2332,6 @@ static ssize_t mmio_stale_data_show_state(char *buf)
>
>  static char *stibp_state(void)
>  {
> -       if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> -               return "";
> -
>         switch (spectre_v2_user_stibp) {
>         case SPECTRE_V2_USER_NONE:
>                 return ", STIBP: disabled";
> --
> 2.39.1
>
  

Drop stable@ again.

On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote:
> IBRS is only enabled in kernel space.  Since it's not enabled in user
> space, user space isn't protected from indirect branch prediction
> attacks from a sibling CPU thread.
> 
> Allow STIBP to be enabled to protect against such attacks.
> 
> Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")

Yah, look at that one:

commit 7c693f54c873691a4b7da05c7e0f74e67745d144
Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Date:   Tue Jun 14 23:15:55 2022 +0200

    x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS

    Extend spectre_v2= boot option with Kernel IBRS.

    [jpoimboe: no STIBP with IBRS]
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I'm assuming this was supposed to mean no STIBP in *kernel mode* when
IBRS is selected?

In user mode, STIBP should be selectable as we disable IBRS there.

Close?

If so, pls document it too while at it:

Documentation/admin-guide/hw-vuln/spectre.rst

because we will be wondering next time again.

Like we wonder each time this madness is being touched. ;-(

Thx.
  

On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote:
>  static char *stibp_state(void)
>  {
> -       if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> +       if (!spectre_v2_user_needs_stibp(spectre_v2_enabled))
>                 return "";
> 
>         switch (spectre_v2_user_stibp) {
> 
> Also Josh, is it okay for us to have a discussion and have me write
> the patch as a v2? Your current patch does not even credit me at all.
> Seems a bit unfair, but I don't really care. I was going to rev up the
> patch with your suggestions.

Well, frankly the patch needed a complete rewrite.  The patch
description was unclear about what the problem is and what's being
fixed.  The code was obtuse and the comments didn't help.  I could tell
by the other replies that I wasn't the only one confused.

I can give you Reported-by, or did you have some other tag in mind?
  

On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote:
> >  static char *stibp_state(void)
> >  {
> > -       if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> > +       if (!spectre_v2_user_needs_stibp(spectre_v2_enabled))
> >                 return "";
> >
> >         switch (spectre_v2_user_stibp) {
> >
> > Also Josh, is it okay for us to have a discussion and have me write
> > the patch as a v2? Your current patch does not even credit me at all.
> > Seems a bit unfair, but I don't really care. I was going to rev up the
> > patch with your suggestions.
>
> Well, frankly the patch needed a complete rewrite.  The patch
> description was unclear about what the problem is and what's being

Josh, this is a complex issue, we are figuring it out together on the
list. It's complex, that's why folks got it wrong in the first place.
Calling the patch obtuse and unclear is unfair!

> fixed.  The code was obtuse and the comments didn't help.  I could tell
> by the other replies that I wasn't the only one confused.

The patch you sent is not clear either, it implicitly ties in STIBP
with eIBRS. There is no explanation anywhere that IBRS just means
KERNEL_IBRS.

>
> I can give you Reported-by, or did you have some other tag in mind?
>
> --
> Josh
  

On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote:
> Drop stable@ again.
> 
> On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote:
> > IBRS is only enabled in kernel space.  Since it's not enabled in user
> > space, user space isn't protected from indirect branch prediction
> > attacks from a sibling CPU thread.
> > 
> > Allow STIBP to be enabled to protect against such attacks.
> > 
> > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> 
> Yah, look at that one:
> 
> commit 7c693f54c873691a4b7da05c7e0f74e67745d144
> Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Date:   Tue Jun 14 23:15:55 2022 +0200
> 
>     x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> 
>     Extend spectre_v2= boot option with Kernel IBRS.
> 
>     [jpoimboe: no STIBP with IBRS]
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I'm assuming this was supposed to mean no STIBP in *kernel mode* when
> IBRS is selected?

No it was supposed to be "no STIBP with *eIBRS*".

> In user mode, STIBP should be selectable as we disable IBRS there.
> 
> Close?
> 
> If so, pls document it too while at it:
> 
> Documentation/admin-guide/hw-vuln/spectre.rst
> 
> because we will be wondering next time again.
> 
> Like we wonder each time this madness is being touched. ;-(

As far as I can tell, that document was never updated to describe
spectre_v2=ibrs in the first place.  That would be a whole 'nother patch
which I'm not volunteering for.  Nice try ;-)
  

On Mon, Feb 20, 2023 at 11:09 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote:
> > Drop stable@ again.
> >
> > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote:
> > > IBRS is only enabled in kernel space.  Since it's not enabled in user
> > > space, user space isn't protected from indirect branch prediction
> > > attacks from a sibling CPU thread.
> > >
> > > Allow STIBP to be enabled to protect against such attacks.
> > >
> > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> >
> > Yah, look at that one:
> >
> > commit 7c693f54c873691a4b7da05c7e0f74e67745d144
> > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Date:   Tue Jun 14 23:15:55 2022 +0200
> >
> >     x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> >
> >     Extend spectre_v2= boot option with Kernel IBRS.
> >
> >     [jpoimboe: no STIBP with IBRS]
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > I'm assuming this was supposed to mean no STIBP in *kernel mode* when
> > IBRS is selected?
>
> No it was supposed to be "no STIBP with *eIBRS*".
>
> > In user mode, STIBP should be selectable as we disable IBRS there.
> >
> > Close?
> >
> > If so, pls document it too while at it:
> >
> > Documentation/admin-guide/hw-vuln/spectre.rst
> >
> > because we will be wondering next time again.
> >
> > Like we wonder each time this madness is being touched. ;-(
>
> As far as I can tell, that document was never updated to describe
> spectre_v2=ibrs in the first place.  That would be a whole 'nother patch
> which I'm not volunteering for.  Nice try ;-)

This should at least be documented in the code.

Now it seems like it is not okay to work with people on the list and
just send revisions bypassing them. This is not something we do in the
kernel area I come from (an x86 favorite ;)).  Please feel free to go
with Josh's version (or its future revisions). If you want me to
re-spin with some comments, happy to. If not, please do at least give
me Reported-by here.



>
> --
> Josh
  

On Mon, Feb 20, 2023 at 11:04:56AM -0800, KP Singh wrote:
> On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote:
> > >  static char *stibp_state(void)
> > >  {
> > > -       if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
> > > +       if (!spectre_v2_user_needs_stibp(spectre_v2_enabled))
> > >                 return "";
> > >
> > >         switch (spectre_v2_user_stibp) {
> > >
> > > Also Josh, is it okay for us to have a discussion and have me write
> > > the patch as a v2? Your current patch does not even credit me at all.
> > > Seems a bit unfair, but I don't really care. I was going to rev up the
> > > patch with your suggestions.
> >
> > Well, frankly the patch needed a complete rewrite.  The patch
> > description was unclear about what the problem is and what's being
> 
> Josh, this is a complex issue, we are figuring it out together on the
> list. It's complex, that's why folks got it wrong in the first place.
> Calling the patch obtuse and unclear is unfair!
> 
> > fixed.  The code was obtuse and the comments didn't help.  I could tell
> > by the other replies that I wasn't the only one confused.
> 
> The patch you sent is not clear either, it implicitly ties in STIBP
> with eIBRS. There is no explanation anywhere that IBRS just means
> KERNEL_IBRS.

Ok, so something like this on top?

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b97c0d28e573..fb3079445700 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1201,6 +1201,10 @@ spectre_v2_user_select_mitigation(void)
 	/*
 	 * If no STIBP, enhanced IBRS is enabled, or SMT impossible,
 	 * STIBP is not required.
+	 *
+	 * For legacy IBRS, STIBP may still be needed because IBRS is only
+	 * enabled in kernel space, so user space isn't protected from indirect
+	 * branch prediction attacks from a sibling CPU thread.
 	 */
 	if (!boot_cpu_has(X86_FEATURE_STIBP) ||
 	    !smt_possible ||
  

On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote:
> No it was supposed to be "no STIBP with *eIBRS*".

Ok, good.

> As far as I can tell, that document was never updated to describe
> spectre_v2=ibrs in the first place.  That would be a whole 'nother patch
> which I'm not volunteering for.  Nice try ;-)

What do you think the maintainers are for? To clean up after
people... ;-\

As a matter of fact, updating the documentation is already on my
ever-growing TODO list.

But for this patch I'd like to ask whoever of you does it, to add a one
or two sentences about why we need to allow STIBP selection with IBRS.

The rest of the documentation we'll flesh out in time.

Thx.
  

On Mon, Feb 20, 2023 at 11:16:45AM -0800, KP Singh wrote:
> > As far as I can tell, that document was never updated to describe
> > spectre_v2=ibrs in the first place.  That would be a whole 'nother patch
> > which I'm not volunteering for.  Nice try ;-)
> 
> This should at least be documented in the code.
> 
> Now it seems like it is not okay to work with people on the list and
> just send revisions bypassing them. This is not something we do in the
> kernel area I come from (an x86 favorite ;)).  Please feel free to go
> with Josh's version (or its future revisions). If you want me to
> re-spin with some comments, happy to. If not, please do at least give
> me Reported-by here.

It's a common practice even outside of x86.  I'd recommend not taking it
personally.  In the end it's all about what produces better code.

And please don't take this the wrong way, but sometimes it takes a bad
patch to inspire a better one.  I mean that with no disrespect, I myself
have sent many bad patches.

And if you don't like my patch, fine, send a v2...
  

On Mon, Feb 20, 2023 at 11:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Feb 20, 2023 at 11:16:45AM -0800, KP Singh wrote:
> > > As far as I can tell, that document was never updated to describe
> > > spectre_v2=ibrs in the first place.  That would be a whole 'nother patch
> > > which I'm not volunteering for.  Nice try ;-)
> >
> > This should at least be documented in the code.
> >
> > Now it seems like it is not okay to work with people on the list and
> > just send revisions bypassing them. This is not something we do in the
> > kernel area I come from (an x86 favorite ;)).  Please feel free to go
> > with Josh's version (or its future revisions). If you want me to
> > re-spin with some comments, happy to. If not, please do at least give
> > me Reported-by here.
>
> It's a common practice even outside of x86.  I'd recommend not taking it
> personally.  In the end it's all about what produces better code.
>
> And please don't take this the wrong way, but sometimes it takes a bad
> patch to inspire a better one.  I mean that with no disrespect, I myself
> have sent many bad patches.

Appreciate the clarification! Thank you so much.

>
> And if you don't like my patch, fine, send a v2...

I can also take a stab at the Documentation piece, this will make
Boris happy too. Will send a v2 later today.

>
> --
> Josh
  

On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote:
> On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote:
> > Drop stable@ again.
> > 
> > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote:
> > > IBRS is only enabled in kernel space.  Since it's not enabled in user
> > > space, user space isn't protected from indirect branch prediction
> > > attacks from a sibling CPU thread.
> > > 
> > > Allow STIBP to be enabled to protect against such attacks.
> > > 
> > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > 
> > Yah, look at that one:
> > 
> > commit 7c693f54c873691a4b7da05c7e0f74e67745d144
> > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Date:   Tue Jun 14 23:15:55 2022 +0200
> > 
> >     x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> > 
> >     Extend spectre_v2= boot option with Kernel IBRS.
> > 
> >     [jpoimboe: no STIBP with IBRS]
> >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > I'm assuming this was supposed to mean no STIBP in *kernel mode* when
> > IBRS is selected?
> 
> No it was supposed to be "no STIBP with *eIBRS*".

Maybe not, "no STIBP with eIBRS" was the state before the said patch.

In an offlist discussion during Retbleed embargo(copied below), it
appears to mean "no STIBP *in kernel* with IBRS". But anyways, we missed
to consider userspace.

(BTW replying late because yesterday was a holiday in my geo).

---
> > Subject: [PATCH v5 26/30] x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> > 
> > From: Peter Zijlstra <peterz@infradead.org>
> > 
> > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > 
> > The "spectre_v2=" boot option is extended to enable Kernel IBRS.
> > 
> > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  Documentation/admin-guide/kernel-parameters.txt |    1 
> >  arch/x86/include/asm/nospec-branch.h            |    1 
> >  arch/x86/kernel/cpu/bugs.c                      |   29 ++++++++++++++++++++++--
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > @@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit
> >  	case SPECTRE_V2_EIBRS:
> >  		break;
> >  
> > +	case SPECTRE_V2_IBRS:
> > +		setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
> > +		break;
> 
> Don't we also need to set SPEC_CTRL_IBRS in x86_spec_ctrl_base?

Also, STIBP isn't needed with IBRS.  Suggested changes:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 344ab7c9a4e2..498cb36587a3 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -897,11 +897,13 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
 	return SPECTRE_V2_USER_CMD_AUTO;
 }
 
-static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
+static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
 {
-	return (mode == SPECTRE_V2_EIBRS ||
-		mode == SPECTRE_V2_EIBRS_RETPOLINE ||
-		mode == SPECTRE_V2_EIBRS_LFENCE);
+
+	return spectre_v2_mode == SPECTRE_V2_IBRS
+	       spectre_v2_mode == SPECTRE_V2_EIBRS ||
+	       spectre_v2_mode == SPECTRE_V2_EIBRS_RETPOLINE ||
+	       spectre_v2_mode == SPECTRE_V2_EIBRS_LFENCE;
 }
 
 static void __init
@@ -966,12 +968,12 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
 	}
 
 	/*
-	 * If no STIBP, enhanced IBRS is enabled or SMT impossible, STIBP is not
-	 * required.
+	 * 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_eibrs_mode(spectre_v2_enabled))
+	    spectre_v2_in_ibrs_mode(spectre_v2_enabled))
 		return;
 
 	/*
@@ -1171,7 +1173,7 @@ static void __init spectre_v2_select_mitigation(void)
 	if (mode == SPECTRE_V2_EIBRS && unprivileged_ebpf_enabled())
 		pr_err(SPECTRE_V2_EIBRS_EBPF_MSG);
 
-	if (spectre_v2_in_eibrs_mode(mode)) {
+	if (spectre_v2_in_ibrs_mode(mode)) {
 		/* Force it so VMEXIT will restore correctly */
 		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
 		wr_spec_ctrl(x86_spec_ctrl_base, true);
@@ -1212,19 +1214,17 @@ static void __init spectre_v2_select_mitigation(void)
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
 
 	/*
-	 * Retpoline means the kernel is safe because it has no indirect
-	 * branches. Enhanced IBRS protects firmware too, so, enable restricted
-	 * speculation around firmware calls only when Enhanced IBRS isn't
-	 * supported or kernel IBRS isn't enabled.
+	 * Retpoline protects the kernel, but doesn't protect firmware.  IBRS
+	 * and Enhanced IBRS protect firmware too, so enable IBRS around
+	 * firmware calls only when IBRS / Enhanced IBRS aren't otherwise
+	 * enabled.
 	 *
 	 * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because
 	 * the user might select retpoline on the kernel command line and if
 	 * the CPU supports Enhanced IBRS, kernel might un-intentionally not
 	 * enable IBRS around firmware calls.
 	 */
-	if (boot_cpu_has(X86_FEATURE_IBRS) &&
-	    !boot_cpu_has(X86_FEATURE_KERNEL_IBRS) &&
-	    !spectre_v2_in_eibrs_mode(mode)) {
+	if (boot_cpu_has(X86_FEATURE_IBRS) && !spectre_v2_in_ibrs_mode(mode)) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
 		pr_info("Enabling Restricted Speculation for firmware calls\n");
 	}
@@ -1937,7 +1937,7 @@ static ssize_t tsx_async_abort_show_state(char *buf)
 
 static char *stibp_state(void)
 {
-	if (spectre_v2_in_eibrs_mode(spectre_v2_enabled))
+	if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
 		return "";
 
 	switch (spectre_v2_user_stibp) {
  

On Tue, Feb 21, 2023 at 5:20 PM Pawan Gupta
<pawan.kumar.gupta@linux.intel.com> wrote:
>
> On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote:
> > On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote:
> > > Drop stable@ again.
> > >
> > > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote:
> > > > IBRS is only enabled in kernel space.  Since it's not enabled in user
> > > > space, user space isn't protected from indirect branch prediction
> > > > attacks from a sibling CPU thread.
> > > >
> > > > Allow STIBP to be enabled to protect against such attacks.
> > > >
> > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
> > >
> > > Yah, look at that one:
> > >
> > > commit 7c693f54c873691a4b7da05c7e0f74e67745d144
> > > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > > Date:   Tue Jun 14 23:15:55 2022 +0200
> > >
> > >     x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> > >
> > >     Extend spectre_v2= boot option with Kernel IBRS.
> > >
> > >     [jpoimboe: no STIBP with IBRS]
> > >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > I'm assuming this was supposed to mean no STIBP in *kernel mode* when
> > > IBRS is selected?
> >
> > No it was supposed to be "no STIBP with *eIBRS*".
>
> Maybe not, "no STIBP with eIBRS" was the state before the said patch.
>
> In an offlist discussion during Retbleed embargo(copied below), it
> appears to mean "no STIBP *in kernel* with IBRS". But anyways, we missed
> to consider userspace.
>
> (BTW replying late because yesterday was a holiday in my geo).
>
> ---
> > > Subject: [PATCH v5 26/30] x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
> > >
> > > From: Peter Zijlstra <peterz@infradead.org>
> > >
> > > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > >
> > > The "spectre_v2=" boot option is extended to enable Kernel IBRS.
> > >
> > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  Documentation/admin-guide/kernel-parameters.txt |    1
> > >  arch/x86/include/asm/nospec-branch.h            |    1
> > >  arch/x86/kernel/cpu/bugs.c                      |   29 ++++++++++++++++++++++--
> > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > >
> > > @@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit
> > >     case SPECTRE_V2_EIBRS:
> > >             break;
> > >
> > > +   case SPECTRE_V2_IBRS:
> > > +           setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
> > > +           break;
> >
> > Don't we also need to set SPEC_CTRL_IBRS in x86_spec_ctrl_base?
>
> Also, STIBP isn't needed with IBRS.  Suggested changes:
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 344ab7c9a4e2..498cb36587a3 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -897,11 +897,13 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd)
>         return SPECTRE_V2_USER_CMD_AUTO;
>  }
>
> -static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode)
> +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
>  {
> -       return (mode == SPECTRE_V2_EIBRS ||
> -               mode == SPECTRE_V2_EIBRS_RETPOLINE ||
> -               mode == SPECTRE_V2_EIBRS_LFENCE);
> +
> +       return spectre_v2_mode == SPECTRE_V2_IBRS
> +              spectre_v2_mode == SPECTRE_V2_EIBRS ||
> +              spectre_v2_mode == SPECTRE_V2_EIBRS_RETPOLINE ||
> +              spectre_v2_mode == SPECTRE_V2_EIBRS_LFENCE;
>  }
>
>  static void __init
> @@ -966,12 +968,12 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd)
>         }
>
>         /*
> -        * If no STIBP, enhanced IBRS is enabled or SMT impossible, STIBP is not
> -        * required.
> +        * 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_eibrs_mode(spectre_v2_enabled))
> +           spectre_v2_in_ibrs_mode(spectre_v2_enabled))
>                 return;
>
>         /*
> @@ -1171,7 +1173,7 @@ static void __init spectre_v2_select_mitigation(void)
>         if (mode == SPECTRE_V2_EIBRS && unprivileged_ebpf_enabled())
>                 pr_err(SPECTRE_V2_EIBRS_EBPF_MSG);
>
> -       if (spectre_v2_in_eibrs_mode(mode)) {
> +       if (spectre_v2_in_ibrs_mode(mode)) {

Pawan can you review the v2 that I sent out here:

https://lore.kernel.org/lkml/20230221184908.2349578-1-kpsingh@kernel.org/T/#t

>                 /* Force it so VMEXIT will restore correctly */
>                 x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
>                 wr_spec_ctrl(x86_spec_ctrl_base, true);
> @@ -1212,19 +1214,17 @@ static void __init spectre_v2_select_mitigation(void)
>         pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
>
>         /*
> -        * Retpoline means the kernel is safe because it has no indirect
> -        * branches. Enhanced IBRS protects firmware too, so, enable restricted
> -        * speculation around firmware calls only when Enhanced IBRS isn't
> -        * supported or kernel IBRS isn't enabled.
> +        * Retpoline protects the kernel, but doesn't protect firmware.  IBRS
> +        * and Enhanced IBRS protect firmware too, so enable IBRS around
> +        * firmware calls only when IBRS / Enhanced IBRS aren't otherwise
> +        * enabled.
>          *
>          * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because
>          * the user might select retpoline on the kernel command line and if
>          * the CPU supports Enhanced IBRS, kernel might un-intentionally not
>          * enable IBRS around firmware calls.
>          */
> -       if (boot_cpu_has(X86_FEATURE_IBRS) &&
> -           !boot_cpu_has(X86_FEATURE_KERNEL_IBRS) &&
> -           !spectre_v2_in_eibrs_mode(mode)) {
> +       if (boot_cpu_has(X86_FEATURE_IBRS) && !spectre_v2_in_ibrs_mode(mode)) {
>                 setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
>                 pr_info("Enabling Restricted Speculation for firmware calls\n");
>         }
> @@ -1937,7 +1937,7 @@ static ssize_t tsx_async_abort_show_state(char *buf)
>
>  static char *stibp_state(void)
>  {
> -       if (spectre_v2_in_eibrs_mode(spectre_v2_enabled))
> +       if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
>                 return "";
>
>         switch (spectre_v2_user_stibp) {
  

On Tue, Feb 21, 2023 at 05:26:31PM -0800, KP Singh wrote:
> Pawan can you review the v2 that I sent out here:
> 
> https://lore.kernel.org/lkml/20230221184908.2349578-1-kpsingh@kernel.org/T/#t

Sure, looking at it.