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) {
On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
+static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode) +{
- /* When IBRS or enhanced IBRS is enabled, STIBP is not needed.
** However, With KERNEL_IBRS, the IBRS bit is cleared on return* to user and the user-mode code needs to be able to enable protection* from cross-thread training, either by always enabling STIBP or* by enabling it via prctl.*/- return (spectre_v2_in_ibrs_mode(mode) &&
!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));+}
The comments and code confused me, they both seem to imply some distinction between IBRS and KERNEL_IBRS, but in the kernel those are functionally the same thing. e.g., the kernel doesn't have a user IBRS mode.
And, unless I'm missing some subtlety here, it seems to be a convoluted way of saying that eIBRS doesn't need STIBP in user space.
It would be simpler to just call it spectre_v2_in_eibrs_mode().
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return mode == SPECTRE_V2_EIBRS || mode == SPECTRE_V2_EIBRS_RETPOLINE || mode == SPECTRE_V2_EIBRS_LFENCE; }
And then spectre_v2_in_ibrs_mode() could be changed to call that:
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS; }
@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void) break; case SPECTRE_V2_IBRS:
pr_err("enabling KERNEL_IBRS");
Why?
@@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf) static char *stibp_state(void) {
- if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
- if (spectre_v2_user_no_stibp(spectre_v2_enabled)) return "";
This seems like old cruft, can we just remove this check altogether? In the eIBRS case, spectre_v2_user_stibp will already have its default of SPECTRE_V2_USER_NONE.
On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf jpoimboe@kernel.org wrote:
On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
+static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode) +{
/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.** However, With KERNEL_IBRS, the IBRS bit is cleared on return* to user and the user-mode code needs to be able to enable protection* from cross-thread training, either by always enabling STIBP or* by enabling it via prctl.*/return (spectre_v2_in_ibrs_mode(mode) &&!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));+}
The comments and code confused me, they both seem to imply some distinction between IBRS and KERNEL_IBRS, but in the kernel those are functionally the same thing. e.g., the kernel doesn't have a user IBRS mode.
And, unless I'm missing some subtlety here, it seems to be a convoluted way of saying that eIBRS doesn't need STIBP in user space.
It would be simpler to just call it spectre_v2_in_eibrs_mode().
Thanks, yeah this would work too. I was just trying to ensure that, if somehow, KERNEL_IBRS gets enabled with SPECTRE_V2_EIBRS, but this does not seem to be the case currently. Maybe we should also add a BUG_ON to ensure that KERNEL_IBRS does not get enabled in EIBRS mode?
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return mode == SPECTRE_V2_EIBRS || mode == SPECTRE_V2_EIBRS_RETPOLINE || mode == SPECTRE_V2_EIBRS_LFENCE; }
And then spectre_v2_in_ibrs_mode() could be changed to call that:
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS; }
@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void) break;
case SPECTRE_V2_IBRS:
pr_err("enabling KERNEL_IBRS");Why?
Removed.
@@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
static char *stibp_state(void) {
if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
if (spectre_v2_user_no_stibp(spectre_v2_enabled)) return "";This seems like old cruft, can we just remove this check altogether? In the eIBRS case, spectre_v2_user_stibp will already have its default of SPECTRE_V2_USER_NONE.
-- Josh
On Mon, Feb 20, 2023 at 4:20 AM KP Singh kpsingh@kernel.org wrote:
On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf jpoimboe@kernel.org wrote:
On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
+static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode) +{
/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.** However, With KERNEL_IBRS, the IBRS bit is cleared on return* to user and the user-mode code needs to be able to enable protection* from cross-thread training, either by always enabling STIBP or* by enabling it via prctl.*/return (spectre_v2_in_ibrs_mode(mode) &&!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));+}
The comments and code confused me, they both seem to imply some distinction between IBRS and KERNEL_IBRS, but in the kernel those are functionally the same thing. e.g., the kernel doesn't have a user IBRS mode.
And, unless I'm missing some subtlety here, it seems to be a convoluted way of saying that eIBRS doesn't need STIBP in user space.
Actually, there is a subtlety, STIBP is not needed in IBRS and eIBRS however, with KERNEL_IBRS we only enable IBRS (by setting and unsetting the IBRS bit of SPEC_CTL) in the kernel context and this is why we need to allow STIBP in userspace. If it were for pure IBRS, we would not need it either (based on my reading). Now, with spectre_v2_in_ibrs_mode, as per the current implementation implies that KERNEL_IBRS is chosen, but this may change. Also, I would also prefer to have it more readable in the sense that:
"If the kernel decides to write 0 to the IBRS bit on returning to the user, STIBP needs to to be allowed in user space"
It would be simpler to just call it spectre_v2_in_eibrs_mode().
Thanks, yeah this would work too. I was just trying to ensure that, if somehow, KERNEL_IBRS gets enabled with SPECTRE_V2_EIBRS, but this does not seem to be the case currently. Maybe we should also add a BUG_ON to ensure that KERNEL_IBRS does not get enabled in EIBRS mode?
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return mode == SPECTRE_V2_EIBRS || mode == SPECTRE_V2_EIBRS_RETPOLINE || mode == SPECTRE_V2_EIBRS_LFENCE; }
And then spectre_v2_in_ibrs_mode() could be changed to call that:
static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS; }
@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void) break;
case SPECTRE_V2_IBRS:
pr_err("enabling KERNEL_IBRS");Why?
Removed.
@@ -2327,7 +2336,7 @@ static ssize_t mmio_stale_data_show_state(char *buf)
static char *stibp_state(void) {
if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
if (spectre_v2_user_no_stibp(spectre_v2_enabled)) return "";This seems like old cruft, can we just remove this check altogether? In the eIBRS case, spectre_v2_user_stibp will already have its default of SPECTRE_V2_USER_NONE.
-- Josh
On Mon, Feb 20, 2023 at 04:34:02AM -0800, KP Singh wrote:
On Mon, Feb 20, 2023 at 4:20 AM KP Singh kpsingh@kernel.org wrote:
On Mon, Feb 20, 2023 at 4:13 AM Josh Poimboeuf jpoimboe@kernel.org wrote:
On Mon, Feb 20, 2023 at 01:01:27PM +0100, KP Singh wrote:
+static inline bool spectre_v2_user_no_stibp(enum spectre_v2_mitigation mode) +{
/* When IBRS or enhanced IBRS is enabled, STIBP is not needed.** However, With KERNEL_IBRS, the IBRS bit is cleared on return* to user and the user-mode code needs to be able to enable protection* from cross-thread training, either by always enabling STIBP or* by enabling it via prctl.*/return (spectre_v2_in_ibrs_mode(mode) &&!cpu_feature_enabled(X86_FEATURE_KERNEL_IBRS));+}
The comments and code confused me, they both seem to imply some distinction between IBRS and KERNEL_IBRS, but in the kernel those are functionally the same thing. e.g., the kernel doesn't have a user IBRS mode.
And, unless I'm missing some subtlety here, it seems to be a convoluted way of saying that eIBRS doesn't need STIBP in user space.
Actually, there is a subtlety, STIBP is not needed in IBRS and eIBRS however, with KERNEL_IBRS we only enable IBRS (by setting and unsetting the IBRS bit of SPEC_CTL) in the kernel context and this is why we need to allow STIBP in userspace. If it were for pure IBRS, we would not need it either (based on my reading). Now, with spectre_v2_in_ibrs_mode, as per the current implementation implies that KERNEL_IBRS is chosen, but this may change. Also, I would also prefer to have it more readable in the sense that:
"If the kernel decides to write 0 to the IBRS bit on returning to the user, STIBP needs to to be allowed in user space"
We will never enable IBRS in user space. We tried that years ago and it was very slow.
On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
We will never enable IBRS in user space. We tried that years ago and it was very slow.
Then I don't know what this is trying to fix.
We'd need a proper bug statement in the commit message what the problem is. As folks have established, the hw vuln mess is a whole universe of crazy. So without proper documenting, this spaghetti madness will be completely unreadable.
Thx.
On Mon, Feb 20, 2023 at 06:46:04PM +0100, Borislav Petkov wrote:
On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
We will never enable IBRS in user space. We tried that years ago and it was very slow.
Then I don't know what this is trying to fix.
We'd need a proper bug statement in the commit message what the problem is. As folks have established, the hw vuln mess is a whole universe of crazy. So without proper documenting, this spaghetti madness will be completely unreadable.
Agreed, and there seems to be a lot of confusion around this patch. I think I understand the issue, let me write up a new patch which is hopefully clearer.
On Mon, Feb 20, 2023 at 9:59 AM Josh Poimboeuf jpoimboe@kernel.org wrote:
On Mon, Feb 20, 2023 at 06:46:04PM +0100, Borislav Petkov wrote:
On Mon, Feb 20, 2023 at 08:34:42AM -0800, Josh Poimboeuf wrote:
We will never enable IBRS in user space. We tried that years ago and it was very slow.
Then I don't know what this is trying to fix.
We'd need a proper bug statement in the commit message what the problem is. As folks have established, the hw vuln mess is a whole universe of crazy. So without proper documenting, this spaghetti madness will be completely unreadable.
Agreed, and there seems to be a lot of confusion around this patch. I think I understand the issue, let me write up a new patch which is hopefully clearer.
Feel free to write a patch, but I don't get the confusion.
Well, we disable IBRS userspace (this is KENREL_IBRS), because it is slow. Now if a user space process wants to protect itself from cross thread training, it should be able to do it, either by turning STIBP always on or using a prctl to enable. With the current logic, it's unable to do so. All of this is mentioned in the commit message.
-- Josh
On Mon, Feb 20, 2023 at 10:01:57AM -0800, KP Singh wrote:
Well, we disable IBRS userspace (this is KENREL_IBRS), because it is slow. Now if a user space process wants to protect itself from cross thread training, it should be able to do it, either by turning STIBP always on or using a prctl to enable. With the current logic, it's unable to do so.
Ofcourse it can:
[SPECTRE_V2_USER_PRCTL] = "User space: Mitigation: STIBP via prctl",
we did this at the time so that a userspace process can control it via prctl().
So, maybe you should explain what you're trying to accomplish in detail and where it fails...
On Mon, Feb 20, 2023 at 10:22 AM Borislav Petkov bp@alien8.de wrote:
On Mon, Feb 20, 2023 at 10:01:57AM -0800, KP Singh wrote:
Well, we disable IBRS userspace (this is KENREL_IBRS), because it is slow. Now if a user space process wants to protect itself from cross thread training, it should be able to do it, either by turning STIBP always on or using a prctl to enable. With the current logic, it's unable to do so.
Ofcourse it can:
[SPECTRE_V2_USER_PRCTL] = "User space: Mitigation: STIBP via prctl",we did this at the time so that a userspace process can control it via prctl().
No it cannot with IBRS which is really just KERNEL_IBRS enabled, we bail out if spectre_v2_in_inbrs_mode and ignore any selections:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch...
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.
On Mon, Feb 20, 2023 at 10:44:21AM -0800, KP Singh wrote:
No it cannot with IBRS which is really just KERNEL_IBRS enabled, we
See my other reply. The intent is there to be able to do it. What needs to be figured out now is *why* we said no STIBP with IBRS? Was it an omission or was there some intent behind it.
On Mon, Feb 20, 2023 at 10:52 AM Borislav Petkov bp@alien8.de wrote:
On Mon, Feb 20, 2023 at 10:44:21AM -0800, KP Singh wrote:
No it cannot with IBRS which is really just KERNEL_IBRS enabled, we
See my other reply. The intent is there to be able to do it. What needs to be figured out now is *why* we said no STIBP with IBRS? Was it an omission or was there some intent behind it.
Sure, it looks like an omission to me, we wrote a POC on Skylake that was able to do cross-thread training with the current set of mitigations.
STIBP with IBRS is still correct if spectre_v2=ibrs had really meant IBRS everywhere, but just means KERNEL_IBRS, which means only kernel is protected, userspace is still unprotected.
-- Regards/Gruss, Boris.
On Mon, Feb 20, 2023 at 10:56:38AM -0800, KP Singh wrote:
Sure, it looks like an omission to me, we wrote a POC on Skylake that was able to do cross-thread training with the current set of mitigations.
Right.
STIBP with IBRS is still correct if spectre_v2=ibrs had really meant IBRS everywhere,
Yeah, IBRS everywhere got shot down as a no-no very early in the game, for apparent reasons.
but just means KERNEL_IBRS, which means only kernel is protected, userspace is still unprotected.
Yes, that was always the intent with IBRS: enable on kernel entry and disable on exit.
Thx.
On Mon, Feb 20, 2023 at 11:02 AM Borislav Petkov bp@alien8.de wrote:
On Mon, Feb 20, 2023 at 10:56:38AM -0800, KP Singh wrote:
Sure, it looks like an omission to me, we wrote a POC on Skylake that was able to do cross-thread training with the current set of mitigations.
Right.
STIBP with IBRS is still correct if spectre_v2=ibrs had really meant IBRS everywhere,
Yeah, IBRS everywhere got shot down as a no-no very early in the game, for apparent reasons.
As you said in the other thread, this needs to be documented both in the code and the kernel documentation.
but just means KERNEL_IBRS, which means only kernel is protected, userspace is still unprotected.
Yes, that was always the intent with IBRS: enable on kernel entry and disable on exit.
Thx.
-- Regards/Gruss, Boris.
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";
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
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
On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote:
static char *stibp_state(void) {
if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) return ""; switch (spectre_v2_user_stibp) {Also Josh, is it okay for us to have a discussion and have me write the patch as a v2? Your current patch does not even credit me at all. Seems a bit unfair, but I don't really care. I was going to rev up the patch with your suggestions.
Well, frankly the patch needed a complete rewrite. The patch description was unclear about what the problem is and what's being fixed. The code was obtuse and the comments didn't help. I could tell by the other replies that I wasn't the only one confused.
I can give you Reported-by, or did you have some other tag in mind?
On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf jpoimboe@kernel.org wrote:
On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote:
static char *stibp_state(void) {
if (spectre_v2_in_ibrs_mode(spectre_v2_enabled))
if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) return ""; switch (spectre_v2_user_stibp) {Also Josh, is it okay for us to have a discussion and have me write the patch as a v2? Your current patch does not even credit me at all. Seems a bit unfair, but I don't really care. I was going to rev up the patch with your suggestions.
Well, frankly the patch needed a complete rewrite. The patch description was unclear about what the problem is and what's being
Josh, this is a complex issue, we are figuring it out together on the list. It's complex, that's why folks got it wrong in the first place. Calling the patch obtuse and unclear is unfair!
fixed. The code was obtuse and the comments didn't help. I could tell by the other replies that I wasn't the only one confused.
The patch you sent is not clear either, it implicitly ties in STIBP with eIBRS. There is no explanation anywhere that IBRS just means KERNEL_IBRS.
I can give you Reported-by, or did you have some other tag in mind?
-- Josh
On Mon, Feb 20, 2023 at 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 ||
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) {
linux-stable-mirror@lists.linaro.org
-
Borislav Petkov -
Josh Poimboeuf -
kernel test robot -
KP Singh -
tip-bot2 for KP Singh