diff mbox

x86/speculation: Clean up various Spectre related details

Message ID 20180211185057.rest4bf2ydx7slrk@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ingo Molnar Feb. 11, 2018, 6:50 p.m. UTC 
* David Woodhouse <dwmw@amazon.co.uk> wrote:

> +	/*
> +	 * Retpoline means the kernel is safe because it has no indirect
> +	 * branches. But firmware isn't, so use IBRS to protect that.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_IBRS)) {
> +		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
> +		pr_info("Enabling Restricted Speculation for firmware calls\n");
> +	}

I have changed this text to say:

		pr_info("Spectre mitigation: Restricting branch speculation (enabling IBRS) for firmware calls\n");

In fact while at it I found and improved a few other details as well, such as:

 	 * Retpoline means the kernel is safe because it has no indirect
-	 * branches. But firmware isn't, so use IBRS to protect that.
+	 * branches. But we don't know whether the firmware is safe, so
+	 * use IBRS to protect against that:

most Spectre related messages are now harmonized:

arch/x86/kernel/cpu/bugs.c:             pr_info("Spectre mitigation: Filling RSB on context switch\n");
arch/x86/kernel/cpu/bugs.c:             pr_info("Spectre mitigation: Enabling Indirect Branch Prediction Barrier (IBPB)\n");
arch/x86/kernel/cpu/bugs.c:             pr_info("Spectre mitigation: Restricting branch speculation (enabling IBRS) for firmware calls\n");

Find the full patch below.

Thanks,

	Ingo

Comments

David Woodhouse Feb. 11, 2018, 7:25 p.m. UTC | #1 
On Sun, 2018-02-11 at 19:50 +0100, Ingo Molnar wrote:
> 
> From 82c2b2f29691143a05181333f387e786646aa28b Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@kernel.org>
> Date: Sat, 10 Feb 2018 11:51:57 +0100
> Subject: [PATCH] x86/speculation: Clean up various Spectre related details
> 
> Harmonize all the Spectre messages so that a:
> 
>     dmesg | grep -i spectre
> 
> ... gives us most Spectre related kernel boot messages.
> 
> Also fix a few other details:
> 
>  - clarify a comment about firmware speculation control
> 
>  - s/KPTI/PTI
> 
>  - remove various line-breaks that made the code uglier
> 
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>



Acked-by: David Woodhouse <dwmw@amazon.co.uk>

with a couple of comments:


-        * If neither SMEP or KPTI are available, there is a risk of
+        * If neither SMEP or PTI are available, there is a risk of

Make that 'neither SMEP nor PTI' while you're at it though please;
that's bugged me a couple of times in passing.

And should these say 'Spectre v2' not just 'Spectre'?
Ingo Molnar Feb. 11, 2018, 7:43 p.m. UTC | #2 
* David Woodhouse <dwmw2@infradead.org> wrote:

> 
> 
> On Sun, 2018-02-11 at 19:50 +0100, Ingo Molnar wrote:
> > 
> > From 82c2b2f29691143a05181333f387e786646aa28b Mon Sep 17 00:00:00 2001
> > From: Ingo Molnar <mingo@kernel.org>
> > Date: Sat, 10 Feb 2018 11:51:57 +0100
> > Subject: [PATCH] x86/speculation: Clean up various Spectre related details
> > 
> > Harmonize all the Spectre messages so that a:
> > 
> >     dmesg | grep -i spectre
> > 
> > ... gives us most Spectre related kernel boot messages.
> > 
> > Also fix a few other details:
> > 
> >  - clarify a comment about firmware speculation control
> > 
> >  - s/KPTI/PTI
> > 
> >  - remove various line-breaks that made the code uglier
> > 
> > Cc: Andy Lutomirski <luto@kernel.org>
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: David Woodhouse <dwmw@amazon.co.uk>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> 
> 
> 
> Acked-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks, added.

> with a couple of comments:
> 
> 
> -        * If neither SMEP or KPTI are available, there is a risk of
> +        * If neither SMEP or PTI are available, there is a risk of
> 
> Make that 'neither SMEP nor PTI' while you're at it though please;
> that's bugged me a couple of times in passing.

Ok, fixed that too.

> 
> And should these say 'Spectre v2' not just 'Spectre'?

Yeah, you are probably right, but I didn't want to make the messages too specific 
- do we really know that this is the end of Spectre-style speculation holes?

Thanks,

	Ingo
David Woodhouse Feb. 12, 2018, 3:30 p.m. UTC | #3 
On Sun, 2018-02-11 at 20:43 +0100, Ingo Molnar wrote:
> > And should these say 'Spectre v2' not just 'Spectre'?
> 
> Yeah, you are probably right, but I didn't want to make the messages too specific 
> - do we really know that this is the end of Spectre-style speculation holes?

Well... if a new problem is also remedied by use if IBRS/IBPB and
retpoline, I think we can happily call it a subclass of "Spectre v2".

And if it *isn't* addressed by those same things, then it's clearly
something different. Either way, these messages should be 'v2', no?

On the whole though, there are plenty of better things to be worrying
about :)
Ingo Molnar Feb. 13, 2018, 8:04 a.m. UTC | #4 
* David Woodhouse <dwmw2@infradead.org> wrote:

> On Sun, 2018-02-11 at 20:43 +0100, Ingo Molnar wrote:
> > > And should these say 'Spectre v2' not just 'Spectre'?
> > 
> > Yeah, you are probably right, but I didn't want to make the messages too specific 
> > - do we really know that this is the end of Spectre-style speculation holes?
> 
> Well... if a new problem is also remedied by use if IBRS/IBPB and
> retpoline, I think we can happily call it a subclass of "Spectre v2".
> 
> And if it *isn't* addressed by those same things, then it's clearly
> something different. Either way, these messages should be 'v2', no?

Ok, fair enough - I've changed it to v2 as you suggest:

-		pr_info("Filling RSB on context switch\n");
+		pr_info("Spectre v2 mitigation: Filling RSB on context switch\n");
-		pr_info("Enabling Indirect Branch Prediction Barrier\n");
+		pr_info("Spectre v2 mitigation: Enabling Indirect Branch Prediction Barrier\n");

> On the whole though, there are plenty of better things to be worrying
> about :)

Sure - nevertheless I fixed these while they were still hot ;-)

Thanks,

	Ingo
diff mbox

Patch

=========================>
From 82c2b2f29691143a05181333f387e786646aa28b Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@kernel.org>
Date: Sat, 10 Feb 2018 11:51:57 +0100
Subject: [PATCH] x86/speculation: Clean up various Spectre related details

Harmonize all the Spectre messages so that a:

    dmesg | grep -i spectre

... gives us most Spectre related kernel boot messages.

Also fix a few other details:

 - clarify a comment about firmware speculation control

 - s/KPTI/PTI

 - remove various line-breaks that made the code uglier

Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/bugs.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6f6d763225c8..eff45477fcca 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -162,8 +162,7 @@  static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 	if (cmdline_find_option_bool(boot_command_line, "nospectre_v2"))
 		return SPECTRE_V2_CMD_NONE;
 	else {
-		ret = cmdline_find_option(boot_command_line, "spectre_v2", arg,
-					  sizeof(arg));
+		ret = cmdline_find_option(boot_command_line, "spectre_v2", arg, sizeof(arg));
 		if (ret < 0)
 			return SPECTRE_V2_CMD_AUTO;
 
@@ -175,8 +174,7 @@  static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 		}
 
 		if (i >= ARRAY_SIZE(mitigation_options)) {
-			pr_err("unknown option (%s). Switching to AUTO select\n",
-			       mitigation_options[i].option);
+			pr_err("unknown option (%s). Switching to AUTO select\n", mitigation_options[i].option);
 			return SPECTRE_V2_CMD_AUTO;
 		}
 	}
@@ -185,8 +183,7 @@  static enum spectre_v2_mitigation_cmd __init spectre_v2_parse_cmdline(void)
 	     cmd == SPECTRE_V2_CMD_RETPOLINE_AMD ||
 	     cmd == SPECTRE_V2_CMD_RETPOLINE_GENERIC) &&
 	    !IS_ENABLED(CONFIG_RETPOLINE)) {
-		pr_err("%s selected but not compiled in. Switching to AUTO select\n",
-		       mitigation_options[i].option);
+		pr_err("%s selected but not compiled in. Switching to AUTO select\n", mitigation_options[i].option);
 		return SPECTRE_V2_CMD_AUTO;
 	}
 
@@ -256,14 +253,14 @@  static void __init spectre_v2_select_mitigation(void)
 			goto retpoline_auto;
 		break;
 	}
-	pr_err("kernel not compiled with retpoline; no mitigation available!");
+	pr_err("Spectre mitigation: kernel not compiled with retpoline; no mitigation available!");
 	return;
 
 retpoline_auto:
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 	retpoline_amd:
 		if (!boot_cpu_has(X86_FEATURE_LFENCE_RDTSC)) {
-			pr_err("LFENCE not serializing. Switching to generic retpoline\n");
+			pr_err("Spectre mitigation: LFENCE not serializing, switching to generic retpoline\n");
 			goto retpoline_generic;
 		}
 		mode = retp_compiler() ? SPECTRE_V2_RETPOLINE_AMD :
@@ -281,7 +278,7 @@  static void __init spectre_v2_select_mitigation(void)
 	pr_info("%s\n", spectre_v2_strings[mode]);
 
 	/*
-	 * If neither SMEP or KPTI are available, there is a risk of
+	 * If neither SMEP or PTI are available, there is a risk of
 	 * hitting userspace addresses in the RSB after a context switch
 	 * from a shallow call stack to a deeper one. To prevent this fill
 	 * the entire RSB, even when using IBRS.
@@ -295,30 +292,30 @@  static void __init spectre_v2_select_mitigation(void)
 	if ((!boot_cpu_has(X86_FEATURE_PTI) &&
 	     !boot_cpu_has(X86_FEATURE_SMEP)) || is_skylake_era()) {
 		setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
-		pr_info("Filling RSB on context switch\n");
+		pr_info("Spectre mitigation: Filling RSB on context switch\n");
 	}
 
 	/* Initialize Indirect Branch Prediction Barrier if supported */
 	if (boot_cpu_has(X86_FEATURE_IBPB)) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBPB);
-		pr_info("Enabling Indirect Branch Prediction Barrier\n");
+		pr_info("Spectre mitigation: Enabling Indirect Branch Prediction Barrier (IBPB)\n");
 	}
 
 	/*
 	 * Retpoline means the kernel is safe because it has no indirect
-	 * branches. But firmware isn't, so use IBRS to protect that.
+	 * branches. But we don't know whether the firmware is safe, so
+	 * use IBRS to protect against that:
 	 */
 	if (boot_cpu_has(X86_FEATURE_IBRS)) {
 		setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW);
-		pr_info("Enabling Restricted Speculation for firmware calls\n");
+		pr_info("Spectre mitigation: Restricting branch speculation (enabling IBRS) for firmware calls\n");
 	}
 }
 
 #undef pr_fmt
 
 #ifdef CONFIG_SYSFS
-ssize_t cpu_show_meltdown(struct device *dev,
-			  struct device_attribute *attr, char *buf)
+ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	if (!boot_cpu_has_bug(X86_BUG_CPU_MELTDOWN))
 		return sprintf(buf, "Not affected\n");
@@ -327,16 +324,14 @@  ssize_t cpu_show_meltdown(struct device *dev,
 	return sprintf(buf, "Vulnerable\n");
 }
 
-ssize_t cpu_show_spectre_v1(struct device *dev,
-			    struct device_attribute *attr, char *buf)
+ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V1))
 		return sprintf(buf, "Not affected\n");
 	return sprintf(buf, "Mitigation: __user pointer sanitization\n");
 }
 
-ssize_t cpu_show_spectre_v2(struct device *dev,
-			    struct device_attribute *attr, char *buf)
+ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
 		return sprintf(buf, "Not affected\n");