by Ingo Molnar

[permalink] [raw]

Subject: [PATCH] x86/speculation: Clean up various Spectre related details

* David Woodhouse <[email protected]> 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

=========================>
From 82c2b2f29691143a05181333f387e786646aa28b Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
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 <[email protected]>
Cc: Arjan van de Ven <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dan Williams <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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");

2018-02-11 19:26:23

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 :)

Attachments:

smime.p7s (5.09 kB)

2018-02-12 16:22:36

by Dave Hansen

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

On 02/12/2018 02:22 AM, Ingo Molnar wrote:
>> +static inline void firmware_restrict_branch_speculation_end(void)
>> +{
>> + alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,
>> + X86_FEATURE_USE_IBRS_FW);
> BTW., there's a detail that only occurred to me today, this enabling/disabling
> sequence is not NMI safe, and it might be called from NMI context:

FWIW, Tim Chen and I talked about this a bunch. We ended up just
saving/restoring the MSR verbatim in the NMI handler the same way we do
CR3, stashing it in a high general-purpose-register (r%12?). That costs
a RDMSR (at least) and an WRMSR (which you can optimize out). We have a
patch for that somewhere if anybody wants it.

We also came to the same conclusion that it's a rather challenging thing
to exploit these cases, especially when you consider that we can easily
do RSB stuffing on NMI exit just before going back to the firmware.

2018-02-12 16:32:13

by David Woodhouse

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

On Mon, 2018-02-12 at 11:29 +0530, afzal mohammed wrote:
> Hi,
>
> On Sun, Feb 11, 2018 at 11:19:10AM -0800, tip-bot for David Woodhouse wrote:
>
> >
> > x86/speculation: Use IBRS if available before calling into firmware
> >
> > Retpoline means the kernel is safe because it has no indirect branches.
> > But firmware isn't, so use IBRS for firmware calls if it's available.
>
> afaui, so only retpoline means still mitigation not enough.
>
> Also David W has mentioned [1] that even with retpoline, IBPB is also
> required (except Sky Lake).

Retpoline is sufficient to protect the *kernel*, which is the biggest
target. (Except on Skylake, where IBRS is the only full mitigation and
people are still working trying to come up with a "good enough"
mitigation that isn't IBRS.)

On all CPUs, you need IBPB to protect userspace processes from each
other, although since it's slow we don't actually *do* that for every
context switch; only when switching to non-dumpable processes.

That IBPB requirement for protecting userspace is true even on the next
generation of CPUs with the "Enhanced IBRS" (IBRS_ALL) feature. It only
goes away in CPUs which are even *further* in the future, when Intel
manage to fix it completely in hardware. They haven't even documented
the feature bit they're going to advertise to indicate that fix yet!

> If IBPB & IBRS is not supported by ucode, shouldn't the below indicate
> some thing on the lines of Mitigation not enough ?
>
> >
> > - return sprintf(buf, "%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> > + return sprintf(buf, "%s%s%s%s\n", spectre_v2_strings[spectre_v2_enabled],
> >        boot_cpu_has(X86_FEATURE_USE_IBPB) ? ", IBPB" : "",
> > +        boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "",
> >        spectre_v2_module_string());
> On 4.16-rc1, w/ GCC 7.3.0,
>
> /sys/devices/system/cpu/vulnerabilities/meltdown:Mitigation: PTI
> /sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user pointer sanitization
> /sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation: Full generic retpoline
>
> Here for the user (at least for me), it is not clear whether the
> mitigation is enough. In the present system (Ivy Bridge), as ucode
> update is not available, IBPB is not printed along with
> "spectre_v2:Mitigation", so unless i am missing something, till then
> this system should be considered vulnerable, but for a user not
> familiar with details of the issue, it cannot be deduced.
>
> Perhaps an additional status field [OKAY,PARTIAL] to Mitigation in
> sysfs might be helpful. All these changes are in the air for me, this
> is from a user perspective, sorry if my feedback seems idiotic.

Given that we only do it for non-dumpable processes, it's *always*
going to be only partial. (Although I think Thomas was looking at a
command line option to make that happen on every context switch?)

And on Skylake the current plan is that strictly speaking it would also
be partial.

I understand the concern, but I'm not sure that there's much we can do
to improve it. If it says "Mitigation:" that's generally OK, and if it
says anything else, it's not.

Attachments:

smime.p7s (5.09 kB)

2018-02-12 16:50:10

On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
>
> I did not update or otherwise change packages while I was bisecting; the
> machine is:
>
> vendor_id       : GenuineIntel
> cpu family      : 6
> model           : 62
> model name      : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> stepping        : 4
> microcode       : 0x428

That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
think there's a publicly available microcode that does; I assume you
didn't have one and build it into your kernel for early loading, and
thus you really weren't even using IBRS here? The code never even gets
patched in?

Attachments:

smime.p7s (5.09 kB)

2018-02-19 09:37:37

by Ingo Molnar

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

* Peter Zijlstra <[email protected]> wrote:

> On Sat, Feb 17, 2018 at 11:26:16AM +0100, Ingo Molnar wrote:
> > Note that PeterZ was struggling with intermittent boot hangs yesterday as well,
> > which hangs came and went during severeal (fruitless) bisection attempts. Then at
> > a certain point the hangs went away altogether.
> >
> > The symptoms for both his and your hangs are consistent with an alignment
> > dependent bug.
>
> Mine would consistently hang right after
>
> "Freeing SMP alternatives memory: 44K"
>
> At one point I bisected it to commit:
>
> a06cc94f3f8d ("x86/build: Drop superfluous ALIGN from the linker script")

So, just to make sure this commit had no effect: I cannot really see anything
wrong with that commit, it does a single substantial change, which is to remove
this explicit alignment:

. = ALIGN(8);
TEXT_TEXT

which seems fine to me, since it expanded to:

. = ALIGN(8);
ALIGN_FUNCTION(); \
...

which expanded to:

. = ALIGN(8);
. = ALIGN(8);
...

... which duplication the commit removed.

... where all the relevant defitions of TEXT_TEXT and ALIGN_FUNCTION are
unconditional and not overriden anywhere for arch/x86 builds.

I.e. the commit is a NOP AFAICS.

Thanks,

Ingo

2018-02-19 09:40:25

by Ingo Molnar

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

* David Woodhouse <[email protected]> wrote:

> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> >
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> >
> > vendor_id?????? : GenuineIntel
> > cpu family????? : 6
> > model?????????? : 62
> > model name????? : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping??????? : 4
> > microcode?????? : 0x428
>
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

Note that PeterZ's boot troubles only match the *symptoms* of the spurious
failures reported by Tim Chen. Your commit wasn't bisected to.

I linked these two reports on the (remote) possibility that they might be related
via some alignment dependent bug somewhere else in the x86 kernel - possibly
completely unrelated to any IBRS/IBPB details.

Thanks,

Ingo

2018-02-19 09:45:32

by David Woodhouse

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

On Mon, 2018-02-19 at 10:39 +0100, Ingo Molnar wrote:
> * David Woodhouse <[email protected]> wrote:
>
> >
> > On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> > >
> > >
> > > I did not update or otherwise change packages while I was bisecting; the
> > > machine is:
> > >
> > > vendor_id       : GenuineIntel
> > > cpu family      : 6
> > > model           : 62
> > > model name      : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > > stepping        : 4
> > > microcode       : 0x428
> > That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> > think there's a publicly available microcode that does; I assume you
> > didn't have one and build it into your kernel for early loading, and
> > thus you really weren't even using IBRS here? The code never even gets
> > patched in?
> Note that PeterZ's boot troubles only match the *symptoms* of the spurious
> failures reported by Tim Chen. Your commit wasn't bisected to.
>
> I linked these two reports on the (remote) possibility that they might be related
> via some alignment dependent bug somewhere else in the x86 kernel - possibly
> completely unrelated to any IBRS/IBPB details.

Understood. I was merely *accentuating* the "completely unrelated to
any IBRS/IBPB" bit, in a "la la la I'm ignoring this because I'm
working on other things" kind of a way... :)

Attachments:

smime.p7s (5.09 kB)

2018-02-19 10:09:42

by Peter Zijlstra

[permalink] [raw]

Subject: Re: [tip:x86/pti] x86/speculation: Use IBRS if available before calling into firmware

On Mon, Feb 19, 2018 at 09:29:12AM +0000, David Woodhouse wrote:
> On Mon, 2018-02-19 at 10:20 +0100, Peter Zijlstra wrote:
> >
> > I did not update or otherwise change packages while I was bisecting; the
> > machine is:
> >
> > vendor_id?????? : GenuineIntel
> > cpu family????? : 6
> > model?????????? : 62
> > model name????? : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
> > stepping??????? : 4
> > microcode?????? : 0x428
>
> That's IVX with a microcode that doesn't *have* IBRS/IBPB. I don't
> think there's a publicly available microcode that does; I assume you
> didn't have one and build it into your kernel for early loading, and
> thus you really weren't even using IBRS here? The code never even gets
> patched in?

I wasn't using IRBS afaik. I was bisceting tip/master before the
IBRS/firmware patches (not sure they're in now or not).

No fancy ucode setup, I think I have ucode image in the initrd, but that
would be same for all kernels.