x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS
Subsystems (4)
PATCH RESEND 1/1 x86/speculation: Fix user-mode spectre-v2 protection with KERNEL_IBRS
With the introduction of KERNEL_IBRS, STIBP is no longer needed to prevent cross thread training in the kernel space. When KERNEL_IBRS was added, it also disabled the user-mode protections for spectre_v2. KERNEL_IBRS does not mitigate cross thread training in the userspace.
In order to demonstrate the issue, one needs to avoid syscalls in the victim as syscalls can shorten the window size due to
a user -> kernel -> user transition which sets the
IBRS bit when entering kernel space and clearing any training the attacker may have done.
Allow users to select a spectre_v2_user mitigation (STIBP always on, opt-in via prctl) when KERNEL_IBRS is enabled.
--- arch/x86/kernel/cpu/bugs.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)
arch/x86/kernel/cpu/bugs.c +17 -8
@@ -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) {
Signature · 1 line
2.39.2.637.g21b0678d19-goog
Josh Poimboeuf
maintainerarch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines · KP Singh
arch/x86/kernel/cpu/bugs.c +12
-0+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;
}arch/x86/kernel/cpu/bugs.c static void __init spectre_v2_select_mitigation(void) · line 1507
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +1
-0@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)break ;case SPECTRE_V2_IBRS : +pr_err ("enabling KERNEL_IBRS" );
Why?
arch/x86/kernel/cpu/bugs.c static ssize_t mmio_stale_data_show_state(char *buf) · line 2340
Quoted 7 lines
arch/x86/kernel/cpu/bugs.c +1
-1@@ -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.
Signature · 1 line
Josh
Quoted 20 lines · Josh Poimboeuf
arch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines · KP Singh
arch/x86/kernel/cpu/bugs.c +12
-0+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.
Quoted 4 lines
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?
Quoted 15 lines
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; }
Quoted 7 lines
arch/x86/kernel/cpu/bugs.c static void __init spectre_v2_select_mitigation(void) · line 1507
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +1
-0@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)break ;case SPECTRE_V2_IBRS : +pr_err ("enabling KERNEL_IBRS" );Why?
Removed.
Quoted 15 lines
arch/x86/kernel/cpu/bugs.c static ssize_t mmio_stale_data_show_state(char *buf) · line 2340
Quoted 8 lines
arch/x86/kernel/cpu/bugs.c +1
-1@@ -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_stibpwill already have its default ofSPECTRE_V2_USER_NONE.--
Josh
Quoted 22 lines · KP Singh
Quoted 20 lines · Josh Poimboeuf
arch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines · KP Singh
arch/x86/kernel/cpu/bugs.c +12
-0+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.
Quoted 2 lines
Quoted 2 lines
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"
Quoted 48 lines
Quoted 2 lines
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_IBRSgets enabled withSPECTRE_V2_EIBRS, but this does not seem to be the case currently. Maybe we should also add aBUG_ONto ensure thatKERNEL_IBRSdoes not get enabled in EIBRS mode?Quoted 15 lines
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; }Quoted 7 lines
arch/x86/kernel/cpu/bugs.c static void __init spectre_v2_select_mitigation(void) · line 1507
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +1
-0@@ -1496,6 +1504,7 @@ static void __init spectre_v2_select_mitigation(void)break ;case SPECTRE_V2_IBRS : +pr_err ("enabling KERNEL_IBRS" );Why?
Removed.
Quoted 15 lines
arch/x86/kernel/cpu/bugs.c static ssize_t mmio_stale_data_show_state(char *buf) · line 2340
Quoted 8 lines
arch/x86/kernel/cpu/bugs.c +1
-1@@ -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_stibpwill already have its default ofSPECTRE_V2_USER_NONE.--
Josh
Borislav Petkov
maintainerDrop stable@.
Quoted 31 lines · KP Singh
Quoted 19 lines
Quoted 19 lines · KP Singh
arch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines
arch/x86/kernel/cpu/bugs.c +12
-0+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.Quoted 2 lines
Quoted 2 lines
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_IBRSwe only enable IBRS (by setting and unsetting the IBRS bit ofSPEC_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, withspectre_v2_in_ibrs_mode, as per the current implementation implies thatKERNEL_IBRSis chosen, but this may change. Also, I would also prefer to have it more readable in the sense that:
Quoted 2 lines
"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:
"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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 2 lines · Borislav Petkov
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.
Quoted 48 lines · Borislav Petkov
Drop stable@.
Quoted 29 lines · KP Singh
Quoted 18 lines
Quoted 18 lines · KP Singh
arch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines
arch/x86/kernel/cpu/bugs.c +12
-0+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.Quoted 2 lines
Quoted 2 lines
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_IBRSwe only enable IBRS (by setting and unsetting the IBRS bit ofSPEC_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, withspectre_v2_in_ibrs_mode, as per the current implementation implies thatKERNEL_IBRSis chosen, but this may change. Also, I would also prefer to have it more readable in the sense that:Quoted 2 lines
"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:
"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_CTRLMSR 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."
Quoted 8 lines
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
Borislav Petkov
maintainerQuoted 8 lines · Andrew Cooper
"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.
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 3 lines
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 46 lines · Borislav Petkov
Quoted 8 lines · Andrew Cooper
"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.
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 3 lines
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;
Quoted 4 lines
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.
Quoted 7 lines
Thx.
--
Regards/Gruss,
Boris.
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Andrew Cooper
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
Quoted 22 lines · Andrew Cooper
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Andrew Cooper
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 AutoIBRSwhich 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.
Quoted 4 lines
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?
(quoted diff) +0 -0
b/Documentation/admin-guide/hw-vuln/spectre.rst
index c4dcdb3d0d45..d7003bbc82f6 100644
Documentation/admin-guide/hw-vuln/spectre.rst +5 -2
@@ -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.
Quoted 9 lines
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
Quoted 70 lines · KP Singh
Quoted 22 lines · Andrew Cooper
Quoted 5 lines · Borislav Petkov
Quoted 2 lines · Andrew Cooper
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 AutoIBRSwhich 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.
Quoted 4 lines
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?
Documentation/admin-guide/hw-vuln/spectre.rst +5
-2@@ -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 systemswith enhanced
Documentation/admin-guide/hw-vuln/spectre.rst +2
-0+ IBRS, the kernel sets the bit once, which keeps cross-thread protections + always enabled, obviating the need for an explicit STIBP. On CPUswith legacy
Documentation/admin-guide/hw-vuln/spectre.rst +4
-2+ 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()
Documentation/admin-guide/hw-vuln/spectre.rst Spectre variant 2 · line 524
Quoted 10 lines
Documentation/admin-guide/hw-vuln/spectre.rst +6
-1- (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)
Borislav Petkov
maintainerQuoted 2 lines · Andrew Cooper
1) Intel's legacy IBRS
2) AMD's regular IBRS
Yeah, we don't do that in the kernel.
Quoted 9 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteJosh Poimboeuf
maintainerQuoted 35 lines · KP Singh
Quoted 22 lines · KP Singh
Quoted 20 lines · Josh Poimboeuf
arch/x86/kernel/cpu/bugs.c static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) · line 1146
Quoted 12 lines · KP Singh
arch/x86/kernel/cpu/bugs.c +12
-0+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.Quoted 2 lines
Quoted 2 lines
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_IBRSwe only enable IBRS (by setting and unsetting the IBRS bit ofSPEC_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, withspectre_v2_in_ibrs_mode, as per the current implementation implies thatKERNEL_IBRSis chosen, but this may change. Also, I would also prefer to have it more readable in the sense that:
Quoted 2 lines
"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.
Signature · 1 line
Josh
Borislav Petkov
maintainerQuoted 2 lines · Josh Poimboeuf
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteJosh Poimboeuf
maintainerQuoted 10 lines · Borislav Petkov
Quoted 2 lines · Josh Poimboeuf
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.
Signature · 1 line
Josh
Quoted 8 lines · Josh Poimboeuf
Quoted 6 lines · Borislav Petkov
Quoted 2 lines · Josh Poimboeuf
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 3 lines
--
Josh
Borislav Petkov
maintainerQuoted 5 lines · KP Singh
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...
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 8 lines · Borislav Petkov
Quoted 5 lines · KP Singh
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.
Quoted 6 lines
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:
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.
Quoted 9 lines
So, maybe you should explain what you're trying to accomplish in detail and where it fails...
--
Regards/Gruss,
Boris.
Borislav Petkov
maintainerQuoted 1 line · KP Singh
No it cannot with IBRS which is really just
KERNEL_IBRSenabled, 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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 8 lines · Borislav Petkov
Quoted 1 line · KP Singh
No it cannot with IBRS which is really just
KERNEL_IBRSenabled, weSee 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.
Quoted 5 lines
--
Regards/Gruss,
Boris.
Borislav Petkov
maintainerQuoted 3 lines · KP Singh
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.
Quoted 2 lines
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.
Quoted 2 lines
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteQuoted 6 lines · Borislav Petkov
Quoted 3 lines · KP Singh
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.
Quoted 7 lines
Right.
Quoted 2 lines
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.
Quoted 14 lines
Quoted 3 lines
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.
Josh Poimboeuf
maintainerPATCH 1/1 x86/bugs: Allow STIBP with IBRS
In reply to KP SinghIBRS 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.
--- arch/x86/kernel/cpu/bugs.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
arch/x86/kernel/cpu/bugs.c +10 -8
@@ -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" ;
Signature · 1 line
2.39.1
kernel test robot
build botRe: [PATCH] x86/bugs: Allow STIBP with IBRS
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Rule: 'Cc: [email protected]' or 'commit <sha1> upstream.'
Subject: [PATCH] x86/bugs: Allow STIBP with IBRS
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [PATCH] x86/bugs: Allow STIBP with IBRS
In reply to Josh PoimboeufQuoted 22 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")Reported-by José Oliveira <[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(-)arch/x86/kernel/cpu/bugs.c +0
-0@@ -1124,14 +1124,19 @@ spectre_v2_parse_user_cmdline(void)return SPECTRE_V2_USER_CMD_AUTO ; }
arch/x86/kernel/cpu/bugs.c spectre_v2_parse_user_cmdline(void) · line 1132
Quoted 10 lines
arch/x86/kernel/cpu/bugs.c +2
-3-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:
arch/x86/kernel/cpu/bugs.c +23 -11
@@ -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.
Quoted 38 lines
arch/x86/kernel/cpu/bugs.c +8
-6+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
Josh Poimboeuf
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 8 lines · KP Singh
static char
*stibp_state(void)
{arch/x86/kernel/cpu/bugs.c +1
-1-if (spectre_v2_in_ibrs_mode (spectre_v2_enabled)) +if (!spectre_v2_user_needs_stibp (spectre_v2_enabled))return "" ;switch (spectre_v2_user_stibp) {
Quoted 4 lines
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?
Signature · 1 line
Josh
Re: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 10 lines · Josh Poimboeuf
Quoted 8 lines · KP Singh
static char
*stibp_state(void)
{arch/x86/kernel/cpu/bugs.c +1
-1-if (spectre_v2_in_ibrs_mode (spectre_v2_enabled)) +if (!spectre_v2_user_needs_stibp (spectre_v2_enabled))return "" ;switch (spectre_v2_user_stibp) {
Quoted 7 lines
Quoted 4 lines
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!
Quoted 2 lines
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.
Quoted 5 lines
I can give you Reported-by, or did you have some other tag in mind?
--
Josh
Josh Poimboeuf
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 23 lines · KP Singh
Quoted 10 lines · Josh Poimboeuf
Quoted 8 lines · KP Singh
static char
*stibp_state(void)
{(quoted diff) +1
-1- if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) + if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) return ""; switch (spectre_v2_user_stibp) {Quoted 7 lines
Quoted 4 lines
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!
Quoted 6 lines
Quoted 2 lines
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?
arch/x86/kernel/cpu/bugs.c +4 -0
@@ -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 ||
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
In reply to Josh PoimboeufDrop stable@ again.
Quoted 7 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquetteJosh Poimboeuf
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 19 lines · Borislav Petkov
Drop stable@ again.
Quoted 7 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")Yah, look at that one:
commit
7c693f54c873691a4b7da05c7e0f74e67745d144Author: Pawan Gupta <[email protected]> Date: Tue Jun 14 23:15:55 2022 +0200x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
Quoted 7 lines
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*".
Quoted 11 lines
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.rstbecause 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 ;-)
Signature · 1 line
Josh
Re: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 39 lines · Josh Poimboeuf
Quoted 19 lines · Borislav Petkov
Drop stable@ again.
Quoted 7 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")Yah, look at that one:
commit
7c693f54c873691a4b7da05c7e0f74e67745d144Author: Pawan Gupta <[email protected]> Date: Tue Jun 14 23:15:55 2022 +0200x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
Quoted 7 lines
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*".
Quoted 8 lines
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
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
--
Josh
Josh Poimboeuf
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 4 lines · KP Singh
Quoted 3 lines
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 ;-)
Quoted 8 lines
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...
Signature · 1 line
Josh
Re: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 15 lines · Josh Poimboeuf
Quoted 4 lines · KP Singh
Quoted 3 lines
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 ;-)
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
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.
Quoted 3 lines
--
Josh
Borislav Petkov
maintainerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
In reply to Josh PoimboeufQuoted 1 line · Josh Poimboeuf
No it was supposed to be "no STIBP with *eIBRS*".
Ok, good.
Quoted 3 lines
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.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePawan Gupta
reviewerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
In reply to Josh PoimboeufQuoted 22 lines · Josh Poimboeuf
Quoted 21 lines · Borislav Petkov
Drop stable@ again.
Quoted 7 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")Yah, look at that one:
commit
7c693f54c873691a4b7da05c7e0f74e67745d144Author: Pawan Gupta <[email protected]> Date: Tue Jun 14 23:15:55 2022 +0200x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
Extend spectre_v2= boot option with Kernel IBRS.
Quoted 7 lines
Quoted 5 lines
[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).
---
Quoted 20 lines
Quoted 20 lines
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(-)(quoted diff) +0
-0@@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit case SPECTRE_V2_EIBRS: break;
Quoted 5 lines
Review static void __init spectre_v2_select_mit · line 1187
Quoted 3 lines
(quoted diff) +3
-0+ case SPECTRE_V2_IBRS: + setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS); + break;Don't we also need to set
SPEC_CTRL_IBRSinx86_spec_ctrl_base?
Also, STIBP isn't needed with IBRS. Suggested changes:
arch/x86/kernel/cpu/bugs.c +16 -16
@@ -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) {
Re: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 108 lines · Pawan Gupta
Quoted 22 lines · Josh Poimboeuf
Quoted 21 lines · Borislav Petkov
Drop stable@ again.
Quoted 7 lines · Josh Poimboeuf
IBRS is only enabled in kernel space. Since it's not enabled in user space, user space isn't protected from indirect branch prediction attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")Yah, look at that one:
commit
7c693f54c873691a4b7da05c7e0f74e67745d144Author: Pawan Gupta <[email protected]> Date: Tue Jun 14 23:15:55 2022 +0200x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS
Extend spectre_v2= boot option with Kernel IBRS.
Quoted 7 lines
Quoted 5 lines
[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).
---
Quoted 20 lines
Quoted 20 lines
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(-)(quoted diff) +0
-0@@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit case SPECTRE_V2_EIBRS: break;Quoted 5 lines
Review static void __init spectre_v2_select_mit · line 1187
Quoted 3 lines
(quoted diff) +3
-0+ case SPECTRE_V2_IBRS: + setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS); + break;Don't we also need to set
SPEC_CTRL_IBRSinx86_spec_ctrl_base?Also, STIBP isn't needed with IBRS. Suggested changes:
arch/x86/kernel/cpu/bugs.c +9
-7@@ -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 ;/*
arch/x86/kernel/cpu/bugs.c static void __init spectre_v2_select_mitigation(void) · line 1176
Quoted 5 lines
arch/x86/kernel/cpu/bugs.c +1
-1@@ -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:
Quoted 37 lines
arch/x86/kernel/cpu/bugs.c +6
-8/* 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) {
Pawan Gupta
reviewerRe: [PATCH] x86/bugs: Allow STIBP with IBRS
Quoted 3 lines · KP Singh
Pawan can you review the v2 that I sent out here:
Sure, looking at it.
[tip: x86/urgent] x86/speculation: Allow enabling STIBP with legacy IBRS
In reply to KP SinghThe following commit has been merged into the x86/urgent branch of tip:
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. ]
--- arch/x86/kernel/cpu/bugs.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
arch/x86/kernel/cpu/bugs.c +18 -7
@@ -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) {