Despite the fact that all the other code there seems to be doing it,
just using set_cpu_cap() in early_intel_init() doesn't actually work.
When the CPU is queried again in identify_boot_cpu(), it all gets
overwritten again. Do it in init_scattered_cpuid_features() instead.
Turning the bits off for bad microcode can use setup_clear_cpu_cap()
to force them off for all CPUs; I was less keen on forcing the feature
bits *on* that way.
Signed-off-by: David Woodhouse <[email protected]>
Fixes: 2961298e ("x86/cpufeatures: Clean up Spectre v2 related CPUID flags")
---
I feel I must be missing something. Is the rest of early_init_intel() broken too?
arch/x86/kernel/cpu/intel.c | 27 ++++++++-------------------
arch/x86/kernel/cpu/scattered.c | 13 +++++++++++++
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 0c8b916..4cf4f8c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -175,28 +175,17 @@ static void early_init_intel(struct cpuinfo_x86 *c)
if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64))
c->microcode = intel_get_microcode_revision();
- /*
- * The Intel SPEC_CTRL CPUID bit implies IBRS and IBPB support,
- * and they also have a different bit for STIBP support. Also,
- * a hypervisor might have set the individual AMD bits even on
- * Intel CPUs, for finer-grained selection of what's available.
- */
- if (cpu_has(c, X86_FEATURE_SPEC_CTRL)) {
- set_cpu_cap(c, X86_FEATURE_IBRS);
- set_cpu_cap(c, X86_FEATURE_IBPB);
- }
- if (cpu_has(c, X86_FEATURE_INTEL_STIBP))
- set_cpu_cap(c, X86_FEATURE_STIBP);
-
/* Now if any of them are set, check the blacklist and clear the lot */
- if ((cpu_has(c, X86_FEATURE_IBRS) || cpu_has(c, X86_FEATURE_IBPB) ||
+ if ((cpu_has(c, X86_FEATURE_SPEC_CTRL) ||
+ cpu_has(c, X86_FEATURE_INTEL_STIBP) ||
+ cpu_has(c, X86_FEATURE_IBRS) || cpu_has(c, X86_FEATURE_IBPB) ||
cpu_has(c, X86_FEATURE_STIBP)) && bad_spectre_microcode(c)) {
pr_warn("Intel Spectre v2 broken microcode detected; disabling Speculation Control\n");
- clear_cpu_cap(c, X86_FEATURE_IBRS);
- clear_cpu_cap(c, X86_FEATURE_IBPB);
- clear_cpu_cap(c, X86_FEATURE_STIBP);
- clear_cpu_cap(c, X86_FEATURE_SPEC_CTRL);
- clear_cpu_cap(c, X86_FEATURE_INTEL_STIBP);
+ setup_clear_cpu_cap(X86_FEATURE_IBRS);
+ setup_clear_cpu_cap(X86_FEATURE_IBPB);
+ setup_clear_cpu_cap(X86_FEATURE_STIBP);
+ setup_clear_cpu_cap(X86_FEATURE_SPEC_CTRL);
+ setup_clear_cpu_cap(X86_FEATURE_INTEL_STIBP);
}
/*
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index df11f5d..121b91e 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -53,6 +53,19 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
if (regs[cb->reg] & (1 << cb->bit))
set_cpu_cap(c, cb->feature);
}
+
+ /*
+ * The Intel SPEC_CTRL CPUID bit implies IBRS and IBPB support,
+ * and they also have a different bit for STIBP support. Also,
+ * a hypervisor might have set the individual AMD bits even on
+ * Intel CPUs, for finer-grained selection of what's available.
+ */
+ if (cpu_has(c, X86_FEATURE_SPEC_CTRL)) {
+ set_cpu_cap(c, X86_FEATURE_IBRS);
+ set_cpu_cap(c, X86_FEATURE_IBPB);
+ }
+ if (cpu_has(c, X86_FEATURE_INTEL_STIBP))
+ set_cpu_cap(c, X86_FEATURE_STIBP);
}
u32 get_scattered_cpuid_leaf(unsigned int level, unsigned int sub_leaf,
--
2.7.4
On Mon, 29 Jan 2018, David Woodhouse wrote:
> Despite the fact that all the other code there seems to be doing it,
> just using set_cpu_cap() in early_intel_init() doesn't actually work.
>
> When the CPU is queried again in identify_boot_cpu(), it all gets
> overwritten again. Do it in init_scattered_cpuid_features() instead.
>
> Turning the bits off for bad microcode can use setup_clear_cpu_cap()
> to force them off for all CPUs; I was less keen on forcing the feature
> bits *on* that way.
>
> Signed-off-by: David Woodhouse <[email protected]>
> Fixes: 2961298e ("x86/cpufeatures: Clean up Spectre v2 related CPUID flags")
> ---
> I feel I must be missing something. Is the rest of early_init_intel() broken too?
A lot of that stuff is duct taped and works more by chance than by
design. Borislav is looking into cleaning that up already.
Thanks,
tglx
On Mon, Jan 29, 2018 at 11:49:33PM +0000, David Woodhouse wrote:
> Despite the fact that all the other code there seems to be doing it,
> just using set_cpu_cap() in early_intel_init() doesn't actually work.
>
> When the CPU is queried again in identify_boot_cpu(), it all gets
> overwritten again. Do it in init_scattered_cpuid_features() instead.
>
> Turning the bits off for bad microcode can use setup_clear_cpu_cap()
> to force them off for all CPUs; I was less keen on forcing the feature
> bits *on* that way.
>
> Signed-off-by: David Woodhouse <[email protected]>
> Fixes: 2961298e ("x86/cpufeatures: Clean up Spectre v2 related CPUID flags")
> ---
> I feel I must be missing something. Is the rest of early_init_intel() broken too?
Does that help?
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 6936d14d4c77..1dd596d0a6c4 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -182,21 +182,21 @@ static void early_init_intel(struct cpuinfo_x86 *c)
* Intel CPUs, for finer-grained selection of what's available.
*/
if (cpu_has(c, X86_FEATURE_SPEC_CTRL)) {
- set_cpu_cap(c, X86_FEATURE_IBRS);
- set_cpu_cap(c, X86_FEATURE_IBPB);
+ setup_force_cpu_cap(X86_FEATURE_IBRS);
+ setup_force_cpu_cap(X86_FEATURE_IBPB);
}
if (cpu_has(c, X86_FEATURE_INTEL_STIBP))
- set_cpu_cap(c, X86_FEATURE_STIBP);
+ setup_force_cpu_cap(X86_FEATURE_STIBP);
/* Now if any of them are set, check the blacklist and clear the lot */
if ((cpu_has(c, X86_FEATURE_IBRS) || cpu_has(c, X86_FEATURE_IBPB) ||
cpu_has(c, X86_FEATURE_STIBP)) && bad_spectre_microcode(c)) {
pr_warn("Intel Spectre v2 broken microcode detected; disabling Speculation Control\n");
- clear_cpu_cap(c, X86_FEATURE_IBRS);
- clear_cpu_cap(c, X86_FEATURE_IBPB);
- clear_cpu_cap(c, X86_FEATURE_STIBP);
- clear_cpu_cap(c, X86_FEATURE_SPEC_CTRL);
- clear_cpu_cap(c, X86_FEATURE_INTEL_STIBP);
+ setup_clear_cpu_cap(X86_FEATURE_IBRS);
+ setup_clear_cpu_cap(X86_FEATURE_IBPB);
+ setup_clear_cpu_cap(X86_FEATURE_STIBP);
+ setup_clear_cpu_cap(X86_FEATURE_SPEC_CTRL);
+ setup_clear_cpu_cap(X86_FEATURE_INTEL_STIBP);
}
/*
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Tue, 2018-01-30 at 11:58 +0100, Borislav Petkov wrote:
>
> Does that help?
>
> diff --git a/arch/x86/kernel/cpu/intel.c
> b/arch/x86/kernel/cpu/intel.c
> index 6936d14d4c77..1dd596d0a6c4 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -182,21 +182,21 @@ static void early_init_intel(struct cpuinfo_x86
> *c)
> * Intel CPUs, for finer-grained selection of what's
> available.
> */
> if (cpu_has(c, X86_FEATURE_SPEC_CTRL)) {
> - set_cpu_cap(c, X86_FEATURE_IBRS);
> - set_cpu_cap(c, X86_FEATURE_IBPB);
> + setup_force_cpu_cap(X86_FEATURE_IBRS);
> + setup_force_cpu_cap(X86_FEATURE_IBPB);
> }
> if (cpu_has(c, X86_FEATURE_INTEL_STIBP))
> - set_cpu_cap(c, X86_FEATURE_STIBP);
> + setup_force_cpu_cap(X86_FEATURE_STIBP);
I pondered that, but I didn't like it. I didn't want to always *force*
those features on, for all CPUs, just because they happened to be
discovered at boot time on the first CPU (which *did* have its
microcode updated by the crappy BIOS, while the others didn't).
I strongly suspect that's purely an academic concern, and we mostly
check boot_cpu_has() and never even *notice* if secondary CPUs don't
match. I just didn't want to make that *worse*. It tickled my OCD.
On Tue, Jan 30, 2018 at 11:03:50AM +0000, David Woodhouse wrote:
> I pondered that, but I didn't like it. I didn't want to always *force*
> those features on, for all CPUs, just because they happened to be
> discovered at boot time on the first CPU (which *did* have its
> microcode updated by the crappy BIOS, while the others didn't).
>
> I strongly suspect that's purely an academic concern, and we mostly
> check boot_cpu_has() and never even *notice* if secondary CPUs don't
> match. I just didn't want to make that *worse*. It tickled my OCD.
Well, you need to do it because those bits are AMD-specific and they are
not set in the Intel CPUID leaf and identify_cpu() towards the end takes
care of "ironing" all those bits out which are not part of the common
feature set and which get_cpu_cap() has *not* read out from CPUID.
It is one of those I-told-you-so moments when I suggested to make the
visible feature bits the artificial ones and have the *actual* hardware
ones set those.
:-)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On Tue, 2018-01-30 at 12:18 +0100, Borislav Petkov wrote:
> On Tue, Jan 30, 2018 at 11:03:50AM +0000, David Woodhouse wrote:
> >
> > I pondered that, but I didn't like it. I didn't want to always *force*
> > those features on, for all CPUs, just because they happened to be
> > discovered at boot time on the first CPU (which *did* have its
> > microcode updated by the crappy BIOS, while the others didn't).
> >
> > I strongly suspect that's purely an academic concern, and we mostly
> > check boot_cpu_has() and never even *notice* if secondary CPUs don't
> > match. I just didn't want to make that *worse*. It tickled my OCD.
>
> Well, you need to do it because those bits are AMD-specific and they are
> not set in the Intel CPUID leaf and identify_cpu() towards the end takes
> care of "ironing" all those bits out which are not part of the common
> feature set and which get_cpu_cap() has *not* read out from CPUID.
I need to set them for each CPU which has the Intel hardware bits set,
sure. I don't need to use setup_force_cpu_cap() to do it. The patch I
sent was doing it for each CPU.
> It is one of those I-told-you-so moments when I suggested to make the
> visible feature bits the artificial ones and have the *actual* hardware
> ones set those.
We don't have artificial ones for the hardware capability, but yes I
could add another three. I could add X86_FEATURE_IBRS which is a
virtual bit, set when *either* X86_FEATURE_SPEC_CTRL (on Intel) or
X86_FEATURE_AMD_IBRS (on AMD) is set.
But actually... that doesn't help, does it? Because early_init_intel()
is still only called *once* for the boot CPU. Those software bits would
be set... and perhaps not later cleared when identify_boot_cpu()
happens later, but would they ever get set for secondary CPUs? The code
to set those virtual bits would *still* need to live somewhere that
will get called for secondary CPUs, as I've done in this patch.
I could use setup_force_cpu_cap() but I still don't like that, as
discussed.
So no, I don't see why inventing three more "virtual" bits to precisely
parallel the AMD bits would really make much difference.
On Tue, 30 Jan 2018, David Woodhouse wrote:
> On Tue, 2018-01-30 at 12:18 +0100, Borislav Petkov wrote:
> > On Tue, Jan 30, 2018 at 11:03:50AM +0000, David Woodhouse wrote:
> > >
> > > I pondered that, but I didn't like it. I didn't want to always *force*
> > > those features on, for all CPUs, just because they happened to be
> > > discovered at boot time on the first CPU (which *did* have its
> > > microcode updated by the crappy BIOS, while the others didn't).
> > >
> > > I strongly suspect that's purely an academic concern, and we mostly
> > > check boot_cpu_has() and never even *notice* if secondary CPUs don't
> > > match. I just didn't want to make that *worse*. It tickled my OCD.
> >
> > Well, you need to do it because those bits are AMD-specific and they are
> > not set in the Intel CPUID leaf and identify_cpu() towards the end takes
> > care of "ironing" all those bits out which are not part of the common
> > feature set and which get_cpu_cap() has *not* read out from CPUID.
>
> I need to set them for each CPU which has the Intel hardware bits set,
> sure. I don't need to use setup_force_cpu_cap() to do it. The patch I
> sent was doing it for each CPU.
>
> > It is one of those I-told-you-so moments when I suggested to make the
> > visible feature bits the artificial ones and have the *actual* hardware
> > ones set those.
>
> We don't have artificial ones for the hardware capability, but yes I
> could add another three. I could add X86_FEATURE_IBRS which is a
> virtual bit, set when *either* X86_FEATURE_SPEC_CTRL (on Intel) or
> X86_FEATURE_AMD_IBRS (on AMD) is set.
>
> But actually... that doesn't help, does it? Because early_init_intel()
> is still only called *once* for the boot CPU. Those software bits would
> be set... and perhaps not later cleared when identify_boot_cpu()
> happens later, but would they ever get set for secondary CPUs? The code
> to set those virtual bits would *still* need to live somewhere that
> will get called for secondary CPUs, as I've done in this patch.
>
> I could use setup_force_cpu_cap() but I still don't like that, as
> discussed.
>
> So no, I don't see why inventing three more "virtual" bits to precisely
> parallel the AMD bits would really make much difference.
In any case, if there is ucode mismatch between CPUs the whole thing is
hosed anyway no matter what. So can you please agree on a solution so we
can unbreak the current state of affairs?
Thanks,
tglx
On Tue, 2018-01-30 at 12:37 +0100, Thomas Gleixner wrote:
> In any case, if there is ucode mismatch between CPUs the whole thing is
> hosed anyway no matter what. So can you please agree on a solution so we
> can unbreak the current state of affairs?
If there is µcode mismatch between CPUs then the inconsistent bits
should be filtered down to the lowest common denominator and we
shouldn't use the features that are not consistently present. That much
ought to work already with my patch.
Boris's version uses setup_force_cpu_cap() and forces the bit to be set
even on secondary CPUs which don't really have it, and thus it won't
get filtered out. We'll try to use it, and it will fault on the CPUs
which don't have it.
That's the difference.
On Tue, 30 Jan 2018, David Woodhouse wrote:
> On Tue, 2018-01-30 at 12:37 +0100, Thomas Gleixner wrote:
>
> > In any case, if there is ucode mismatch between CPUs the whole thing is
> > hosed anyway no matter what. So can you please agree on a solution so we
> > can unbreak the current state of affairs?
>
> If there is µcode mismatch between CPUs then the inconsistent bits
> should be filtered down to the lowest common denominator and we
> shouldn't use the features that are not consistently present. That much
> ought to work already with my patch.
>
> Boris's version uses setup_force_cpu_cap() and forces the bit to be set
> even on secondary CPUs which don't really have it, and thus it won't
> get filtered out. We'll try to use it, and it will fault on the CPUs
> which don't have it.
So much for the theory. That's not going to work. If the boot cpu has the
feature then the alternatives will have been applied. So even if the flag
mismatch can be observed when a secondary CPU comes up the outcome will be
access to a non existing MSR and #GP.
The whole per cpu feature flag magic in x86 is just an empty shell
providing the illusion of supporting heterogenous systems. If that "works" in
a particular constellation then by pure chance and not by design.
All you can reasonably do is to detect the mismatch once the CPU is brought
up and then immediately aborting the hotplug operation _before_ it has the
chance to touch anything. But that does not necessarily require per cpu
storage.
Thanks,
tglx
On Tue, Jan 30, 2018 at 01:57:21PM +0100, Thomas Gleixner wrote:
> So much for the theory. That's not going to work. If the boot cpu has the
> feature then the alternatives will have been applied. So even if the flag
> mismatch can be observed when a secondary CPU comes up the outcome will be
> access to a non existing MSR and #GP.
Yes, with mismatched microcode we're f*cked.
So my question is: is there such microcode out there or is this
something theoretical which we want to address?
(.. and adressing this will be ugly, no matter what.)
And if I were able to wish, I'd like to blacklist that microcode in
dracut so that it doesn't come anywhere near my system.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 1/30/2018 5:11 AM, Borislav Petkov wrote:
> On Tue, Jan 30, 2018 at 01:57:21PM +0100, Thomas Gleixner wrote:
>> So much for the theory. That's not going to work. If the boot cpu has the
>> feature then the alternatives will have been applied. So even if the flag
>> mismatch can be observed when a secondary CPU comes up the outcome will be
>> access to a non existing MSR and #GP.
>
> Yes, with mismatched microcode we're f*cked.
I think in the super early days of SMP there was an occasional broken BIOS.
(and when Linux then did the ucode update it was sane again)
Not since a long time though (I think the various certification suites check for it now)
>
> So my question is: is there such microcode out there or is this
> something theoretical which we want to address?
at this point it's insane theoretical; no OS can actually cope with this, so
if you're an OEM selling this, your customer can run zero OSes ;-)
>
> (.. and adressing this will be ugly, no matter what.)
>
> And if I were able to wish, I'd like to blacklist that microcode in
> dracut so that it doesn't come anywhere near my system.
I'm not sure what you'd want dracut to do... panic() the system
on such a bios?
On Tue, 30 Jan 2018 06:01:43 -0800
Arjan van de Ven <[email protected]> wrote:
> On 1/30/2018 5:11 AM, Borislav Petkov wrote:
> > On Tue, Jan 30, 2018 at 01:57:21PM +0100, Thomas Gleixner wrote:
> >> So much for the theory. That's not going to work. If the boot cpu has the
> >> feature then the alternatives will have been applied. So even if the flag
> >> mismatch can be observed when a secondary CPU comes up the outcome will be
> >> access to a non existing MSR and #GP.
> >
> > Yes, with mismatched microcode we're f*cked.
>
> I think in the super early days of SMP there was an occasional broken BIOS.
> (and when Linux then did the ucode update it was sane again)
>
> Not since a long time though (I think the various certification suites check for it now)
The only case I can think of where you'd get a non boot processor that
didn't have the same microcode loaded as the rest on entry to the OS would
be in a system where it was possibly to phyically hot plug processors
post boot.
There are not many such systems and it may be that all of them do
sufficient deeply unmentionable things in their firmware to cover this.
Alan
On Tue, 2018-01-30 at 14:54 +0000, Alan Cox wrote:
> The only case I can think of where you'd get a non boot processor that
> didn't have the same microcode loaded as the rest on entry to the OS would
> be in a system where it was possibly to phyically hot plug processors
> post boot.
>
> There are not many such systems and it may be that all of them do
> sufficient deeply unmentionable things in their firmware to cover this.
We've got a patch lurking somewhere to properly collect the return code
from microcode loading on all CPUs, because we've seen it *fail* on one
CPU. Leaving them inconsistent, and on a kexec after that we really
*did* see the boot CPU with IBRS support and a secondary without it.
But really, my point in this patch was not that I expect all this to
work nicely, but that I don't want to make things *worse* by using
setup_force_cpu_cap() when really I only want to set the bit for *this*
CPU.
I'm *all* for ditching the per-CPU bitmasks since they're largely
pointless and we've applied alternatives before the secondaries are
brought up anyway. It's just a rabbithole I didn't need to go down
today.
On Tue, 30 Jan 2018, Borislav Petkov wrote:
> On Tue, Jan 30, 2018 at 01:57:21PM +0100, Thomas Gleixner wrote:
> > So much for the theory. That's not going to work. If the boot cpu has the
> > feature then the alternatives will have been applied. So even if the flag
> > mismatch can be observed when a secondary CPU comes up the outcome will be
> > access to a non existing MSR and #GP.
>
> Yes, with mismatched microcode we're f*cked.
>
> So my question is: is there such microcode out there or is this
> something theoretical which we want to address?
Old mixed-stepping systems would have mismatched microcode even when
everything went right, just because they had different processor
steppings that took different microcode.
Those systems could have mismatched CPU flags (regardless of their
microcode being up-to-date or not) simply because the processor with the
newer stepping might have more features. This happened at least once
during the FSB-era Xeons. I came across several (large three-letter
vendor) x86 Intel-based servers that were in such a condition, *all
using official parts*, because they got upgraded with a second processor
sometime after they were boght, and the part number delivered by said
three-letter vendor was a newer stepping.
You will notice those were all *fully and officially supported* hardware
configurations, and documented as such in the processor specification
updates.
There was also a crop of UEFI and BIOSes out there that would not
properly update the microcode on all processor cores, several years ago.
> And if I were able to wish, I'd like to blacklist that microcode in
> dracut so that it doesn't come anywhere near my system.
Well, the microcode update of a core can soft-fail, and the fact is that
we don't handle such failures at all in any way. Not that I know what
would be the right error handling for something like this... kicking
the processor offline (or refusing to online it) might be a good way to
handle it. And one could at least retry once... Anyway, we don't even
report it, which is certainly Not Good Enough.
That said, iucode-tool v2.3, released a couple days ago, can do
revision-based blacklisting of microcode[1], if you want to fine-comb
the stuff you're going to ship to your users, etc.
[1] iucode-tool is somewhat widely used *outside* of the Fedora/RedHat
ecosystem, but not even packaged by Fedora, AFAIK. So, it won't be
relevant to several people in the To/CC list ;-)
--
Henrique Holschuh