Discussion:
[PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data
Henrique de Moraes Holschuh
2014-09-08 17:37:53 UTC
Permalink
The full requirements for the memory area which holds the microcode
update binary data can be found in the Intel SDM, vol 3A, section
9.11.6, page 9-34. They basically boil down to: 16-byte alignment, and
the data area must be entirely mapped if paging is already enabled.

The regular microcode update driver doesn't have to do anything special
to meet these requirements. For peace of mind, add a check to
WARN_ONCE() when the alignment is (unexpectedly) incorrect, and abort
the microcode update.

However, the early microcode update driver can only expect 4-byte
alignment out of the early initramfs file format (which is actually good
enough for many Intel processors, but unless Intel oficially documents
this, we cannot make use of that fact). The microcode update data will
not be aligned to a 16-byte boundary unless userspace takes special
steps to ensure it.

Change the early microcode driver to make a temporary copy of a portion
of the microcode header, and move the microcode data backwards
(overwriting the header) to a suitably aligned position, right before
issuing the microcode update WRMSR.

Unfortunately, to keep things simple, this has to be undone right after
the processor finishes the WRMSR. Therefore, the alignment process will
have to be redone *for every processor core*. This might end up not
being really noticeable, as the microcode update operation itself is
already extremely expensive in processor cycles.

If the microcode update data is already aligned in the first place, the
alignment process is skipped. Users of large systems are encouraged to
use updated userspace that ensures 16-byte alignment of the microcode
data file contents inside the early initramfs image.

Add the relevant details to Documentation/x86/early-microcode.txt.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
Documentation/x86/early-microcode.txt | 10 +++++++++
arch/x86/kernel/cpu/microcode/intel.c | 5 +++++
arch/x86/kernel/cpu/microcode/intel_early.c | 30 +++++++++++++++++++++++++--
3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/x86/early-microcode.txt b/Documentation/x86/early-microcode.txt
index d62bea6..c4f2ebd 100644
--- a/Documentation/x86/early-microcode.txt
+++ b/Documentation/x86/early-microcode.txt
@@ -14,6 +14,16 @@ during boot time. The microcode file in cpio name space is:
on Intel: kernel/x86/microcode/GenuineIntel.bin
on AMD : kernel/x86/microcode/AuthenticAMD.bin

+For Intel processors, the microcode load process will be faster when special
+care is taken to ensure that the kernel/x86/microcode/GenuineIntel.bin file
+*data* inside the cpio archive is aligned to a paragraph (16-byte boundary).
+Standard pax/cpio can be coaxed into doing this by adding a padding file, e.g.
+"kernel/x86/microcode/.padding" with the appropriate size *right before* the
+kernel/x86/microcode/GenuineIntel.bin file. Beware the required size for the
+padding file as it depends on the behavior of the tool used to create the cpio
+archive. It is also possible to use a specific tool that appends enough NULs
+_to_ the file name (not _after_ the file name!) to align the file data.
+
During BSP boot (before SMP starts), if the kernel finds the microcode file in
the initrd file, it parses the microcode and saves matching microcode in memory.
If matching microcode is found, it will be uploaded in BSP and later on in all
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 2182cec..40caef1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -157,6 +157,11 @@ static int apply_microcode_intel(int cpu)
if (mc_intel == NULL)
return 0;

+ /* Intel SDM vol 3A section 9.11.6, page 9-34 */
+ if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
+ "microcode data incorrectly aligned"))
+ return -1;
+
/*
* Microcode on this CPU might be already up-to-date. Only apply
* the microcode patch in mc_intel when it is newer than the one
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 92629a8..994c59b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -662,14 +662,40 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
struct microcode_intel *mc_intel;
unsigned int val[2];

+ char savedbuf[16];
+ void *mcu_data;
+ void *aligned_mcu_data;
+ unsigned int mcu_size = 0;
+
mc_intel = uci->mc;
if (mc_intel == NULL)
return 0;

+ mcu_data = mc_intel->bits;
+ aligned_mcu_data = mc_intel->bits;
+
+ /* Intel SDM vol 3A section 9.11.6, page 9-34: */
+ /* WRMSR MSR_IA32_UCODE_WRITE requires 16-byte alignment */
+ if ((unsigned long)(mcu_data) % 16) {
+ /* We have more than 16 bytes worth of microcode header
+ * just before mc_intel->bits on a version 1 header */
+ BUILD_BUG_ON(offsetof(struct microcode_intel, bits) < 16);
+
+ aligned_mcu_data = (void *)((unsigned long)(mcu_data) & ~15UL);
+ mcu_size = get_datasize(&mc_intel->hdr);
+ memcpy(savedbuf, aligned_mcu_data, sizeof(savedbuf));
+ memmove(aligned_mcu_data, mcu_data, mcu_size);
+ }
+
/* write microcode via MSR 0x79 */
native_wrmsr(MSR_IA32_UCODE_WRITE,
- (unsigned long) mc_intel->bits,
- (unsigned long) mc_intel->bits >> 16 >> 16);
+ lower_32_bits((unsigned long)aligned_mcu_data),
+ upper_32_bits((unsigned long)aligned_mcu_data));
+
+ if (mcu_size) {
+ memmove(mcu_data, aligned_mcu_data, mcu_size);
+ memcpy(aligned_mcu_data, savedbuf, sizeof(savedbuf));
+ }

/* get the current revision from MSR 0x8B */
native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
--
1.7.10.4
Henrique de Moraes Holschuh
2014-09-08 17:37:52 UTC
Permalink
The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).

The microcode driver was abusing sync_core() to do this, probably
because it predates by nearly a decade the current "asm volatile
(:::"memory")" implementation of native_cpuid(), which is required for
the Intel MSR 8BH access protocol.

sync_core() semanthics are that of being a speculative execution
barrier, and not "run cpuid with EAX=1".

Change the Intel microcode driver to use native_cpuid instead, which is
more appropriate. native_cpuid() is already in use by the early
microcode driver in one place.

Some small code reordering was done to avoid calling cpuid twice in
collect_cpu_info_early().

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/include/asm/microcode_intel.h | 12 ++++++++++++
arch/x86/kernel/cpu/microcode/intel.c | 6 ++----
arch/x86/kernel/cpu/microcode/intel_early.c | 25 +++++++------------------
3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index bbe296e..d40a45e 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -84,4 +84,16 @@ static inline int save_mc_for_early(u8 *mc)
}
#endif

+static inline unsigned int intel_ucode_cpuid_1(void)
+{
+ unsigned int eax, ebx, ecx, edx;
+
+ eax = 1;
+ ecx = 0;
+ /* extremely important: must be asm volatile(:::"memory") */
+ native_cpuid(&eax, &ebx, &ecx, &edx);
+
+ return eax;
+}
+
#endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index e99cdd8..2182cec 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -94,8 +94,6 @@ static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)

memset(csig, 0, sizeof(*csig));

- csig->sig = cpuid_eax(0x00000001);
-
if ((c->x86_model >= 5) || (c->x86 > 6)) {
/* get processor flags from MSR 0x17 */
rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
@@ -104,7 +102,7 @@ static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)

/* get the current microcode revision from MSR 0x8B */
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
- sync_core();
+ csig->sig = intel_ucode_cpuid_1(); /* a cpuid(1) must happen here */
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

csig->rev = val[1];
@@ -174,7 +172,7 @@ static int apply_microcode_intel(int cpu)

/* get the current revision from MSR 0x8B */
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
- sync_core();
+ intel_ucode_cpuid_1();
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

if (val[1] != mc_intel->hdr.rev) {
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 8ad50d6..92629a8 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -379,7 +379,6 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
unsigned int val[2];
u8 x86, x86_model;
struct cpu_signature csig;
- unsigned int eax, ebx, ecx, edx;

csig.sig = 0;
csig.pf = 0;
@@ -387,10 +386,11 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)

memset(uci, 0, sizeof(*uci));

- eax = 0x00000001;
- ecx = 0;
- native_cpuid(&eax, &ebx, &ecx, &edx);
- csig.sig = eax;
+ /* get the current revision from MSR 0x8B */
+ native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ csig.sig = intel_ucode_cpuid_1(); /* a cpuid(1) must happen here */
+ native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+ csig.rev = val[1];

x86 = get_x86_family(csig.sig);
x86_model = get_x86_model(csig.sig);
@@ -400,15 +400,6 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
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]);
-
- csig.rev = val[1];

uci->cpu_sig = csig;
uci->valid = 1;
@@ -679,12 +670,10 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
native_wrmsr(MSR_IA32_UCODE_WRITE,
(unsigned long) mc_intel->bits,
(unsigned long) mc_intel->bits >> 16 >> 16);
- 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_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ intel_ucode_cpuid_1();
native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
if (val[1] != mc_intel->hdr.rev) {
print_ucode(INTEL_EARLYMCU_REJECTED, uci, mc_intel->hdr.rev);
--
1.7.10.4
Henrique de Moraes Holschuh
2014-09-08 17:37:50 UTC
Permalink
Enhance the logging in the Intel early microcode update driver to
be able to report errors.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 94 +++++++++++++++------------
1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index f73fc0a..8ad50d6 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -31,6 +31,12 @@
#include <asm/tlbflush.h>
#include <asm/setup.h>

+enum {
+ INTEL_EARLYMCU_NONE = 0, /* did nothing */
+ INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
+ INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
+};
+
static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
static struct mc_saved_data {
unsigned int mc_saved_count;
@@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,

/*
* Print ucode update info.
+ * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
+ * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
*/
-static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+static void print_ucode_info(const unsigned int status,
+ const struct ucode_cpu_info *uci,
+ const unsigned int data)
{
int cpu = smp_processor_id();
-
- pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
- cpu,
- uci->cpu_sig.rev,
- date & 0xffff,
- date >> 24,
- (date >> 16) & 0xff);
+ struct ucode_cpu_info ucil;
+
+ switch (status) {
+ case INTEL_EARLYMCU_NONE:
+ break;
+ case INTEL_EARLYMCU_UPDATEOK:
+ if (!uci) {
+ collect_cpu_info_early(&ucil);
+ uci = &ucil;
+ }
+ pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
+ cpu,
+ uci->cpu_sig.rev,
+ data & 0xffff,
+ data >> 24,
+ (data >> 16) & 0xff);
+ break;
+ case INTEL_EARLYMCU_REJECTED:
+ pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
+ break;
+ }
}

#ifdef CONFIG_X86_32

-static int delay_ucode_info;
-static int current_mc_date;
+static unsigned int delay_ucode_info;
+static unsigned int delay_ucode_info_data;

/*
* Print early updated ucode info after printk works. This is delayed info dump.
+ * This is only used for the BSP.
*/
void show_ucode_info_early(void)
{
- struct ucode_cpu_info uci;
-
- if (delay_ucode_info) {
- collect_cpu_info_early(&uci);
- print_ucode_info(&uci, current_mc_date);
- delay_ucode_info = 0;
- }
+ print_ucode_info(delay_ucode_info, NULL, delay_ucode_info_data);
+ delay_ucode_info = INTEL_EARLYMCU_NONE;
}

/*
@@ -614,21 +633,18 @@ void show_ucode_info_early(void)
* mc_saved_data.mc_saved and delay printing microcode info in
* show_ucode_info_early() until printk() works.
*/
-static void print_ucode(struct ucode_cpu_info *uci)
+static void print_ucode(const unsigned int status,
+ const struct ucode_cpu_info * const uci,
+ const unsigned int data)
{
- struct microcode_intel *mc_intel;
- int *delay_ucode_info_p;
- int *current_mc_date_p;
-
- mc_intel = uci->mc;
- if (mc_intel == NULL)
- return;
+ unsigned int *delay_ucode_info_p;
+ unsigned int *delay_ucode_info_data_p;

- delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
- current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
+ delay_ucode_info_p = (unsigned int *)__pa_nodebug(&delay_ucode_info);
+ delay_ucode_info_data_p = (unsigned int *)__pa_nodebug(&delay_ucode_info_data);

- *delay_ucode_info_p = 1;
- *current_mc_date_p = mc_intel->hdr.date;
+ *delay_ucode_info_p = status;
+ *delay_ucode_info_data_p = data;
}
#else

@@ -641,15 +657,11 @@ static inline void flush_tlb_early(void)
__native_flush_tlb_global_irq_disabled();
}

-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(const unsigned int status,
+ const struct ucode_cpu_info * const uci,
+ const unsigned int data)
{
- struct microcode_intel *mc_intel;
-
- mc_intel = uci->mc;
- if (mc_intel == NULL)
- return;
-
- print_ucode_info(uci, mc_intel->hdr.date);
+ print_ucode_info(status, uci, data);
}
#endif

@@ -674,8 +686,10 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,

/* get the current revision from MSR 0x8B */
native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
- if (val[1] != mc_intel->hdr.rev)
+ if (val[1] != mc_intel->hdr.rev) {
+ print_ucode(INTEL_EARLYMCU_REJECTED, uci, mc_intel->hdr.rev);
return -1;
+ }

#ifdef CONFIG_X86_64
/* Flush global tlb. This is precaution. */
@@ -683,7 +697,7 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
#endif
uci->cpu_sig.rev = val[1];

- print_ucode(uci);
+ print_ucode(INTEL_EARLYMCU_UPDATEOK, uci, mc_intel->hdr.date);

return 0;
}
--
1.7.10.4
Borislav Petkov
2014-10-20 15:08:01 UTC
Permalink
Post by Henrique de Moraes Holschuh
Enhance the logging in the Intel early microcode update driver to
be able to report errors.
---
arch/x86/kernel/cpu/microcode/intel_early.c | 94 +++++++++++++++------------
1 file changed, 54 insertions(+), 40 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index f73fc0a..8ad50d6 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -31,6 +31,12 @@
#include <asm/tlbflush.h>
#include <asm/setup.h>
+enum {
+ INTEL_EARLYMCU_NONE = 0, /* did nothing */
+ INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
+ INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
+};
+
static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
static struct mc_saved_data {
unsigned int mc_saved_count;
@@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
/*
* Print ucode update info.
+ * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
+ * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
*/
-static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+static void print_ucode_info(const unsigned int status,
+ const struct ucode_cpu_info *uci,
+ const unsigned int data)
{
int cpu = smp_processor_id();
-
- pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
- cpu,
- uci->cpu_sig.rev,
- date & 0xffff,
- date >> 24,
- (date >> 16) & 0xff);
+ struct ucode_cpu_info ucil;
+
+ switch (status) {
+ break;
+ if (!uci) {
+ collect_cpu_info_early(&ucil);
+ uci = &ucil;
+ }
+ pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
+ cpu,
+ uci->cpu_sig.rev,
+ data & 0xffff,
+ data >> 24,
+ (data >> 16) & 0xff);
+ break;
+ pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
+ break;
+ }
}
#ifdef CONFIG_X86_32
-static int delay_ucode_info;
-static int current_mc_date;
+static unsigned int delay_ucode_info;
+static unsigned int delay_ucode_info_data;
First of all, this really is date and not data and prefixing it with
"delay" really doesn't make it cleaner.

Then, this whole scheme can be simplified a bit by dropping
delay_ucode_info and using current_mc_date to test whether to print the
message or not. After printing, you set it back to 0.

And then you can drop the _REJECTED case as it is not needed.
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-09-08 17:37:54 UTC
Permalink
Microcode updates that requires an unknown loader should never reach the
apply_* functions (the code should have rejected it earlier). Likewise
for an unknown microcode header layout.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel.c | 2 ++
arch/x86/kernel/cpu/microcode/intel_early.c | 2 ++
2 files changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 40caef1..439681f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -157,6 +157,8 @@ static int apply_microcode_intel(int cpu)
if (mc_intel == NULL)
return 0;

+ BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
+
/* Intel SDM vol 3A section 9.11.6, page 9-34 */
if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
"microcode data incorrectly aligned"))
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 994c59b..095db11 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -671,6 +671,8 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
if (mc_intel == NULL)
return 0;

+ BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
+
mcu_data = mc_intel->bits;
aligned_mcu_data = mc_intel->bits;
--
1.7.10.4
Henrique de Moraes Holschuh
2014-09-08 17:37:47 UTC
Permalink
The Intel SDM vol 3A, section 9.11.1, and also table 9-6, requires that
the Data Size field be a multiple of 4 bytes. All of the microcode
update header structures are dword-based, so the Total Size field must
also be a multiple of the dword size.

Ensure that data_size is a multiple of the dword size (4 bytes). The
driver code assumes this to be true for both data_size and total_size,
and will not work correctly otherwise.

Futhermore, require that total_size be a multiple of 1024, as per the
Intel SDM, vol 3A, section 9.11.1, page 9-28; table 9-6, page 9-29, and
others. Test added by request of Borislav Petkov.

Also refuse a microcode update with a microcode revision of zero.
According to the Intel SDM, vol 3A, section 9.11.7, page 9-36, a
microcode revision of zero is special:

"CPUID returns a value in a model specific register in addition to
its usual register return values. The semantics of CPUID cause it
to deposit an update ID value in the 64-bit model-specific register
at address 08BH (IA32_BIOS_SIGN_ID). If no update is present in the
processor, the value in the MSR remains unmodified. The BIOS must
pre-load a zero into the MSR before executing CPUID. If a read of
the MSR at 8BH still returns zero after executing CPUID, this
indicates that no update is present."

This effectively reserves revision zero to mean "no microcode update
installed on the processor": the microcode loader cannot differentiate
sucess from failure when updating microcode to the same revision as the
one currently installed on the processor, and this would always happen
to updates to revision zero in the BIOS/UEFI loader.

There is every reason to be paranoid about any microcode update with a
revision of zero, as Intel will never release such a microcode update.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel_lib.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320..25915e3 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -55,9 +55,10 @@ int microcode_sanity_check(void *mc, int print_err)
total_size = get_totalsize(mc_header);
data_size = get_datasize(mc_header);

- if (data_size + MC_HEADER_SIZE > total_size) {
+ if ((data_size % DWSIZE) || (total_size % 1024) ||
+ (data_size + MC_HEADER_SIZE > total_size)) {
if (print_err)
- pr_err("error! Bad data size in microcode data file\n");
+ pr_err("error: bad data size or total size in microcode data file\n");
return -EINVAL;
}

@@ -83,6 +84,26 @@ int microcode_sanity_check(void *mc, int print_err)
ext_sigcount = ext_header->count;
}

+ /*
+ * A version 1 loader cannot differentiate failure from success when
+ * attempting a microcode update to the same revision as the one
+ * currently installed. The loader is supposed to never attempt a
+ * same-version update (or a microcode downgrade, for that matter).
+ *
+ * This will always cause issues for microcode updates to revision zero
+ * in the UEFI/BIOS microcode loader: the processor reports a revision
+ * of zero when it is running without any microcode updates installed,
+ * such as after a reset/power up.
+ *
+ * Intel will never issue a microcode update with a revision of zero
+ * for the version 1 loader. Reject it.
+ */
+ if (mc_header->rev == 0) { /* reserved for "no-update-installed" */
+ if (print_err)
+ pr_err("error: restricted revision 0 in microcode data file\n");
+ return -EINVAL;
+ }
+
/* check extended table checksum */
if (ext_table_size) {
int ext_table_sum = 0;
--
1.7.10.4
Borislav Petkov
2014-10-05 17:34:54 UTC
Permalink
Post by Henrique de Moraes Holschuh
The Intel SDM vol 3A, section 9.11.1, and also table 9-6, requires that
the Data Size field be a multiple of 4 bytes. All of the microcode
update header structures are dword-based, so the Total Size field must
also be a multiple of the dword size.
Ensure that data_size is a multiple of the dword size (4 bytes). The
driver code assumes this to be true for both data_size and total_size,
and will not work correctly otherwise.
Futhermore, require that total_size be a multiple of 1024, as per the
Intel SDM, vol 3A, section 9.11.1, page 9-28; table 9-6, page 9-29, and
others. Test added by request of Borislav Petkov.
Also refuse a microcode update with a microcode revision of zero.
According to the Intel SDM, vol 3A, section 9.11.7, page 9-36, a
"CPUID returns a value in a model specific register in addition to
its usual register return values. The semantics of CPUID cause it
to deposit an update ID value in the 64-bit model-specific register
at address 08BH (IA32_BIOS_SIGN_ID). If no update is present in the
processor, the value in the MSR remains unmodified. The BIOS must
pre-load a zero into the MSR before executing CPUID. If a read of
the MSR at 8BH still returns zero after executing CPUID, this
indicates that no update is present."
This effectively reserves revision zero to mean "no microcode update
installed on the processor": the microcode loader cannot differentiate
sucess from failure when updating microcode to the same revision as the
one currently installed on the processor, and this would always happen
to updates to revision zero in the BIOS/UEFI loader.
There is every reason to be paranoid about any microcode update with a
revision of zero, as Intel will never release such a microcode update.
---
arch/x86/kernel/cpu/microcode/intel_lib.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320..25915e3 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -55,9 +55,10 @@ int microcode_sanity_check(void *mc, int print_err)
total_size = get_totalsize(mc_header);
data_size = get_datasize(mc_header);
- if (data_size + MC_HEADER_SIZE > total_size) {
+ if ((data_size % DWSIZE) || (total_size % 1024) ||
+ (data_size + MC_HEADER_SIZE > total_size)) {
if (print_err)
- pr_err("error! Bad data size in microcode data file\n");
+ pr_err("error: bad data size or total size in microcode data file\n");
Shorten:

pr_err("error: bad data/total size in microcode data file\n");
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
@@ -83,6 +84,26 @@ int microcode_sanity_check(void *mc, int print_err)
ext_sigcount = ext_header->count;
}
+ /*
+ * A version 1 loader cannot differentiate failure from success when
+ * attempting a microcode update to the same revision as the one
+ * currently installed. The loader is supposed to never attempt a
+ * same-version update (or a microcode downgrade, for that matter).
+ *
+ * This will always cause issues for microcode updates to revision zero
+ * in the UEFI/BIOS microcode loader: the processor reports a revision
+ * of zero when it is running without any microcode updates installed,
+ * such as after a reset/power up.
+ *
+ * Intel will never issue a microcode update with a revision of zero
+ * for the version 1 loader. Reject it.
+ */
This comment is too long. How about this instead:

/*
* 0 is not a valid microcode revision as it is used to denote the
* failure of a microcode update, see MSR 0x8b (IA32_BIOS_SIGN_ID):
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/

This is one of those seldom times where the documentation is actually clear. :-)
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-10-05 19:37:03 UTC
Permalink
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
- if (data_size + MC_HEADER_SIZE > total_size) {
+ if ((data_size % DWSIZE) || (total_size % 1024) ||
+ (data_size + MC_HEADER_SIZE > total_size)) {
if (print_err)
- pr_err("error! Bad data size in microcode data file\n");
+ pr_err("error: bad data size or total size in microcode data file\n");
pr_err("error: bad data/total size in microcode data file\n");
will do.
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
+ /*
+ * A version 1 loader cannot differentiate failure from success when
+ * attempting a microcode update to the same revision as the one
+ * currently installed. The loader is supposed to never attempt a
+ * same-version update (or a microcode downgrade, for that matter).
+ *
+ * This will always cause issues for microcode updates to revision zero
+ * in the UEFI/BIOS microcode loader: the processor reports a revision
+ * of zero when it is running without any microcode updates installed,
+ * such as after a reset/power up.
+ *
+ * Intel will never issue a microcode update with a revision of zero
+ * for the version 1 loader. Reject it.
+ */
/*
* 0 is not a valid microcode revision as it is used to denote the
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/
This is one of those seldom times where the documentation is actually clear. :-)
Not realy, because it got you confused! :-)

Zero does not denote a failure to update microcode. What zero means, *when
you did the pre-load and issued a cpuid(1)*, is that the processor microcode
has not been updated since power-on/reset.

What flags a *sucessful* microcode update is a change on IA32_BIOS_SIGN_ID
(which must be read with the zero preload and cpuid(1) protocol).

If IA32_BIOS_SIGN_ID didn't change, the microcode update was rejected...
obviously, this only holds when you never attempt to update the microcode to
the same version the processor already had running.

And that's why we cannot detect whether a same-version update worked or not.

The reason Intel will never issue a microcode with revision zero is because
it cannot be safely applied by UEFI or BIOS at system power up: it would
look like a same-version update (IA32_BIOS_SIGN_ID would return zero before
the update, and would return zero after the update, whether it was applied
sucessfully or not).

And since Intel will never issue such microcode, we don't want to deal with
anything that claims to be a microcode update to revision zero.


IOW, this is a failure:

IA32_BIOS_SIGN_ID before the update is the same as IA32_BIOS_SIGN_ID after
the update attempt.

this is a sucessful update:

IA32_BIOS_SIGN_ID before the update is different from IA32_BIOS_SIGN_ID
after the update attempt.

In any case, you always need to do the zero-preload and cpuid(1) to read
IA32_BIOS_SIGN_ID.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Borislav Petkov
2014-10-05 21:13:04 UTC
Permalink
Post by Henrique de Moraes Holschuh
Not realy, because it got you confused! :-)
No, it didn't get me confused - it got you confused that I'm confused.

You need to read the comment as a *whole*. The zero value is special
because it is *used* to *denote* a failure. You can use any other
invalid revision value for that matter.

Maybe "denote" was not precise enough - maybe it should say "0 is an
invalid microcode revision and is used to detect the failure of a
microcode update" or similar. Yeah, "detect" sounds better.
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-10-05 21:49:08 UTC
Permalink
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
Not realy, because it got you confused! :-)
No, it didn't get me confused - it got you confused that I'm confused.
Indeed.
Post by Borislav Petkov
You need to read the comment as a *whole*. The zero value is special
because it is *used* to *denote* a failure. You can use any other
invalid revision value for that matter.
Well, the new wording is confusing me... so maybe we can try a third time?
:-)
Post by Borislav Petkov
Maybe "denote" was not precise enough - maybe it should say "0 is an
invalid microcode revision and is used to detect the failure of a
microcode update" or similar. Yeah, "detect" sounds better.
How about this:

/*
* 0 is not a valid microcode revision as it is used to detect the
* absence of any sucessful microcode update since reset /
* power-on, see MSR 0x8b (IA32_BIOS_SIGN_ID):
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/

?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Borislav Petkov
2014-10-06 05:15:36 UTC
Permalink
Post by Henrique de Moraes Holschuh
/*
* 0 is not a valid microcode revision as it is used to detect the
* absence of any sucessful microcode update since reset /
*
* "It is required that this register field be pre-loaded with zero
* prior to executing the CPUID, function 1. If the field remains
* equal to zero, then there is no microcode update loaded. Another
* non-zero value will be the signature."
*/
Yep.
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-09-08 17:37:49 UTC
Permalink
The Intel microcode update driver will skip the second hardware thread
on hyper-threaded cores during an update run, as the first hardware
thread will have updated the entire core. This can confuse users,
because it will look like some CPUs were not updated in the system log.
Attempt to clarify the log messages to hint that we might be updating
more than one CPU (hardware thread) at a time.

Make sure all driver log messages conform to this template: "microcode:
CPU#: <message>". The <message> might refer to the core, and not to the
hardware thread/CPU.

Reword error and debug messages for clarity or style. Tag most error
messages as "error:", and warnings as "warning:". Report conditions
which will stop a microcode update as errors, and conditions which will
not stop a microcode update as warnings.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel.c | 10 +++++-----
arch/x86/kernel/cpu/microcode/intel_early.c | 11 +++++++----
arch/x86/kernel/cpu/microcode/intel_lib.c | 12 ++++++------
3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 2c629d1..e99cdd8 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -115,7 +115,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
__collect_cpu_info(cpu_num, csig);

- pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
+ pr_info("CPU%d: sig=0x%x, pf=0x%x, revision=0x%x\n",
cpu_num, csig->sig, csig->pf, csig->rev);

return 0;
@@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

if (val[1] != mc_intel->hdr.rev) {
- pr_err("CPU%d update to revision 0x%x failed\n",
+ pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
cpu_num, mc_intel->hdr.rev);
return -1;
}
- pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
+ pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
cpu_num, val[1],
mc_intel->hdr.date & 0xffff,
mc_intel->hdr.date >> 24,
@@ -214,7 +214,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,

mc_size = get_totalsize(&mc_header);
if (!mc_size || mc_size > leftover) {
- pr_err("error! Bad data in microcode data file\n");
+ pr_err("error: invalid microcode update data\n");
break;
}

@@ -268,7 +268,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
*/
save_mc_for_early(new_mc);

- pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
+ pr_debug("CPU%d: found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
out:
return state;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f..f73fc0a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -16,6 +16,9 @@
* as published by the Free Software Foundation; either version
* 2 of the License, or (at your option) any later version.
*/
+
+#define pr_fmt(fmt) "microcode: " fmt
+
#include <linux/module.h>
#include <linux/mm.h>
#include <linux/slab.h>
@@ -418,7 +421,7 @@ static void __ref show_saved_mc(void)
pr_debug("no microcode data saved.\n");
return;
}
- pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
+ pr_debug("total microcode entries saved: %d\n", mc_saved_data.mc_saved_count);

collect_cpu_info_early(&uci);

@@ -519,7 +522,7 @@ int save_mc_for_early(u8 *mc)
*/
ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
if (ret) {
- pr_err("Cannot save microcode patch.\n");
+ pr_warn("warning: could not store microcode update data for later use.\n");
goto out;
}

@@ -579,7 +582,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
{
int cpu = smp_processor_id();

- pr_info("CPU%d microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
+ pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
cpu,
uci->cpu_sig.rev,
date & 0xffff,
@@ -701,7 +704,7 @@ int __init save_microcode_in_initrd_intel(void)
microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
ret = save_microcode(&mc_saved_data, mc_saved, count);
if (ret)
- pr_err("Cannot save microcode patches from initrd.\n");
+ pr_warn("warning: failed to save early microcode update data for future use\n");

show_saved_mc();

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 25915e3..1cc6494 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -64,7 +64,7 @@ int microcode_sanity_check(void *mc, int print_err)

if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
if (print_err)
- pr_err("error! Unknown microcode update format\n");
+ pr_err("error: unknown microcode update format\n");
return -EINVAL;
}
ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
@@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
if ((ext_table_size < EXT_HEADER_SIZE)
|| ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
if (print_err)
- pr_err("error! Small exttable size in microcode data file\n");
+ pr_err("error: small exttable size in microcode data file\n");
return -EINVAL;
}
ext_header = mc + MC_HEADER_SIZE + data_size;
if (ext_table_size != exttable_size(ext_header)) {
if (print_err)
- pr_err("error! Bad exttable size in microcode data file\n");
+ pr_err("error: bad exttable size in microcode data file\n");
return -EFAULT;
}
ext_sigcount = ext_header->count;
@@ -114,7 +114,7 @@ int microcode_sanity_check(void *mc, int print_err)
ext_table_sum += ext_tablep[i];
if (ext_table_sum) {
if (print_err)
- pr_warn("aborting, bad extended signature table checksum\n");
+ pr_err("error: bad extended signature table checksum\n");
return -EINVAL;
}
}
@@ -126,7 +126,7 @@ int microcode_sanity_check(void *mc, int print_err)
orig_sum += ((int *)mc)[i];
if (orig_sum) {
if (print_err)
- pr_err("aborting, bad checksum\n");
+ pr_err("error: bad microcode update checksum\n");
return -EINVAL;
}
if (!ext_table_size)
@@ -140,7 +140,7 @@ int microcode_sanity_check(void *mc, int print_err)
+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
if (sum) {
if (print_err)
- pr_err("aborting, bad checksum\n");
+ pr_err("error: bad extended signature checksum\n");
return -EINVAL;
}
}
--
1.7.10.4
Borislav Petkov
2014-10-20 13:52:50 UTC
Permalink
Post by Henrique de Moraes Holschuh
The Intel microcode update driver will skip the second hardware thread
on hyper-threaded cores during an update run, as the first hardware
thread will have updated the entire core. This can confuse users,
because it will look like some CPUs were not updated in the system log.
Attempt to clarify the log messages to hint that we might be updating
more than one CPU (hardware thread) at a time.
CPU#: <message>". The <message> might refer to the core, and not to the
hardware thread/CPU.
Reword error and debug messages for clarity or style. Tag most error
messages as "error:", and warnings as "warning:". Report conditions
which will stop a microcode update as errors, and conditions which will
not stop a microcode update as warnings.
---
arch/x86/kernel/cpu/microcode/intel.c | 10 +++++-----
arch/x86/kernel/cpu/microcode/intel_early.c | 11 +++++++----
arch/x86/kernel/cpu/microcode/intel_lib.c | 12 ++++++------
3 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 2c629d1..e99cdd8 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -115,7 +115,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
__collect_cpu_info(cpu_num, csig);
- pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
+ pr_info("CPU%d: sig=0x%x, pf=0x%x, revision=0x%x\n",
cpu_num, csig->sig, csig->pf, csig->rev);
return 0;
@@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
if (val[1] != mc_intel->hdr.rev) {
- pr_err("CPU%d update to revision 0x%x failed\n",
+ pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
cpu_num, mc_intel->hdr.rev);
return -1;
}
- pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
+ pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
Those two above are not really needed IMO.
Post by Henrique de Moraes Holschuh
cpu_num, val[1],
mc_intel->hdr.date & 0xffff,
mc_intel->hdr.date >> 24,
@@ -214,7 +214,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
mc_size = get_totalsize(&mc_header);
if (!mc_size || mc_size > leftover) {
- pr_err("error! Bad data in microcode data file\n");
+ pr_err("error: invalid microcode update data\n");
What's wrong with the original message?
Post by Henrique de Moraes Holschuh
break;
}
@@ -268,7 +268,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
*/
save_mc_for_early(new_mc);
- pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
+ pr_debug("CPU%d: found a matching microcode update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
return state;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f..f73fc0a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -16,6 +16,9 @@
* as published by the Free Software Foundation; either version
* 2 of the License, or (at your option) any later version.
*/
+
+#define pr_fmt(fmt) "microcode: " fmt
+
#include <linux/module.h>
#include <linux/mm.h>
#include <linux/slab.h>
@@ -418,7 +421,7 @@ static void __ref show_saved_mc(void)
pr_debug("no microcode data saved.\n");
return;
}
- pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
+ pr_debug("total microcode entries saved: %d\n", mc_saved_data.mc_saved_count);
That should be "Total microcode patches saved" - "entries" doesn't say a whole
lot.
Post by Henrique de Moraes Holschuh
collect_cpu_info_early(&uci);
@@ -519,7 +522,7 @@ int save_mc_for_early(u8 *mc)
*/
ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
if (ret) {
- pr_err("Cannot save microcode patch.\n");
+ pr_warn("warning: could not store microcode update data for later use.\n");
Capitalize: "Warning: could... "

otherwise that message clarification makes sense.
Post by Henrique de Moraes Holschuh
goto out;
}
@@ -579,7 +582,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
{
int cpu = smp_processor_id();
- pr_info("CPU%d microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
+ pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
No, please no "entire core" mentions - that'll only confuse people.
Simply think of logical cores as separate cores which share the
microcode hw. No need for more confusion.
Post by Henrique de Moraes Holschuh
cpu,
uci->cpu_sig.rev,
date & 0xffff,
@@ -701,7 +704,7 @@ int __init save_microcode_in_initrd_intel(void)
microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
ret = save_microcode(&mc_saved_data, mc_saved, count);
if (ret)
- pr_err("Cannot save microcode patches from initrd.\n");
+ pr_warn("warning: failed to save early microcode update data for future use\n");
This one actually loses info - the "initrd" part.
Post by Henrique de Moraes Holschuh
show_saved_mc();
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 25915e3..1cc6494 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -64,7 +64,7 @@ int microcode_sanity_check(void *mc, int print_err)
if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
if (print_err)
- pr_err("error! Unknown microcode update format\n");
+ pr_err("error: unknown microcode update format\n");
Actually it should be like a real sentence:

"Error: unknown ... format.\n"
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
@@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
if ((ext_table_size < EXT_HEADER_SIZE)
|| ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
if (print_err)
- pr_err("error! Small exttable size in microcode data file\n");
+ pr_err("error: small exttable size in microcode data file\n");
That doesn't tell me a whole lot - maybe "... truncated exttable in microcode data file" ?
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
ext_header = mc + MC_HEADER_SIZE + data_size;
if (ext_table_size != exttable_size(ext_header)) {
if (print_err)
- pr_err("error! Bad exttable size in microcode data file\n");
+ pr_err("error: bad exttable size in microcode data file\n");
Ditto.
Post by Henrique de Moraes Holschuh
return -EFAULT;
}
ext_sigcount = ext_header->count;
@@ -114,7 +114,7 @@ int microcode_sanity_check(void *mc, int print_err)
ext_table_sum += ext_tablep[i];
if (ext_table_sum) {
if (print_err)
- pr_warn("aborting, bad extended signature table checksum\n");
+ pr_err("error: bad extended signature table checksum\n");
Capitalize.
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
}
@@ -126,7 +126,7 @@ int microcode_sanity_check(void *mc, int print_err)
orig_sum += ((int *)mc)[i];
if (orig_sum) {
if (print_err)
- pr_err("aborting, bad checksum\n");
+ pr_err("error: bad microcode update checksum\n");
Ditto.
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
if (!ext_table_size)
@@ -140,7 +140,7 @@ int microcode_sanity_check(void *mc, int print_err)
+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
if (sum) {
if (print_err)
- pr_err("aborting, bad checksum\n");
+ pr_err("error: bad extended signature checksum\n");
"Aborting ..." was better.
Post by Henrique de Moraes Holschuh
return -EINVAL;
}
}
--
1.7.10.4
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-09-08 17:37:51 UTC
Permalink
The contents of the extended signature entries are already covered by
the extended table checksum, and the microcode driver should not be
attempting to check their internal checksum field.

Unlike the main microcode checksum field and the extended signature
table checksum field, the checksum fields inside the extended signature
entries are not meant to be processed by a microcode update loader. The
extended signature entry checksum field's description in the Intel SDM,
vol 3A, table 9-6, page 9-30, reads in the first paragraph:

"Used by utility software to decompose a microcode update into
multiple microcode updates where each of the new updates is
constructed without the optional Extended Processor Signature
Table."

And the Linux microcode driver is not processing them correctly anyway.
The second paragraph of the signature entry checksum field's description
in the Intel SDM, vol 3A, table 9-6, page 9-30, reads:

"To calculate the Checksum, substitute the Primary Processor
Signature entry and the Processor Flags entry with the corresponding
Extended Patch entry. Delete the Extended Processor Signature Table
entries. The Checksum is correct when the summation of all DWORDs
that comprise the created Extended Processor Patch results in
00000000H."

Deleting the extended signature table changes the Total Size field, and
the Intel SDM paragraph above makes it very clear that such a change must
be accounted for by the checksum. The current extended signature entry
checksum code in the Linux microcode driver, which has been in place
since 2003, will be thrown off by this and reject a valid microcode
update.

The microcode driver is better off by doing what the Intel SDM suggests,
and staying well clear of that checksum field. It has already checked
the whole extended signature table's checksum, anyway.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel_lib.c | 20 ++------------------
1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 1cc6494..9200b83 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -49,8 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
unsigned long total_size, data_size, ext_table_size;
struct microcode_header_intel *mc_header = mc;
struct extended_sigtable *ext_header = NULL;
- int sum, orig_sum, ext_sigcount = 0, i;
- struct extended_signature *ext_sig;
+ int orig_sum, i;

total_size = get_totalsize(mc_header);
data_size = get_datasize(mc_header);
@@ -81,7 +80,6 @@ int microcode_sanity_check(void *mc, int print_err)
pr_err("error: bad exttable size in microcode data file\n");
return -EFAULT;
}
- ext_sigcount = ext_header->count;
}

/*
@@ -129,21 +127,7 @@ int microcode_sanity_check(void *mc, int print_err)
pr_err("error: bad microcode update checksum\n");
return -EINVAL;
}
- if (!ext_table_size)
- return 0;
- /* check extended signature checksum */
- for (i = 0; i < ext_sigcount; i++) {
- ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
- EXT_SIGNATURE_SIZE * i;
- sum = orig_sum
- - (mc_header->sig + mc_header->pf + mc_header->cksum)
- + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
- if (sum) {
- if (print_err)
- pr_err("error: bad extended signature checksum\n");
- return -EINVAL;
- }
- }
+
return 0;
}
EXPORT_SYMBOL_GPL(microcode_sanity_check);
--
1.7.10.4
Henrique de Moraes Holschuh
2014-09-08 17:37:48 UTC
Permalink
Fix a regression introduced by 506ed6b53e00ba303ad778122f08e1fca7cf5efb,
"x86, intel: Output microcode revision in /proc/cpuinfo", which added a
cache of the thread microcode revision to cpu_data()->microcode and
switched the microcode driver to using the cached value.

This caused the driver to needlessly update each processor core twice
when hyper-threading is enabled (once per hardware thread). The early
microcode update code that runs during BSP/AP setup does not have this
problem.

Intel microcode update operations are extremely expensive. The WRMSR
79H instruction could take anywhere from a hundred-thousand to several
million cycles to successfully apply a microcode update, depending on
processor model and microcode update size.

To avoid updating the same core twice per microcode update run, refresh
the microcode revision of each CPU (hardware thread) before deciding
whether it needs an update or not.

A silent version of collect_cpu_info() is required for this fix,
otherwise the logs produced by a microcode update run would be twice as
long and very confusing.

Signed-off-by: Henrique de Moraes Holschuh <***@hmh.eng.br>
---
arch/x86/kernel/cpu/microcode/intel.c | 39 ++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c6826d1..2c629d1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -87,7 +87,7 @@ MODULE_DESCRIPTION("Microcode Update Driver");
MODULE_AUTHOR("Tigran Aivazian <***@aivazian.fsnet.co.uk>");
MODULE_LICENSE("GPL");

-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
unsigned int val[2];
@@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}

- csig->rev = c->microcode;
+ /* get the current microcode revision from MSR 0x8B */
+ wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ sync_core();
+ rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+
+ csig->rev = val[1];
+ c->microcode = val[1]; /* re-sync */
+}
+
+static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+{
+ __collect_cpu_info(cpu_num, csig);
+
pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
cpu_num, csig->sig, csig->pf, csig->rev);

@@ -118,7 +130,10 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
struct cpu_signature cpu_sig;
unsigned int csig, cpf, crev;

- collect_cpu_info(cpu, &cpu_sig);
+ /* NOTE: cpu_data()->microcode will be outdated on HT
+ * processors during an update run, it must be refreshed
+ * from MSR 0x8B */
+ __collect_cpu_info(cpu, &cpu_sig);

csig = cpu_sig.sig;
cpf = cpu_sig.pf;
@@ -145,23 +160,21 @@ static int apply_microcode_intel(int cpu)
return 0;

/*
- * Microcode on this CPU could be updated earlier. Only apply the
- * microcode patch in mc_intel when it is newer than the one on this
- * CPU.
+ * Microcode on this CPU might be already up-to-date. Only apply
+ * the microcode patch in mc_intel when it is newer than the one
+ * on this CPU.
*/
if (get_matching_mc(mc_intel, cpu) == 0)
return 0;

- /* write microcode via MSR 0x79 */
+ /* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
wrmsr(MSR_IA32_UCODE_WRITE,
- (unsigned long) mc_intel->bits,
- (unsigned long) mc_intel->bits >> 16 >> 16);
- wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
- /* As documented in the SDM: Do a CPUID 1 here */
- sync_core();
+ lower_32_bits((unsigned long) mc_intel->bits),
+ upper_32_bits((unsigned long) mc_intel->bits));

/* get the current revision from MSR 0x8B */
+ wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ sync_core();
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

if (val[1] != mc_intel->hdr.rev) {
--
1.7.10.4
Borislav Petkov
2014-10-20 13:32:52 UTC
Permalink
Post by Henrique de Moraes Holschuh
Fix a regression introduced by 506ed6b53e00ba303ad778122f08e1fca7cf5efb,
"x86, intel: Output microcode revision in /proc/cpuinfo", which added a
cache of the thread microcode revision to cpu_data()->microcode and
switched the microcode driver to using the cached value.
This caused the driver to needlessly update each processor core twice
when hyper-threading is enabled (once per hardware thread). The early
microcode update code that runs during BSP/AP setup does not have this
problem.
Intel microcode update operations are extremely expensive. The WRMSR
79H instruction could take anywhere from a hundred-thousand to several
million cycles to successfully apply a microcode update, depending on
processor model and microcode update size.
To avoid updating the same core twice per microcode update run, refresh
the microcode revision of each CPU (hardware thread) before deciding
whether it needs an update or not.
A silent version of collect_cpu_info() is required for this fix,
otherwise the logs produced by a microcode update run would be twice as
long and very confusing.
---
arch/x86/kernel/cpu/microcode/intel.c | 39 ++++++++++++++++++++++-----------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c6826d1..2c629d1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -87,7 +87,7 @@ MODULE_DESCRIPTION("Microcode Update Driver");
MODULE_LICENSE("GPL");
-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
unsigned int val[2];
@@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}
- csig->rev = c->microcode;
+ /* get the current microcode revision from MSR 0x8B */
+ wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ sync_core();
+ rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+
+ csig->rev = val[1];
+ c->microcode = val[1]; /* re-sync */
+}
+
+static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+{
+ __collect_cpu_info(cpu_num, csig);
+
pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
cpu_num, csig->sig, csig->pf, csig->rev);
We probably should downgrade this to pr_debug and use collect_cpu_info()
everywhere instead of having a __ version.
Post by Henrique de Moraes Holschuh
@@ -118,7 +130,10 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
struct cpu_signature cpu_sig;
unsigned int csig, cpf, crev;
- collect_cpu_info(cpu, &cpu_sig);
+ /* NOTE: cpu_data()->microcode will be outdated on HT
+ * processors during an update run, it must be refreshed
+ * from MSR 0x8B */
+ __collect_cpu_info(cpu, &cpu_sig);
csig = cpu_sig.sig;
cpf = cpu_sig.pf;
@@ -145,23 +160,21 @@ static int apply_microcode_intel(int cpu)
return 0;
/*
- * Microcode on this CPU could be updated earlier. Only apply the
- * microcode patch in mc_intel when it is newer than the one on this
- * CPU.
+ * Microcode on this CPU might be already up-to-date. Only apply
+ * the microcode patch in mc_intel when it is newer than the one
+ * on this CPU.
*/
if (get_matching_mc(mc_intel, cpu) == 0)
return 0;
- /* write microcode via MSR 0x79 */
+ /* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
No need for screaming here - we know MSR accesses are expensive. This
comment is totally useless here so drop it altogether.
Post by Henrique de Moraes Holschuh
wrmsr(MSR_IA32_UCODE_WRITE,
- (unsigned long) mc_intel->bits,
- (unsigned long) mc_intel->bits >> 16 >> 16);
- wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
- /* As documented in the SDM: Do a CPUID 1 here */
- sync_core();
+ lower_32_bits((unsigned long) mc_intel->bits),
+ upper_32_bits((unsigned long) mc_intel->bits));
wrmsrl() takes u64 directly - no need for the splitting.
Post by Henrique de Moraes Holschuh
/* get the current revision from MSR 0x8B */
+ wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ sync_core();
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
if (val[1] != mc_intel->hdr.rev) {
--
1.7.10.4
--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
Henrique de Moraes Holschuh
2014-10-20 18:24:27 UTC
Permalink
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
unsigned int val[2];
@@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}
- csig->rev = c->microcode;
+ /* get the current microcode revision from MSR 0x8B */
+ wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+ sync_core();
+ rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+
+ csig->rev = val[1];
+ c->microcode = val[1]; /* re-sync */
+}
+
+static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+{
+ __collect_cpu_info(cpu_num, csig);
+
pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
cpu_num, csig->sig, csig->pf, csig->rev);
We probably should downgrade this to pr_debug and use collect_cpu_info()
everywhere instead of having a __ version.
Over time, grepping for that information on reports and logs all over the
net has helped me a great deal. In fact, it is in my backlog to add it to
the early microcode driver as well.

I really miss the full microcode ID information in /proc/cpuinfo, in fact.

If you want, I can modify the logging it in a future patch so that we print
it only once when all cores have the same sig, pf and revision (which should
cover 95% of the systems out there).
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
- /* write microcode via MSR 0x79 */
+ /* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
No need for screaming here - we know MSR accesses are expensive. This
comment is totally useless here so drop it altogether.
MSR 79H writes are on a class of their own as far as "expensive" goes... On
a modern i3/i5/i7, it will take approximately one million cycles to complete
(the larger the microcode update, the longer it takes).

I don't think people usually associate MSR write with "takes one million
cycles to complete"...

That said, I will remove the comment.
Post by Borislav Petkov
Post by Henrique de Moraes Holschuh
wrmsr(MSR_IA32_UCODE_WRITE,
- (unsigned long) mc_intel->bits,
- (unsigned long) mc_intel->bits >> 16 >> 16);
- wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
- /* As documented in the SDM: Do a CPUID 1 here */
- sync_core();
+ lower_32_bits((unsigned long) mc_intel->bits),
+ upper_32_bits((unsigned long) mc_intel->bits));
wrmsrl() takes u64 directly - no need for the splitting.
This is old code, I guess it predates wrmsrl()...

Should I replace the old split version with wrmsrl() in this patch, or as a
separate patch?
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
Loading...