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

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

[KP Singh]

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(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index bca0bd8f4846..b05ca1575d81 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1132,6 +1132,19 @@ static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode)
 	       mode == SPECTRE_V2_EIBRS_LFENCE;
 }
 
+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));
+}
+
 static void __init
 spectre_v2_user_select_mitigation(void)
 {
@@ -1193,13 +1206,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_no_stibp(spectre_v2_enabled))
 		return;
 
 	/*
@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)
 		break;
 
 	case SPECTRE_V2_IBRS:
+		pr_err("enabling KERNEL_IBRS");
 		setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS);
 		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED))
 			pr_warn(SPECTRE_V2_IBRS_PERF_MSG);
@@ -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 "";
 
 	switch (spectre_v2_user_stibp) {
-- 
2.39.2.637.g21b0678d19-goog

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

[Josh Poimboeuf]

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.

-- 
Josh

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

[KP Singh]

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

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

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

[Josh Poimboeuf]

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.

-- 
Josh

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Josh Poimboeuf]

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.

-- 
Josh

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

[KP Singh]

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

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

[Borislav Petkov]

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

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH] x86/bugs: Allow STIBP with IBRS

[Josh Poimboeuf]

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;
 }
 
+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

Re: [PATCH] x86/bugs: Allow STIBP with IBRS

[kernel test robot]

Hi,

Thanks for your patch.

FYI: kernel test robot notices the stable kernel rule is not satisfied.

Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.'
Subject: [PATCH] x86/bugs: Allow STIBP with IBRS
Link: https://lore.kernel.org/stable/20230220182717.uzrym2gtavlbjbxo%40treble

The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH] x86/bugs: Allow STIBP with IBRS

[KP Singh]

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
>

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

[KP Singh]

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

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[KP Singh]

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

Re: [PATCH] x86/bugs: Allow STIBP with IBRS

[Josh Poimboeuf]

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?

-- 
Josh

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/bugs: Allow STIBP with IBRS

[KP Singh]

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

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

[KP Singh]

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

Re: [PATCH] x86/bugs: Allow STIBP with IBRS

[Josh Poimboeuf]

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

[tip: x86/urgent] x86/speculation: Allow enabling STIBP with legacy IBRS

[tip-bot2 for KP Singh]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     6921ed9049bc7457f66c1596c5b78aec0dae4a9d
Gitweb:        https://git.kernel.org/tip/6921ed9049bc7457f66c1596c5b78aec0dae4a9d
Author:        KP Singh <kpsingh@kernel.org>
AuthorDate:    Mon, 27 Feb 2023 07:05:40 +01:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 27 Feb 2023 18:57:09 +01:00

x86/speculation: Allow enabling STIBP with legacy IBRS

When plain IBRS is enabled (not enhanced IBRS), the logic in
spectre_v2_user_select_mitigation() determines that STIBP is not needed.

The IBRS bit implicitly protects against cross-thread branch target
injection. However, with legacy IBRS, the IBRS bit is cleared on
returning to userspace for performance reasons which leaves userspace
threads vulnerable to cross-thread branch target injection against which
STIBP protects.

Exclude IBRS from the spectre_v2_in_ibrs_mode() check to allow for
enabling STIBP (through seccomp/prctl() by default or always-on, if
selected by spectre_v2_user kernel cmdline parameter).

  [ bp: Massage. ]

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: KP Singh <kpsingh@kernel.org>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20230220120127.1975241-1-kpsingh@kernel.org
Link: https://lore.kernel.org/r/20230221184908.2349578-1-kpsingh@kernel.org
---
 arch/x86/kernel/cpu/bugs.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index cf81848..f9d060e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1133,14 +1133,18 @@ 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;
 }
 
+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)
 {
@@ -1203,12 +1207,19 @@ spectre_v2_user_select_mitigation(void)
 	}
 
 	/*
-	 * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible,
-	 * STIBP is not required.
+	 * If no STIBP, enhanced IBRS is enabled, or SMT impossible, STIBP
+	 * is not required.
+	 *
+	 * Enhanced IBRS also protects against cross-thread branch target
+	 * injection in user-mode as the IBRS bit remains always set which
+	 * implicitly enables cross-thread protections.  However, in legacy IBRS
+	 * mode, the IBRS bit is set only on kernel entry and cleared on return
+	 * to userspace. This disables the implicit cross-thread protection,
+	 * so allow for STIBP to be selected in that case.
 	 */
 	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;
 
 	/*
@@ -2340,7 +2351,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_in_eibrs_mode(spectre_v2_enabled))
 		return "";
 
 	switch (spectre_v2_user_stibp) {