x86/bugs: Default retbleed to =stuff when retpoline is auto enabled
Commit Message
On Intel systems when retpoline mitigation is enabled for spectre-v2,
retbleed=auto does not enable RSB stuffing. This may make the system
vulnerable to retbleed. Retpoline is not the default mitigation when
IBRS is present, but in virtualized cases a VMM can hide IBRS from
guests, resulting in guest deploying retpoline by default.
On Intel systems, when spectre_v2 and retbleed mitigations are in auto
mode, and retpoline is enabled, deploy Call Depth Tracking and RSB
stuffing i.e. retbleed=stuff mitigation. For AMD/Hygon auto mode already
selects the appropriate mitigation.
Reported-by: Alyssa Milburn <alyssa.milburn@intel.com>
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
arch/x86/kernel/cpu/bugs.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
---
base-commit: 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478
change-id: 20240208-retbleed-auto-stuff-53e0fa91305e
Best regards,
Comments
On Thu, Feb 08, 2024 at 05:12:15PM -0800, Pawan Gupta wrote:
> @@ -1025,10 +1041,17 @@ static void __init retbleed_select_mitigation(void)
> retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> else if (IS_ENABLED(CONFIG_CPU_IBPB_ENTRY) && boot_cpu_has(X86_FEATURE_IBPB))
> retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> + } else if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> + spectre_v2_parse_cmdline() == SPECTRE_V2_CMD_AUTO &&
spectre_v2_parse_cmdline() has side effects (printks) and shouldn't be
called twice.
And what's the point of checking spectre_v2= anyway? Shouldn't retbleed
be mitigated by default, independently of whatever the user may have
specified for Spectre v2?
> + spectre_v2_enabled == SPECTRE_V2_RETPOLINE) {
> + if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING))
> + retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
> + else
> + pr_err("WARNING: Retpoline enabled, but kernel not compiled with CALL_DEPTH_TRACKING.\n");
If retbleed is vulnerable then the sysfs file should show that.
Also, I think this pr_err() is redundant with RETBLEED_INTEL_MSG and can
be removed.
Hi Josh / Borislav,
On Fri, Feb 09, 2024 at 10:56:25AM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 08, 2024 at 05:12:15PM -0800, Pawan Gupta wrote:
> > @@ -1025,10 +1041,17 @@ static void __init retbleed_select_mitigation(void)
> > retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> > else if (IS_ENABLED(CONFIG_CPU_IBPB_ENTRY) && boot_cpu_has(X86_FEATURE_IBPB))
> > retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> > + } else if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> > + spectre_v2_parse_cmdline() == SPECTRE_V2_CMD_AUTO &&
>
> spectre_v2_parse_cmdline() has side effects (printks) and shouldn't be
> called twice.
>
> And what's the point of checking spectre_v2= anyway? Shouldn't retbleed
> be mitigated by default, independently of whatever the user may have
> specified for Spectre v2?
@Borislav, what do you think about this approach?
> > + spectre_v2_enabled == SPECTRE_V2_RETPOLINE) {
> > + if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING))
> > + retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
> > + else
> > + pr_err("WARNING: Retpoline enabled, but kernel not compiled with CALL_DEPTH_TRACKING.\n");
>
> If retbleed is vulnerable then the sysfs file should show that.
It does shows vulnerable, retbleed_mitigation is not set to
RETBLEED_MITIGATION_STUFF for CONFIG_CALL_DEPTH_TRACKING=n.
> Also, I think this pr_err() is redundant with RETBLEED_INTEL_MSG and can
> be removed.
IMO, they both serve a different purpose, RETBLEED_INTEL_MSG is a
generic warning, and the message with !CONFIG_CALL_DEPTH_TRACKING hints
a corrective action.
If you look at "case RETBLEED_CMD_STUFF" it also prints a separate warning for
!CONFIG_CALL_DEPTH_TRACKING:
case RETBLEED_CMD_STUFF:
if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING) &&
spectre_v2_enabled == SPECTRE_V2_RETPOLINE) {
retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
} else {
if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING))
pr_err("WARNING: retbleed=stuff depends on spectre_v2=retpoline\n");
else
pr_err("WARNING: kernel not compiled with CALL_DEPTH_TRACKING.\n");
On Fri, Feb 09, 2024 at 12:33:46PM -0800, Pawan Gupta wrote:
> On Fri, Feb 09, 2024 at 10:56:25AM -0800, Josh Poimboeuf wrote:
> > On Thu, Feb 08, 2024 at 05:12:15PM -0800, Pawan Gupta wrote:
> > > + spectre_v2_enabled == SPECTRE_V2_RETPOLINE) {
> > > + if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING))
> > > + retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
> > > + else
> > > + pr_err("WARNING: Retpoline enabled, but kernel not compiled with CALL_DEPTH_TRACKING.\n");
> >
> > If retbleed is vulnerable then the sysfs file should show that.
>
> It does shows vulnerable, retbleed_mitigation is not set to
> RETBLEED_MITIGATION_STUFF for CONFIG_CALL_DEPTH_TRACKING=n.
Ah right, in that case retbleed_mitigation still has its initialized
value of RETBLEED_MITIGATION_NONE.
> > Also, I think this pr_err() is redundant with RETBLEED_INTEL_MSG and can
> > be removed.
>
> IMO, they both serve a different purpose, RETBLEED_INTEL_MSG is a
> generic warning, and the message with !CONFIG_CALL_DEPTH_TRACKING hints
> a corrective action.
>
> If you look at "case RETBLEED_CMD_STUFF" it also prints a separate warning for
> !CONFIG_CALL_DEPTH_TRACKING:
Yep, makes sense.
On Thu, Feb 08, 2024 at 05:12:15PM -0800, Pawan Gupta wrote:
> - * The Intel mitigation (IBRS or eIBRS) was already selected in
> + * If Intel mitigation (IBRS or eIBRS) was already selected in
> * spectre_v2_select_mitigation(). 'retbleed_mitigation' will
^
should be a comma
> * be set accordingly below.
On Fri, Feb 09, 2024 at 04:43:28PM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 08, 2024 at 05:12:15PM -0800, Pawan Gupta wrote:
> > - * The Intel mitigation (IBRS or eIBRS) was already selected in
> > + * If Intel mitigation (IBRS or eIBRS) was already selected in
> > * spectre_v2_select_mitigation(). 'retbleed_mitigation' will
>
> ^
> should be a comma
Ok, will fix. Thanks for the review.
On Fri, Feb 09, 2024 at 12:33:46PM -0800, Pawan Gupta wrote:
> > And what's the point of checking spectre_v2= anyway? Shouldn't retbleed
> > be mitigated by default, independently of whatever the user may have
> > specified for Spectre v2?
>
> @Borislav, what do you think about this approach?
Looks like there is no objection. I will send the next version with
retbleed mitigation enabled by default.
@@ -50,6 +50,8 @@ static void __init l1d_flush_select_mitigation(void);
static void __init srso_select_mitigation(void);
static void __init gds_select_mitigation(void);
+static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void);
+
/* The base value of the SPEC_CTRL MSR without task-specific bits set */
u64 x86_spec_ctrl_base;
EXPORT_SYMBOL_GPL(x86_spec_ctrl_base);
@@ -892,6 +894,20 @@ static int __init nospectre_v1_cmdline(char *str)
}
early_param("nospectre_v1", nospectre_v1_cmdline);
+/* The kernel command line selection for spectre v2 */
+enum spectre_v2_mitigation_cmd {
+ SPECTRE_V2_CMD_NONE,
+ SPECTRE_V2_CMD_AUTO,
+ SPECTRE_V2_CMD_FORCE,
+ SPECTRE_V2_CMD_RETPOLINE,
+ SPECTRE_V2_CMD_RETPOLINE_GENERIC,
+ SPECTRE_V2_CMD_RETPOLINE_LFENCE,
+ SPECTRE_V2_CMD_EIBRS,
+ SPECTRE_V2_CMD_EIBRS_RETPOLINE,
+ SPECTRE_V2_CMD_EIBRS_LFENCE,
+ SPECTRE_V2_CMD_IBRS,
+};
+
enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init = SPECTRE_V2_NONE;
#undef pr_fmt
@@ -1025,10 +1041,17 @@ static void __init retbleed_select_mitigation(void)
retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
else if (IS_ENABLED(CONFIG_CPU_IBPB_ENTRY) && boot_cpu_has(X86_FEATURE_IBPB))
retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
+ } else if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+ spectre_v2_parse_cmdline() == SPECTRE_V2_CMD_AUTO &&
+ spectre_v2_enabled == SPECTRE_V2_RETPOLINE) {
+ if (IS_ENABLED(CONFIG_CALL_DEPTH_TRACKING))
+ retbleed_mitigation = RETBLEED_MITIGATION_STUFF;
+ else
+ pr_err("WARNING: Retpoline enabled, but kernel not compiled with CALL_DEPTH_TRACKING.\n");
}
/*
- * The Intel mitigation (IBRS or eIBRS) was already selected in
+ * If Intel mitigation (IBRS or eIBRS) was already selected in
* spectre_v2_select_mitigation(). 'retbleed_mitigation' will
* be set accordingly below.
*/
@@ -1157,20 +1180,6 @@ static inline bool match_option(const char *arg, int arglen, const char *opt)
return len == arglen && !strncmp(arg, opt, len);
}
-/* The kernel command line selection for spectre v2 */
-enum spectre_v2_mitigation_cmd {
- SPECTRE_V2_CMD_NONE,
- SPECTRE_V2_CMD_AUTO,
- SPECTRE_V2_CMD_FORCE,
- SPECTRE_V2_CMD_RETPOLINE,
- SPECTRE_V2_CMD_RETPOLINE_GENERIC,
- SPECTRE_V2_CMD_RETPOLINE_LFENCE,
- SPECTRE_V2_CMD_EIBRS,
- SPECTRE_V2_CMD_EIBRS_RETPOLINE,
- SPECTRE_V2_CMD_EIBRS_LFENCE,
- SPECTRE_V2_CMD_IBRS,
-};
-
enum spectre_v2_user_cmd {
SPECTRE_V2_USER_CMD_NONE,
SPECTRE_V2_USER_CMD_AUTO,