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 <[email protected]>
Reported-by: Rodrigo Branco <[email protected]>
Reviewed-by: Alexandra Sandulescu <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
Cc: [email protected]
Signed-off-by: KP Singh <[email protected]>
---
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
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
On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <[email protected]> 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 <[email protected]> wrote:
>
> On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <[email protected]> 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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 <[email protected]> wrote:
> >
> > On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf <[email protected]> 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
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
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
On Mon, Feb 20, 2023 at 9:59 AM Josh Poimboeuf <[email protected]> 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...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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 <[email protected]>
Reported-by: Rodrigo Branco <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
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
On Mon, Feb 20, 2023 at 10:27 AM Josh Poimboeuf <[email protected]> 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 <[email protected]>
> Reported-by: Rodrigo Branco <[email protected]>
> Signed-off-by: Josh Poimboeuf <[email protected]>
> ---
> 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 <[email protected]>
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 20, 2023 at 10:22 AM Borislav Petkov <[email protected]> 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 20, 2023 at 10:52 AM Borislav Petkov <[email protected]> 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: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
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
On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf <[email protected]> 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 11:02 AM Borislav Petkov <[email protected]> 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: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 <[email protected]>
> 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 ;-)
--
Josh
On Mon, Feb 20, 2023 at 11:09 AM Josh Poimboeuf <[email protected]> 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 <[email protected]>
> > 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 <[email protected]> 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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...
--
Josh
On Mon, Feb 20, 2023 at 11:35 AM Josh Poimboeuf <[email protected]> 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 20/02/2023 2:31 pm, Borislav Petkov wrote:
> 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.
>
"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.
I think we're discussing the legacy IBRS case here. i.e. what was
retrofitted in microcode for existing parts?
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.
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.
~Andrew
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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 20, 2023 at 1:10 PM Borislav Petkov <[email protected]> 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 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.
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 3:31 PM Andrew Cooper <[email protected]> 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.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Mon, Feb 20, 2023 at 3:45 PM KP Singh <[email protected]> wrote:
>
> On Mon, Feb 20, 2023 at 3:31 PM Andrew Cooper <[email protected]> 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/[email protected]/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)
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 <[email protected]>
> > 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 <[email protected]>
> >
> > From: Pawan Gupta <[email protected]>
> >
> > The "spectre_v2=" boot option is extended to enable Kernel IBRS.
> >
> > Signed-off-by: Pawan Gupta <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > 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
<[email protected]> 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 <[email protected]>
> > > 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 <[email protected]>
> > >
> > > From: Pawan Gupta <[email protected]>
> > >
> > > The "spectre_v2=" boot option is extended to enable Kernel IBRS.
> > >
> > > Signed-off-by: Pawan Gupta <[email protected]>
> > > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > > ---
> > > 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/[email protected]/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/[email protected]/T/#t
Sure, looking at it.
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 <[email protected]>
AuthorDate: Mon, 27 Feb 2023 07:05:40 +01:00
Committer: Borislav Petkov (AMD) <[email protected]>
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 <[email protected]>
Reported-by: Rodrigo Branco <[email protected]>
Signed-off-by: KP Singh <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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) {