[PATCH] x86: Add check for P5 to microcode_intel_early

[PATCH] x86: Add check for P5 to microcode_intel_early

[Bryan O'Donoghue]

Architectural MSRs associated with microcode are for P6 or higher.
Add a check to early microcode to detect < P6.

Without a check for < P6 - we end up reading from unimplemented MSRs
on Pentium.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue.lkml@nexus-software.ie>
---
 arch/x86/kernel/microcode_intel_early.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index d893e8e..e1ff9bf 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -396,6 +396,9 @@ static int __cpuinit collect_cpu_info_early(struct ucode_cpu_info *uci)
 	x86 = get_x86_family(csig.sig);
 	x86_model = get_x86_model(csig.sig);
 
+	if (x86 < 6)
+		return UCODE_ERROR;
+
 	if ((x86_model >= 5) || (x86 > 6)) {
 		/* get processor flags from MSR 0x17 */
 		native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
@@ -428,9 +431,13 @@ static void __ref show_saved_mc(void)
 		pr_debug("no micorcode data saved.\n");
 		return;
 	}
-	pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
 
-	collect_cpu_info_early(&uci);
+	if (collect_cpu_info_early(&uci) == UCODE_ERROR) {
+		pr_debug("microcode unavailable on this architecture\n");
+		return;
+	}
+
+	pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
 
 	sig = uci.cpu_sig.sig;
 	pf = uci.cpu_sig.pf;
@@ -609,7 +616,8 @@ void __cpuinit show_ucode_info_early(void)
 	struct ucode_cpu_info uci;
 
 	if (delay_ucode_info) {
-		collect_cpu_info_early(&uci);
+		if (collect_cpu_info_early(&uci) == UCODE_ERROR)
+			return;
 		print_ucode_info(&uci, current_mc_date);
 		delay_ucode_info = 0;
 	}
@@ -724,7 +732,8 @@ _load_ucode_intel_bsp(struct mc_saved_data *mc_saved_data,
 		      unsigned long initrd_end_early,
 		      struct ucode_cpu_info *uci)
 {
-	collect_cpu_info_early(uci);
+	if (collect_cpu_info_early(uci) == UCODE_ERROR)
+		return;
 	scan_microcode(initrd_start_early, initrd_end_early, mc_saved_data,
 		       mc_saved_in_initrd, uci);
 	load_microcode(mc_saved_data, mc_saved_in_initrd,
@@ -789,7 +798,9 @@ void __cpuinit load_ucode_intel_ap(void)
 	if (mc_saved_data_p->mc_saved_count == 0)
 		return;
 
-	collect_cpu_info_early(&uci);
+	if (collect_cpu_info_early(&uci) == UCODE_ERROR)
+		return;
+
 	load_microcode(mc_saved_data_p, mc_saved_in_initrd_p,
 		       initrd_start_addr, &uci);
 	apply_microcode_early(mc_saved_data_p, &uci);
-- 
1.7.10.4

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Borislav Petkov]

On Fri, Apr 19, 2013 at 06:23:03PM +0100, Bryan O'Donoghue wrote:
> Architectural MSRs associated with microcode are for P6 or higher.
> Add a check to early microcode to detect < P6.
> 
> Without a check for < P6 - we end up reading from unimplemented MSRs
> on Pentium.

Is this something you're actually seeing on some box or just found by
staring at the code?

In any case, the family checks should go into the ucode driver entry
points in arch/x86/kernel/microcode_core_early.c. AFAICT, x86_vendor()
is a good candidate to be taught to read out the family too and return
X86_VENDOR_UNKNOWN if < P6. Or something to that effect.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Bryan O'Donoghue]

On 19/04/13 20:11, Borislav Petkov wrote:
> On Fri, Apr 19, 2013 at 06:23:03PM +0100, Bryan O'Donoghue wrote:
>> Architectural MSRs associated with microcode are for P6 or higher.
>> Add a check to early microcode to detect<  P6.
>>
>> Without a check for<  P6 - we end up reading from unimplemented MSRs
>> on Pentium.
>
> Is this something you're actually seeing on some box or just found by
> staring at the code?

We actually see this on a P5 alright.
The code path is reachable, no question.

> In any case, the family checks should go into the ucode driver entry
> points in arch/x86/kernel/microcode_core_early.c. AFAICT, x86_vendor()
> is a good candidate to be taught to read out the family too and return
> X86_VENDOR_UNKNOWN if<  P6. Or something to that effect.

Hmm

Just returning X86_VENDOR_UNKNOWN - won't fix the bug though - after all 
MSR_IA32_UCODE_REV is also x86_family() >= 6

x86 = get_x86_family(csig.sig);
x86_model = get_x86_model(csig.sig);

if (x86 < 6)
	return UCODE_ERROR;

if ((x86_model >= 5) || (x86 > 6)) {
	/* get processor flags from MSR 0x17 */
	native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
	csig.pf = 1 << ((val[1] >> 18) & 7);
}

native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);

/* As documented in the SDM: Do a CPUID 1 here */
sync_core();

/* get the current revision from MSR 0x8B */
native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

Peter - what's your take ?

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Borislav Petkov]

On Fri, Apr 19, 2013 at 09:35:18PM +0100, Bryan O'Donoghue wrote:
> Just returning X86_VENDOR_UNKNOWN - won't fix the bug though - after
> all MSR_IA32_UCODE_REV is also x86_family() >= 6

What are you talking about?

If you return X86_VENDOR_UNKNOWN from x86_vendor(),
load_ucode_intel_bsp() and load_ucode_intel_ap() won't be called and no
non-existing MSRs will be accessed.

Just filter out P5 and earlier. The code already does that for CPUs
which don't have CPUID.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Borislav Petkov]

On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
> Just filter out P5 and earlier. The code already does that for CPUs
> which don't have CPUID.

Actually, an alternative - more practical albeit not very accurate
solution would be to check for which families Intel delivers microcode
and do the cut off there with a comment as to why you do it like that.

I very much doubt, Intel will add microcode for *older* families to the
package :-).

IOW, in the linux intel-microcode package we currently have:

$ tree /lib/firmware/intel-ucode/
├── 06-0f-02
├── 06-0f-06
├── 06-0f-07
├── 06-0f-0a
├── 06-0f-0b
├── 06-0f-0d
├── 06-16-01
├── 06-17-06
├── 06-17-07
├── 06-17-0a
├── 06-1a-04
├── 06-1c-02
├── 06-1c-0a
├── 06-1d-01
├── 06-1e-04
├── 06-1e-05
├── 06-25-02
├── 06-25-05
├── 06-2a-07
├── 06-2d-06
├── 06-2d-07
├── 06-2f-02
├── 06-3a-09
├── 0f-04-01
├── 0f-04-03
├── 0f-04-04
├── 0f-04-07
├── 0f-04-08
├── 0f-04-09
├── 0f-04-0a
├── 0f-06-02
├── 0f-06-04
├── 0f-06-05
└── 0f-06-08

So < 6 should be fine.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Bryan O'Donoghue]

On 19/04/13 22:25, Borislav Petkov wrote:
> On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
>> Just filter out P5 and earlier. The code already does that for CPUs
>> which don't have CPUID.
>
> Actually, an alternative - more practical albeit not very accurate

More practical ? Hmm - the MSRs don't exist for < P5

> solution would be to check for which families Intel delivers microcode
> and do the cut off there with a comment as to why you do it like that.void

You mean return !intel so this function will never be called.

__init load_ucode_bsp(void)
{
         int vendor = x86_vendor();

         if (vendor == X86_VENDOR_INTEL)
                 load_ucode_intel_bsp();
}

Ah yes. That would work and be less code.

We should do it that way.

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[H. Peter Anvin]

On 04/19/2013 02:44 PM, Bryan O'Donoghue wrote:
> On 19/04/13 22:25, Borislav Petkov wrote:
>> On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
>>> Just filter out P5 and earlier. The code already does that for CPUs
>>> which don't have CPUID.
>>
>> Actually, an alternative - more practical albeit not very accurate
> 
> More practical ? Hmm - the MSRs don't exist for < P5

Yes, and that is definitional.

I like doing this check before boring down in the code too far, however,
I want to make it so that it is structurally clear that this is for
Intel; other CPU vendors might theoretically have other criteria.

Architecturally, I would prefer to do this check early in
load_ucode_intel_{bsp,ap}() but I would be okay with putting it in
load_ucode_bsp()/local_ucode_ap() as well as long as the test is
conditional on the Intel vendor check.

More importantly, though: I really would like to get a fix *today*.

	-hpa

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[H. Peter Anvin]

On 04/19/2013 02:44 PM, Bryan O'Donoghue wrote:
> On 19/04/13 22:25, Borislav Petkov wrote:
>> On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
>>> Just filter out P5 and earlier. The code already does that for CPUs
>>> which don't have CPUID.
>>
>> Actually, an alternative - more practical albeit not very accurate
> 
> More practical ? Hmm - the MSRs don't exist for < P5
> 
>> solution would be to check for which families Intel delivers microcode
>> and do the cut off there with a comment as to why you do it like
>> that.void
> 
> You mean return !intel so this function will never be called.
> 
> __init load_ucode_bsp(void)
> {
>         int vendor = x86_vendor();
> 
>         if (vendor == X86_VENDOR_INTEL)
>                 load_ucode_intel_bsp();
> }
> 
> Ah yes. That would work and be less code.
> 
> We should do it that way.

Do you think you can have a patch for me in the next few hours?

	-hpa

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[Bryan O'Donoghue]

On 20/04/13 00:13, H. Peter Anvin wrote:
> On 04/19/2013 02:44 PM, Bryan O'Donoghue wrote:
>> On 19/04/13 22:25, Borislav Petkov wrote:
>>> On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
>>>> Just filter out P5 and earlier. The code already does that for CPUs
>>>> which don't have CPUID.
>>>
>>> Actually, an alternative - more practical albeit not very accurate
>>
>> More practical ? Hmm - the MSRs don't exist for<  P5
>>
>>> solution would be to check for which families Intel delivers microcode
>>> and do the cut off there with a comment as to why you do it like
>>> that.void
>>
>> You mean return !intel so this function will never be called.
>>
>> __init load_ucode_bsp(void)
>> {
>>          int vendor = x86_vendor();
>>
>>          if (vendor == X86_VENDOR_INTEL)
>>                  load_ucode_intel_bsp();
>> }
>>
>> Ah yes. That would work and be less code.
>>
>> We should do it that way.
>
> Do you think you can have a patch for me in the next few hours?

Sure.

I'm in Ireland (sleeping @ 00:15 on a Friday night sadly).

Give me ~ 40 minutes

Re: [PATCH] x86: Add check for P5 to microcode_intel_early

[H. Peter Anvin]

I committed my own patch yesterday; at this point it would be most helpful if you could test it out to make sure it works as it should...

Bryan O'Donoghue <bryan.odonoghue.lkml@nexus-software.ie> wrote:

>On 20/04/13 00:13, H. Peter Anvin wrote:
>> On 04/19/2013 02:44 PM, Bryan O'Donoghue wrote:
>>> On 19/04/13 22:25, Borislav Petkov wrote:
>>>> On Fri, Apr 19, 2013 at 10:55:15PM +0200, Borislav Petkov wrote:
>>>>> Just filter out P5 and earlier. The code already does that for
>CPUs
>>>>> which don't have CPUID.
>>>>
>>>> Actually, an alternative - more practical albeit not very accurate
>>>
>>> More practical ? Hmm - the MSRs don't exist for<  P5
>>>
>>>> solution would be to check for which families Intel delivers
>microcode
>>>> and do the cut off there with a comment as to why you do it like
>>>> that.void
>>>
>>> You mean return !intel so this function will never be called.
>>>
>>> __init load_ucode_bsp(void)
>>> {
>>>          int vendor = x86_vendor();
>>>
>>>          if (vendor == X86_VENDOR_INTEL)
>>>                  load_ucode_intel_bsp();
>>> }
>>>
>>> Ah yes. That would work and be less code.
>>>
>>> We should do it that way.
>>
>> Do you think you can have a patch for me in the next few hours?
>
>Sure.
>
>I'm in Ireland (sleeping @ 00:15 on a Friday night sadly).
>
>Give me ~ 40 minutes

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

[tip:x86/urgent] x86, microcode: Verify the family before dispatching microcode patching

[tip-bot for H. Peter Anvin]

Commit-ID:  74c3e3fcf350b2e7e3eaf9550528ee3f74e44b37
Gitweb:     http://git.kernel.org/tip/74c3e3fcf350b2e7e3eaf9550528ee3f74e44b37
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Fri, 19 Apr 2013 16:36:03 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Fri, 19 Apr 2013 16:36:03 -0700

x86, microcode: Verify the family before dispatching microcode patching

For each CPU vendor that implements CPU microcode patching, there will
be a minimum family for which this is implemented.  Verify this
minimum level of support.

This can be done in the dispatch function or early in the application
functions.  Doing the latter turned out to be somewhat awkward because
of the ineviable split between the BSP and the AP paths, and rather
than pushing deep into the application functions, do this in
the dispatch function.

Reported-by: "Bryan O'Donoghue" <bryan.odonoghue.lkml@nexus-software.ie>
Suggested-by: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Link: http://lkml.kernel.org/r/1366392183-4149-1-git-send-email-bryan.odonoghue.lkml@nexus-software.ie
---
 arch/x86/kernel/microcode_core_early.c | 38 +++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/microcode_core_early.c b/arch/x86/kernel/microcode_core_early.c
index 577db84..833d51d 100644
--- a/arch/x86/kernel/microcode_core_early.c
+++ b/arch/x86/kernel/microcode_core_early.c
@@ -45,9 +45,6 @@ static int __cpuinit x86_vendor(void)
 	u32 eax = 0x00000000;
 	u32 ebx, ecx = 0, edx;
 
-	if (!have_cpuid_p())
-		return X86_VENDOR_UNKNOWN;
-
 	native_cpuid(&eax, &ebx, &ecx, &edx);
 
 	if (CPUID_IS(CPUID_INTEL1, CPUID_INTEL2, CPUID_INTEL3, ebx, ecx, edx))
@@ -59,18 +56,45 @@ static int __cpuinit x86_vendor(void)
 	return X86_VENDOR_UNKNOWN;
 }
 
+static int __cpuinit x86_family(void)
+{
+	u32 eax = 0x00000001;
+	u32 ebx, ecx = 0, edx;
+	int x86;
+
+	native_cpuid(&eax, &ebx, &ecx, &edx);
+
+	x86 = (eax >> 8) & 0xf;
+	if (x86 == 15)
+		x86 += (eax >> 20) & 0xff;
+
+	return x86;
+}
+
 void __init load_ucode_bsp(void)
 {
-	int vendor = x86_vendor();
+	int vendor, x86;
+
+	if (!have_cpuid_p())
+		return;
 
-	if (vendor == X86_VENDOR_INTEL)
+	vendor = x86_vendor();
+	x86 = x86_family();
+
+	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
 		load_ucode_intel_bsp();
 }
 
 void __cpuinit load_ucode_ap(void)
 {
-	int vendor = x86_vendor();
+	int vendor, x86;
+
+	if (!have_cpuid_p())
+		return;
+
+	vendor = x86_vendor();
+	x86 = x86_family();
 
-	if (vendor == X86_VENDOR_INTEL)
+	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
 		load_ucode_intel_ap();
 }

Re: [tip:x86/urgent] x86, microcode: Verify the family before dispatching microcode patching

[Bryan O'Donoghue]

Works for me