This series fixes a number of cosmetic issues reported by sparse, as well
as some minor issues in the Intel microcode support.
The issues found in the Intel microcode driver are very unlikely to be
triggered by an untampered microcode update... but that could change.
Henrique de Moraes Holschuh (8):
x86, microcode, amd: fix missing static declaration
x86, microcode, intel: fix missing static declarations
x86, microcode, intel: fix typos
x86, microcode, intel: fix missing declaration
x86, microcode, intel: don't use fields from unknown format header
x86, microcode, intel: total_size is valid only when data_size != 0
x86, microcode, intel: forbid some incorrect metadata
x86, microcode, intel: correct extended signature checksum
verification
arch/x86/include/asm/microcode_intel.h | 3 +-
arch/x86/kernel/cpu/microcode/amd_early.c | 2 +-
arch/x86/kernel/cpu/microcode/intel.c | 9 +++--
arch/x86/kernel/cpu/microcode/intel_early.c | 13 +++++---
arch/x86/kernel/cpu/microcode/intel_lib.c | 48 ++++++++++++++++++++++-----
5 files changed, 57 insertions(+), 18 deletions(-)
--
1.7.10.4
gcc reports that a few declarations are missing.
Fix two obvious ones.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 18f7391..b105c11 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -28,8 +28,8 @@
#include <asm/tlbflush.h>
#include <asm/setup.h>
-unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
-struct mc_saved_data {
+static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
+static struct mc_saved_data {
unsigned int mc_saved_count;
struct microcode_intel **mc_saved;
} mc_saved_data;
--
1.7.10.4
We must make sure the microcode has a known header format before
we attempt to access its Total Size or Data Size fields through
get_totalsize() or get_datasize().
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel.c | 5 +++++
arch/x86/kernel/cpu/microcode/intel_early.c | 3 +++
arch/x86/kernel/cpu/microcode/intel_lib.c | 11 ++++++-----
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index a51cb19..61d430e 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -199,6 +199,11 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
if (get_ucode_data(&mc_header, ucode_ptr, sizeof(mc_header)))
break;
+ if (mc_header.hdrver != 1) {
+ pr_err("error! Unknown microcode update format\n");
+ break;
+ }
+
mc_size = get_totalsize(&mc_header);
if (!mc_size || mc_size > leftover) {
pr_err("error! Bad data in microcode data file\n");
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f..c1bf915f 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -324,6 +324,9 @@ get_matching_model_microcode(int cpu, unsigned long start,
while (leftover) {
mc_header = (struct microcode_header_intel *)ucode_ptr;
+ if (mc_header->hdrver != 1)
+ break;
+
mc_size = get_totalsize(mc_header);
if (!mc_size || mc_size > leftover ||
microcode_sanity_check(ucode_ptr, 0) < 0)
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320..95c2d19 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -52,6 +52,12 @@ int microcode_sanity_check(void *mc, int print_err)
int sum, orig_sum, ext_sigcount = 0, i;
struct extended_signature *ext_sig;
+ if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
+ if (print_err)
+ pr_err("error! Unknown microcode update format\n");
+ return -EINVAL;
+ }
+
total_size = get_totalsize(mc_header);
data_size = get_datasize(mc_header);
@@ -61,11 +67,6 @@ int microcode_sanity_check(void *mc, int print_err)
return -EINVAL;
}
- if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
- if (print_err)
- pr_err("error! Unknown microcode update format\n");
- return -EINVAL;
- }
ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
if (ext_table_size) {
if ((ext_table_size < EXT_HEADER_SIZE)
--
1.7.10.4
Fix some typos. One of them was in a struct name, fortunately harmless
because it happened on a "sizeof(struct foo*)" construction.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_early.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b105c11..b88343f 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -415,7 +415,7 @@ static void __ref show_saved_mc(void)
struct ucode_cpu_info uci;
if (mc_saved_data.mc_saved_count == 0) {
- pr_debug("no micorcode data saved.\n");
+ pr_debug("no microcode data saved.\n");
return;
}
pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
@@ -506,7 +506,7 @@ int save_mc_for_early(u8 *mc)
if (mc_saved && mc_saved_count)
memcpy(mc_saved_tmp, mc_saved,
- mc_saved_count * sizeof(struct mirocode_intel *));
+ mc_saved_count * sizeof(struct microcode_intel *));
/*
* Save the microcode patch mc in mc_save_tmp structure if it's a newer
* version.
@@ -526,7 +526,7 @@ int save_mc_for_early(u8 *mc)
show_saved_mc();
/*
- * Free old saved microcod data.
+ * Free old saved microcode data.
*/
if (mc_saved) {
for (i = 0; i < mc_saved_count_init; i++)
--
1.7.10.4
Ensure that both the microcode data_size and total_size fields are a
multiple of the dword size (4 bytes). The Intel SDM vol 3A (order code
253668-051US, June 2014) requires this to be true, and the driver code
assumes it will be true.
Add a comment to the code stating that it is best if we continue to
refrain from ensuring that total_size is a multiple of 1024 bytes. The
reason to never add that check is non-obvious.
Refuse a microcode with a revision of zero, we reserve that for the
factory-provided microcode.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_lib.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 95c2d19..050cd4f 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -61,12 +61,22 @@ 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 % DWSIZE) ||
+ (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;
}
+ /*
+ * DO NOT add a check for total_size to be a multiple of 1024.
+ *
+ * While there is a requirement that total_size be a multiple of 1024
+ * (Intel SDM vol 3A, section 9.11.1, table 9-6, page 9-29), it clashes
+ * with the "delete extended signature table" procedure described for
+ * the Checksum[n] field in the same table 9-6, at page 9-30).
+ */
+
ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
if (ext_table_size) {
if ((ext_table_size < EXT_HEADER_SIZE)
@@ -84,6 +94,13 @@ int microcode_sanity_check(void *mc, int print_err)
ext_sigcount = ext_header->count;
}
+ /* check some of the metadata */
+ if (mc_header->rev == 0) { /* reserved for silicon microcode */
+ 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
We have been calculating the checksum for extended signatures in a way that
is very likely to be incompatible with the Intel public documention. This
code dates back to 2003, when the support for the "new microcode format"
was added to the driver by Intel itself.
The extended signature table should be deleted when an extended signature
is "applied" to the main microcode patch if the Intel SDM is to be believed
(Intel 64 and IA32 Software Developers Manual, vol 3A, page 9-30, entry for
"Checksum[n]" in table 9-6). Deleting the extended signature table changes
the Total Size of the microcode, and that reflects in the checksum that
should be in the extended signature entry if it is to be used unmodified to
replace the main microcode signature.
It is worth noting that deleting the extended signature table results in a
microcode patch that violates the rule that the Total Size field must be a
multiple of 1024, and it is impossible to add any padding to fix that.
This patch changes the extended signature table checksum verification
code to accept both ways of calculating the extended signature checksum
as valid.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel_lib.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 050cd4f..c75f03a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -135,9 +135,21 @@ int microcode_sanity_check(void *mc, int print_err)
sum = orig_sum
- (mc_header->sig + mc_header->pf + mc_header->cksum)
+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
- if (sum) {
+ /*
+ * accept two possibilities for the extended signature entry
+ * checksum: the one we've been using since 2003 (which is
+ * likely incorrect), as well as the one described in the
+ * Intel SDM vol 3A (order #253668-051US, June 2014), table
+ * 9-6, entry for Checksum[n] at page 9-30.
+ *
+ * When one deletes the extended signature table as the Intel
+ * SDM mandates, total_size decreases by ext_table_size, and
+ * so does the checksum, leaving a remainder equal to
+ * ext_table_size in sum.
+ */
+ if (sum != 0 && sum != ext_table_size) {
if (print_err)
- pr_err("aborting, bad checksum\n");
+ pr_err("aborting, bad extended signature checksum\n");
return -EINVAL;
}
}
--
1.7.10.4
gcc warns that a declaration is missing. Fix it.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/kernel/cpu/microcode/amd_early.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd_early.c b/arch/x86/kernel/cpu/microcode/amd_early.c
index 617a9e2..7aa1acc 100644
--- a/arch/x86/kernel/cpu/microcode/amd_early.c
+++ b/arch/x86/kernel/cpu/microcode/amd_early.c
@@ -27,7 +27,7 @@ static u32 ucode_new_rev;
u8 amd_ucode_patch[PATCH_MAX_SIZE];
static u16 this_equiv_id;
-struct cpio_data ucode_cpio;
+static struct cpio_data ucode_cpio;
/*
* Microcode patch container file is prepended to the initrd in cpio format.
--
1.7.10.4
According to the Intel SDM vol 3A (order code 253668-051US, June 2014),
on section 9.11.1, page 9-28:
"For microcode updates with a data size field equal to 00000000H, the
size of the microcode update is 2048 bytes. The first 48 bytes contain
the microcode update header. The remaining 2000 bytes contain encrypted
data."
"For microcode updates with a data size not equal to 00000000H, the total
size field specifies the size of the microcode update"
We were incorrectly assuming that total_size is valid when it is
non-zero, instead of checking data_size to be non-zero. IOW, we were
trusting a reserved field to be zero in a situation where it was, in
fact, undefined.
This is a very old bug, dating back to 2003. It has been dormant ever
since, as Intel seems to set all reserved fields to zero on the
microcode updates they distribute: I could not find a public microcode
update that would trigger this bug.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 2bdbc6b..62f91c1 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -43,7 +43,7 @@ struct extended_sigtable {
#define DWSIZE (sizeof(u32))
#define get_totalsize(mc) \
- (((struct microcode_intel *)mc)->hdr.totalsize ? \
+ (((struct microcode_intel *)mc)->hdr.datasize ? \
((struct microcode_intel *)mc)->hdr.totalsize : \
DEFAULT_UCODE_TOTALSIZE)
--
1.7.10.4
Rename apply_microcode() in microcode/intel.c to apply_microcode_intel(),
and declare it as extern in the asm/microcode_intel.h header.
This is a cosmetic fix to silence a warning issued by sparse. It brings
the microcode/intel.c driver in line with what microcode/amd.c is doing.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 1 +
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 9067166..2bdbc6b 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -62,6 +62,7 @@ extern int microcode_sanity_check(void *mc, int print_err);
extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
extern int
update_match_revision(struct microcode_header_intel *mc_header, int rev);
+extern int apply_microcode_intel(int cpu);
#ifdef CONFIG_MICROCODE_INTEL_EARLY
extern void __init load_ucode_intel_bsp(void);
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index a276fa7..a51cb19 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -127,7 +127,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
return get_matching_microcode(csig, cpf, mc_intel, crev);
}
-int apply_microcode(int cpu)
+int apply_microcode_intel(int cpu)
{
struct microcode_intel *mc_intel;
struct ucode_cpu_info *uci;
@@ -314,7 +314,7 @@ static struct microcode_ops microcode_intel_ops = {
.request_microcode_user = request_microcode_user,
.request_microcode_fw = request_microcode_fw,
.collect_cpu_info = collect_cpu_info,
- .apply_microcode = apply_microcode,
+ .apply_microcode = apply_microcode_intel,
.microcode_fini_cpu = microcode_fini_cpu,
};
--
1.7.10.4
On Wed, Jul 23, 2014 at 05:10:44PM -0300, Henrique de Moraes Holschuh wrote:
> gcc warns that a declaration is missing. Fix it.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Applied, thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:45PM -0300, Henrique de Moraes Holschuh wrote:
> gcc reports that a few declarations are missing.
> Fix two obvious ones.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Applied, thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:46PM -0300, Henrique de Moraes Holschuh wrote:
> Fix some typos. One of them was in a struct name, fortunately harmless
> because it happened on a "sizeof(struct foo*)" construction.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Damn, that's three different, *wrong*, spellings of "microcode" in one
single file! My favourite one is "mirocode".
:-)
Applied, thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:47PM -0300, Henrique de Moraes Holschuh wrote:
> Rename apply_microcode() in microcode/intel.c to apply_microcode_intel(),
> and declare it as extern in the asm/microcode_intel.h header.
>
> This is a cosmetic fix to silence a warning issued by sparse. It brings
> the microcode/intel.c driver in line with what microcode/amd.c is doing.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> arch/x86/include/asm/microcode_intel.h | 1 +
> arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> index 9067166..2bdbc6b 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -62,6 +62,7 @@ extern int microcode_sanity_check(void *mc, int print_err);
> extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
> extern int
> update_match_revision(struct microcode_header_intel *mc_header, int rev);
> +extern int apply_microcode_intel(int cpu);
>
> #ifdef CONFIG_MICROCODE_INTEL_EARLY
> extern void __init load_ucode_intel_bsp(void);
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index a276fa7..a51cb19 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -127,7 +127,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
> return get_matching_microcode(csig, cpf, mc_intel, crev);
> }
>
> -int apply_microcode(int cpu)
> +int apply_microcode_intel(int cpu)
Actually, this function should be static. The AMD counterpart is used in
amd_early.c too, that's why it is exported there, unlike here.
The "_intel" suffix is ok.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:48PM -0300, Henrique de Moraes Holschuh wrote:
> We must make sure the microcode has a known header format before
> we attempt to access its Total Size or Data Size fields through
> get_totalsize() or get_datasize().
Well, this is a pretty weak check but I'm all for being as defensive as
possible with the microcode loader so yes, let's do it.
However, I'd carve it out from microcode_sanity_check() in a separate
function called
static bool microcode_check_header(struct microcode_header_intel *hdr);
and do the ->hdrver and -> ldrver checks in it. In two places to be exact...
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel.c | 5 +++++
> arch/x86/kernel/cpu/microcode/intel_early.c | 3 +++
> arch/x86/kernel/cpu/microcode/intel_lib.c | 11 ++++++-----
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index a51cb19..61d430e 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -199,6 +199,11 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> if (get_ucode_data(&mc_header, ucode_ptr, sizeof(mc_header)))
> break;
>
> + if (mc_header.hdrver != 1) {
> + pr_err("error! Unknown microcode update format\n");
> + break;
> + }
... here ...
> +
> mc_size = get_totalsize(&mc_header);
> if (!mc_size || mc_size > leftover) {
> pr_err("error! Bad data in microcode data file\n");
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index b88343f..c1bf915f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -324,6 +324,9 @@ get_matching_model_microcode(int cpu, unsigned long start,
> while (leftover) {
> mc_header = (struct microcode_header_intel *)ucode_ptr;
>
> + if (mc_header->hdrver != 1)
> + break;
> +
... and here. I.e., everytime we're looking at a mc_header from userspace.
> mc_size = get_totalsize(mc_header);
> if (!mc_size || mc_size > leftover ||
> microcode_sanity_check(ucode_ptr, 0) < 0)
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index ce69320..95c2d19 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -52,6 +52,12 @@ int microcode_sanity_check(void *mc, int print_err)
> int sum, orig_sum, ext_sigcount = 0, i;
> struct extended_signature *ext_sig;
>
> + if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> + if (print_err)
> + pr_err("error! Unknown microcode update format\n");
> + return -EINVAL;
> + }
> +
> total_size = get_totalsize(mc_header);
> data_size = get_datasize(mc_header);
>
> @@ -61,11 +67,6 @@ int microcode_sanity_check(void *mc, int print_err)
> return -EINVAL;
> }
>
> - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> - if (print_err)
> - pr_err("error! Unknown microcode update format\n");
> - return -EINVAL;
> - }
This one is then redundant.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 24 Jul 2014, Borislav Petkov wrote:
> On Wed, Jul 23, 2014 at 05:10:48PM -0300, Henrique de Moraes Holschuh wrote:
> > We must make sure the microcode has a known header format before
> > we attempt to access its Total Size or Data Size fields through
> > get_totalsize() or get_datasize().
>
> Well, this is a pretty weak check but I'm all for being as defensive as
> possible with the microcode loader so yes, let's do it.
>
> However, I'd carve it out from microcode_sanity_check() in a separate
> function called
>
> static bool microcode_check_header(struct microcode_header_intel *hdr);
>
> and do the ->hdrver and -> ldrver checks in it. In two places to be exact...
We care about ->hdrver to get data from the header. We only care about
->ldrver for the exact procedure to install it in the processor.
The more correct implementation would be to not error out, but just skip
anything with an unknown ldrver. We can't do that for an unknown hrdver,
since we don't know the size of the unknown data, but an unknown ldrver just
means we don't know how to punt the microcode to the processor.
Want me to respin the patch to do it like that?
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > ---
> > arch/x86/kernel/cpu/microcode/intel.c | 5 +++++
> > arch/x86/kernel/cpu/microcode/intel_early.c | 3 +++
> > arch/x86/kernel/cpu/microcode/intel_lib.c | 11 ++++++-----
> > 3 files changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index a51cb19..61d430e 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -199,6 +199,11 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> > if (get_ucode_data(&mc_header, ucode_ptr, sizeof(mc_header)))
> > break;
> >
> > + if (mc_header.hdrver != 1) {
> > + pr_err("error! Unknown microcode update format\n");
> > + break;
> > + }
>
> ... here ...
>
> > +
> > mc_size = get_totalsize(&mc_header);
> > if (!mc_size || mc_size > leftover) {
> > pr_err("error! Bad data in microcode data file\n");
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> > index b88343f..c1bf915f 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> > @@ -324,6 +324,9 @@ get_matching_model_microcode(int cpu, unsigned long start,
> > while (leftover) {
> > mc_header = (struct microcode_header_intel *)ucode_ptr;
> >
> > + if (mc_header->hdrver != 1)
> > + break;
> > +
>
> ... and here. I.e., everytime we're looking at a mc_header from userspace.
>
> > mc_size = get_totalsize(mc_header);
> > if (!mc_size || mc_size > leftover ||
> > microcode_sanity_check(ucode_ptr, 0) < 0)
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > index ce69320..95c2d19 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > @@ -52,6 +52,12 @@ int microcode_sanity_check(void *mc, int print_err)
> > int sum, orig_sum, ext_sigcount = 0, i;
> > struct extended_signature *ext_sig;
> >
> > + if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> > + if (print_err)
> > + pr_err("error! Unknown microcode update format\n");
> > + return -EINVAL;
> > + }
> > +
> > total_size = get_totalsize(mc_header);
> > data_size = get_datasize(mc_header);
> >
> > @@ -61,11 +67,6 @@ int microcode_sanity_check(void *mc, int print_err)
> > return -EINVAL;
> > }
> >
> > - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> > - if (print_err)
> > - pr_err("error! Unknown microcode update format\n");
> > - return -EINVAL;
> > - }
>
> This one is then redundant.
Well, I just moved the test to a more appropriate place, it was already
there. I didn't remove it because, IMHO, the intel microcode driver code is
already quite twisty, so I thought it was best to leave that duplicated
check in there just in case.
That said, microcode_sanity_check() requires that the caller perform one of
the most important checks (that the data it got from userspace is large
enough). A better header-version abstraction would give us a
microcode_sanity_check(void **uc, uint32_t* remaining_size, ...) and a new
microcode_skip(void **uc, uint32_t* remaining_size) to walk/validate the
microcode.
I don't think it is really worth it to go that far ATM, though.
--
"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
On Thu, 24 Jul 2014, Borislav Petkov wrote:
> On Wed, Jul 23, 2014 at 05:10:47PM -0300, Henrique de Moraes Holschuh wrote:
> > Rename apply_microcode() in microcode/intel.c to apply_microcode_intel(),
> > and declare it as extern in the asm/microcode_intel.h header.
> >
> > This is a cosmetic fix to silence a warning issued by sparse. It brings
> > the microcode/intel.c driver in line with what microcode/amd.c is doing.
> >
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > ---
> > arch/x86/include/asm/microcode_intel.h | 1 +
> > arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> > index 9067166..2bdbc6b 100644
> > --- a/arch/x86/include/asm/microcode_intel.h
> > +++ b/arch/x86/include/asm/microcode_intel.h
> > @@ -62,6 +62,7 @@ extern int microcode_sanity_check(void *mc, int print_err);
> > extern int get_matching_sig(unsigned int csig, int cpf, void *mc, int rev);
> > extern int
> > update_match_revision(struct microcode_header_intel *mc_header, int rev);
> > +extern int apply_microcode_intel(int cpu);
> >
> > #ifdef CONFIG_MICROCODE_INTEL_EARLY
> > extern void __init load_ucode_intel_bsp(void);
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index a276fa7..a51cb19 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -127,7 +127,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
> > return get_matching_microcode(csig, cpf, mc_intel, crev);
> > }
> >
> > -int apply_microcode(int cpu)
> > +int apply_microcode_intel(int cpu)
>
> Actually, this function should be static. The AMD counterpart is used in
> amd_early.c too, that's why it is exported there, unlike here.
>
> The "_intel" suffix is ok.
Ok, I will respin this 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
On Thu, Jul 24, 2014 at 10:30:59AM -0300, Henrique de Moraes Holschuh wrote:
> We care about ->hdrver to get data from the header. We only care about
> ->ldrver for the exact procedure to install it in the processor.
Does it matter?
microcode_sanity_check() errors out if any of the two are not 1 and we
end up not applying the microcode if so.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 24 Jul 2014, Borislav Petkov wrote:
> On Thu, Jul 24, 2014 at 10:30:59AM -0300, Henrique de Moraes Holschuh wrote:
> > We care about ->hdrver to get data from the header. We only care about
> > ->ldrver for the exact procedure to install it in the processor.
>
> Does it matter?
It might. It could break backwards-compatibility in the microcode update
distribution.
> microcode_sanity_check() errors out if any of the two are not 1 and we
> end up not applying the microcode if so.
We don't just skip that microcode, we also skip every microcode after that
one, because we _abort_ on the first error we find. This is not the best
possible behaviour for a ldrver mismatch, where we could safely skip just
that one microcode.
Suppose you have a box that takes ldrver 1 microcode, and Intel releases
microcode for a new type of core that has a ldrver of 2, and it happens to
not be the last one in the microcode collection sent by userspace (via the
early initrd or /dev/cpu/microcode). We might well abort before we find the
correct microcode update for that box.
It is an unlikely scenario, but AFAIK it is not impossible. All it takes is
Intel releasing something APU-like and deciding to use the microcode update
facilities to update something that is not a CPU core.
--
"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
On Thu, Jul 24, 2014 at 12:07:40PM -0300, Henrique de Moraes Holschuh wrote:
> Suppose you have a box that takes ldrver 1 microcode, and Intel
> releases microcode for a new type of core that has a ldrver of 2, and
> it happens to not be the last one in the microcode collection sent by
> userspace (via the early initrd or /dev/cpu/microcode). We might well
> abort before we find the correct microcode update for that box.
And? The ldrver 2 header will enter microcode_sanity_check() and abort
there. A bit later. Same deal.
If you want to *skip* over ldrver 2 ucode headers but continue looping
over the ucode data, then you need to do more than that.
So what exactly are you trying to fix here?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 24 Jul 2014, Borislav Petkov wrote:
> On Thu, Jul 24, 2014 at 12:07:40PM -0300, Henrique de Moraes Holschuh wrote:
> > Suppose you have a box that takes ldrver 1 microcode, and Intel
> > releases microcode for a new type of core that has a ldrver of 2, and
> > it happens to not be the last one in the microcode collection sent by
> > userspace (via the early initrd or /dev/cpu/microcode). We might well
> > abort before we find the correct microcode update for that box.
>
> And? The ldrver 2 header will enter microcode_sanity_check() and abort
> there. A bit later. Same deal.
>
> If you want to *skip* over ldrver 2 ucode headers but continue looping
> over the ucode data, then you need to do more than that.
Now that I noticed that ldrver problem exists, I want to do that as well,
but that was _not_ the purpose of this patch and it should be done as a
separate change anyway.
> So what exactly are you trying to fix here?
I want to stop accessing fields inside an unknown format of microcode
header.
I didn't notice I could remove the test inside microcode_sanity_check(),
just that it was in the wrong place. However, even if I had noticed it was
a duplicate, I would have not removed it: IMHO, moving it up a bit like
this patch did makes that duplicate test useful as documentation, and it is
true to the intent of the function.
I will respin this patch using the helpers you proposed, and add a new one
enhancing the way we deal with ldrver.
--
"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
Rename apply_microcode() in microcode/intel.c to apply_microcode_intel(),
and declare it as static.
This is a cosmetic fix to silence a warning issued by sparse. It brings
the microcode/intel.c driver in line with what microcode/amd.c is doing.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
---
v2: declare as static instead of extern, and changed patch title
arch/x86/kernel/cpu/microcode/intel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index a276fa7..c6826d1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -127,7 +127,7 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
return get_matching_microcode(csig, cpf, mc_intel, crev);
}
-int apply_microcode(int cpu)
+static int apply_microcode_intel(int cpu)
{
struct microcode_intel *mc_intel;
struct ucode_cpu_info *uci;
@@ -314,7 +314,7 @@ static struct microcode_ops microcode_intel_ops = {
.request_microcode_user = request_microcode_user,
.request_microcode_fw = request_microcode_fw,
.collect_cpu_info = collect_cpu_info,
- .apply_microcode = apply_microcode,
+ .apply_microcode = apply_microcode_intel,
.microcode_fini_cpu = microcode_fini_cpu,
};
--
1.7.10.4
On Thu, Jul 24, 2014 at 03:23:21PM -0300, Henrique de Moraes Holschuh wrote:
> Rename apply_microcode() in microcode/intel.c to apply_microcode_intel(),
> and declare it as static.
>
> This is a cosmetic fix to silence a warning issued by sparse. It brings
> the microcode/intel.c driver in line with what microcode/amd.c is doing.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> v2: declare as static instead of extern, and changed patch title
Applied, thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:49PM -0300, Henrique de Moraes Holschuh wrote:
> According to the Intel SDM vol 3A (order code 253668-051US, June 2014),
> on section 9.11.1, page 9-28:
>
> "For microcode updates with a data size field equal to 00000000H, the
> size of the microcode update is 2048 bytes. The first 48 bytes contain
> the microcode update header. The remaining 2000 bytes contain encrypted
> data."
>
> "For microcode updates with a data size not equal to 00000000H, the total
> size field specifies the size of the microcode update"
>
> We were incorrectly assuming that total_size is valid when it is
> non-zero, instead of checking data_size to be non-zero. IOW, we were
> trusting a reserved field to be zero in a situation where it was, in
> fact, undefined.
I'm perfectly fine with the patch except this statement above. I can't
parse it. What reserved field?
I'd guess the packages they distribute always have total_size
initialized properly, i.e. in the case where data_size is 0, total_size
is 2048 anyway. This is maybe the reason why nobody hit this bug as
get_totalsize() returning always the correct number.
Right?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri, 25 Jul 2014, Borislav Petkov wrote:
> On Wed, Jul 23, 2014 at 05:10:49PM -0300, Henrique de Moraes Holschuh wrote:
> > According to the Intel SDM vol 3A (order code 253668-051US, June 2014),
> > on section 9.11.1, page 9-28:
> >
> > "For microcode updates with a data size field equal to 00000000H, the
> > size of the microcode update is 2048 bytes. The first 48 bytes contain
> > the microcode update header. The remaining 2000 bytes contain encrypted
> > data."
> >
> > "For microcode updates with a data size not equal to 00000000H, the total
> > size field specifies the size of the microcode update"
> >
> > We were incorrectly assuming that total_size is valid when it is
> > non-zero, instead of checking data_size to be non-zero. IOW, we were
> > trusting a reserved field to be zero in a situation where it was, in
> > fact, undefined.
>
> I'm perfectly fine with the patch except this statement above. I can't
> parse it. What reserved field?
Yeah, that commit text could be a bit more clear...
Up to 2002/2003, Intel used an "old format" for the microcode update
containers that was always 2048 bytes in size. That old format did not have
Data Size and Total Size fields, the quadwords at those positions in the
microcode container header were "reserved". The microcode header of the
"old format" microcode container has a hrdver of 0x01. You can hunt down an
old copy of the Intel SDM to validate this through its order number
(#243192). I found one from 1999 through a Google search.
Sometime in 2002/2003 (AFAICT, for the Prescott processors), Intel
documented a new format for the microcode containers and contributed in 2003
some code to the Linux kernel microcode driver implementing support for the
new format. This new format has Data Size and Total Size fields, as well as
the optional extended signature table. However, it reuses the same hrdver
as the old format (0x01), and it can only be told apart from the old format
by a non-zero Data Size field.
In fact, the only reason we can even trust a Data Size of zero to mean that
the microcode container is in the old format, is because Intel reatroatively
promised that the old format would always have a zero there when they wrote
the documentation for the _new_ format.
I have a hunch that the processor flags field went through a similar
extension in the late 1990's. I couldn't find an old enough Intel SDM to
validate this, though.
Well, enough with the archeology.
> I'd guess the packages they distribute always have total_size
> initialized properly, i.e. in the case where data_size is 0, total_size
> is 2048 anyway. This is maybe the reason why nobody hit this bug as
> get_totalsize() returning always the correct number.
>
> Right?
Correct.
As far as I can tell, Intel has always filled the reserved fields in the
microcode data files with zeros. That's why this bug is harmless and went
"undetected" for >10 years.
--
"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
On Fri, Jul 25, 2014 at 04:04:11PM -0300, Henrique de Moraes Holschuh wrote:
> As far as I can tell, Intel has always filled the reserved fields in the
> microcode data files with zeros.
Yes, and this is the statement I was looking for. IF(!) this really
is the case, then we don't have anything to worry about because new
kernel with old container will look at ->datasize, see 0 and return
DEFAULT_UCODE_TOTALSIZE and we're fine.
I've updated the patch with your additional explanation.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jul 23, 2014 at 05:10:50PM -0300, Henrique de Moraes Holschuh wrote:
> Ensure that both the microcode data_size and total_size fields are a
> multiple of the dword size (4 bytes). The Intel SDM vol 3A (order code
> 253668-051US, June 2014) requires this to be true, and the driver code
> assumes it will be true.
>
> Add a comment to the code stating that it is best if we continue to
> refrain from ensuring that total_size is a multiple of 1024 bytes. The
> reason to never add that check is non-obvious.
>
> Refuse a microcode with a revision of zero, we reserve that for the
> factory-provided microcode.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel_lib.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index 95c2d19..050cd4f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -61,12 +61,22 @@ 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 % DWSIZE) ||
> + (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;
> }
>
> + /*
> + * DO NOT add a check for total_size to be a multiple of 1024.
> + *
> + * While there is a requirement that total_size be a multiple of 1024
> + * (Intel SDM vol 3A, section 9.11.1, table 9-6, page 9-29), it clashes
> + * with the "delete extended signature table" procedure described for
> + * the Checksum[n] field in the same table 9-6, at page 9-30).
Why? I don't see anything wrong with doing
->total_size % 1024
as an additional sanity check. It's a whole another question how much it
would catch but it doesn't hurt to do it as part of us being defensive.
> + /* check some of the metadata */
> + if (mc_header->rev == 0) { /* reserved for silicon microcode */
> + if (print_err)
> + pr_err("error! Restricted revision 0 in microcode data file\n");
> + return -EINVAL;
> + }
What is "factory-provided" microcode? What is this check supposed to
accomplish?
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Mon, 28 Jul 2014, Borislav Petkov wrote:
> On Fri, Jul 25, 2014 at 04:04:11PM -0300, Henrique de Moraes Holschuh wrote:
> > As far as I can tell, Intel has always filled the reserved fields in the
> > microcode data files with zeros.
>
> Yes, and this is the statement I was looking for. IF(!) this really
> is the case, then we don't have anything to worry about because new
> kernel with old container will look at ->datasize, see 0 and return
> DEFAULT_UCODE_TOTALSIZE and we're fine.
We're fine AFAICT.
If there's an official microcode update out there that can trigger this
bug, it didn't come from a Linux distro, from Intel's public download site
or from urbanmyth.org.
I've checked 346 unique microcode updates for 189 unique Intel processors,
and none would cause problems. That test corpus covers processors as old
as cpuid 0x611. It is used to check iucode-tool for regressions.
> I've updated the patch with your additional explanation.
Thank you!
--
"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
On Mon, 28 Jul 2014, Borislav Petkov wrote:
> On Wed, Jul 23, 2014 at 05:10:50PM -0300, Henrique de Moraes Holschuh wrote:
> > Ensure that both the microcode data_size and total_size fields are a
> > multiple of the dword size (4 bytes). The Intel SDM vol 3A (order code
> > 253668-051US, June 2014) requires this to be true, and the driver code
> > assumes it will be true.
> >
> > Add a comment to the code stating that it is best if we continue to
> > refrain from ensuring that total_size is a multiple of 1024 bytes. The
> > reason to never add that check is non-obvious.
> >
> > Refuse a microcode with a revision of zero, we reserve that for the
> > factory-provided microcode.
> >
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > ---
> > arch/x86/kernel/cpu/microcode/intel_lib.c | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > index 95c2d19..050cd4f 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > @@ -61,12 +61,22 @@ 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 % DWSIZE) ||
> > + (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;
> > }
> >
> > + /*
> > + * DO NOT add a check for total_size to be a multiple of 1024.
> > + *
> > + * While there is a requirement that total_size be a multiple of 1024
> > + * (Intel SDM vol 3A, section 9.11.1, table 9-6, page 9-29), it clashes
> > + * with the "delete extended signature table" procedure described for
> > + * the Checksum[n] field in the same table 9-6, at page 9-30).
>
> Why? I don't see anything wrong with doing
>
> ->total_size % 1024
>
> as an additional sanity check. It's a whole another question how much it
> would catch but it doesn't hurt to do it as part of us being defensive.
Well, it might actually hurt.
Keep in mind that the test is not required as far as the Linux kernel driver
is concerned. We really don't care, it would be just an additional test.
What we do care about is that data_size and total_size are multiples of 4
(dword size), and that total_size is exactly large enough for the header,
the data payload, and the optional extended signature table, if any.
Anyway, a "total_size % 1024" test makes it impossible for an userspace tool
to split a microcode data file with multiple signatures into several
microcodes with a single signature and no extended signature table.
That kind of split seems to be the whole point of adding per-entry checksums
to the extended signatures (hinted at by the Intel SDM, vol3A, page 9-30,
last row of table 9-6).
It seems safer to just not test for it:
* FreeBSD seems to just WRMSR directly to the processor whatever crap you
feed it, no checks done whatsoever. Userspace has to do everything.
* Intel BITS (r.1084) seems to have broken code. FWIW, it doesn't test for
total_size % 1024.
* Microsoft Windows distributes the driver and the microcode data in the
same DLL, so one will be updated to match the other. I have no idea what
it does inside the DLL.
* TianoCore UDK2014 doesn't test for total_size % 1024.
* OpenSolaris doesn't check for total_size % 1024 either.
Given that mess, I'd rather we don't be the only ones that will refuse a
microcode that is not a multiple of 1024 bytes in size.
> > + /* check some of the metadata */
> > + if (mc_header->rev == 0) { /* reserved for silicon microcode */
> > + if (print_err)
> > + pr_err("error! Restricted revision 0 in microcode data file\n");
> > + return -EINVAL;
> > + }
>
> What is "factory-provided" microcode? What is this check supposed to
> accomplish?
Factory-provided is whatever microcode is hardwired in silicon and active
after processor power up/hard reset.
The procedure to check whether a microcode update was installed is this:
Load MSR 0x8B (IA32_BIOS_SIGN_ID) with a zero.
cpuid(1)
Check value in MSR 0x8B (IA32_BIOS_SIGN_ID).
The documentation says that the contents of the IA32_BIOS_SIGN_ID MSR will
be changed by the cpuid instruction when a microcode update is installed in
the processor, and left unchanged otherwise. If it is changed, it will be
set to the revision of the microcode running in the processor.
Since we write a zero to the MSR before the cpuid(1), we use zero to detect
the lack of change on the MSR. AFAIK, we *must* do it this way: Intel SDM
vol 3A, section 9.11.7.
The result is that we cannot detect whether a microcode with a revision of
zero has been installed succesfully or not. This alone is already grounds
for rejecting such a microcode update IMHO.
Note that currently we cannot even *attempt* to install such a microcode,
anyway. But we will be silent about it without this 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
On Wed, 23 Jul 2014, Henrique de Moraes Holschuh wrote:
> We have been calculating the checksum for extended signatures in a way that
> is very likely to be incompatible with the Intel public documention. This
> code dates back to 2003, when the support for the "new microcode format"
> was added to the driver by Intel itself.
>
> The extended signature table should be deleted when an extended signature
> is "applied" to the main microcode patch if the Intel SDM is to be believed
> (Intel 64 and IA32 Software Developers Manual, vol 3A, page 9-30, entry for
> "Checksum[n]" in table 9-6). Deleting the extended signature table changes
> the Total Size of the microcode, and that reflects in the checksum that
> should be in the extended signature entry if it is to be used unmodified to
> replace the main microcode signature.
>
> It is worth noting that deleting the extended signature table results in a
> microcode patch that violates the rule that the Total Size field must be a
> multiple of 1024, and it is impossible to add any padding to fix that.
>
> This patch changes the extended signature table checksum verification
> code to accept both ways of calculating the extended signature checksum
> as valid.
It looks like this might not be enough.
TianoCore USDK2014 (UEFI reference code) and OpenSolaris do it in a
completely different, and utterly incompatible way. For the record, they
sum all three dwords on the extended signature entry (including the checksum
one) and expect it to be zero. How the heck did they went from what is
described in the Intel SDM to that crazy code, I have no idea.
At this point, I do think we could just remove the entire extended signature
support, and be happy with the LOC reduction. That's code that has never
seen use in 11 years, and which nobody seems to agree on how it should be
implemented. I very much doubt it will ever be used at this point.
Anyway, I propose we stop checking the checksum on the extended microcode
signature entirely. They are already covered by the checksum of the
extended microcode signature *table* (which covers all extended signature
entries).
--
"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
On Mon, Jul 28, 2014 at 04:37:35PM -0300, Henrique de Moraes Holschuh wrote:
> Keep in mind that the test is not required as far as the Linux kernel driver
> is concerned. We really don't care, it would be just an additional test.
This is what I mean.
> What we do care about is that data_size and total_size are multiples of 4
> (dword size), and that total_size is exactly large enough for the header,
> the data payload, and the optional extended signature table, if any.
I'm with you so far.
> Anyway, a "total_size % 1024" test makes it impossible for an userspace tool
> to split a microcode data file with multiple signatures into several
> microcodes with a single signature and no extended signature table.
So on Intel a userspace tool is splitting the blob into chunks and
correcting total_size too?
> That kind of split seems to be the whole point of adding per-entry checksums
> to the extended signatures (hinted at by the Intel SDM, vol3A, page 9-30,
> last row of table 9-6).
>
> It seems safer to just not test for it:
>
> * FreeBSD seems to just WRMSR directly to the processor whatever crap you
> feed it, no checks done whatsoever. Userspace has to do everything.
CPU wouldn't load corrupted microcode anyway, you know that.
> * Intel BITS (r.1084) seems to have broken code. FWIW, it doesn't test for
> total_size % 1024.
>
> * Microsoft Windows distributes the driver and the microcode data in the
> same DLL, so one will be updated to match the other. I have no idea what
> it does inside the DLL.
Who cares.
> * TianoCore UDK2014 doesn't test for total_size % 1024.
>
> * OpenSolaris doesn't check for total_size % 1024 either.
>
> Given that mess, I'd rather we don't be the only ones that will refuse a
> microcode that is not a multiple of 1024 bytes in size.
So is the recommendation in the just for fun?
And AFAICT, the only reason why we don't check it is the split, correct?
So why do we need that split again? Is Intel microcode so big so that we
can't keep it in a single blob, the exact same way it comes from them?
The microcode blob I get there is this:
https://downloadcenter.intel.com/Detail_Desc.aspx?DwnldID=23984&lang=eng
which is 0.75Mb.
We're perfectly fine handling such a blob as a whole AFAICT.
> > > + /* check some of the metadata */
> > > + if (mc_header->rev == 0) { /* reserved for silicon microcode */
> > > + if (print_err)
> > > + pr_err("error! Restricted revision 0 in microcode data file\n");
> > > + return -EINVAL;
> > > + }
> >
> > What is "factory-provided" microcode? What is this check supposed to
> > accomplish?
>
> Factory-provided is whatever microcode is hardwired in silicon and active
> after processor power up/hard reset.
I think you mean the microcode that's in the BIOS. There's no "hardwired"
microcode AFAIK.
> The procedure to check whether a microcode update was installed is this:
>
> Load MSR 0x8B (IA32_BIOS_SIGN_ID) with a zero.
> cpuid(1)
> Check value in MSR 0x8B (IA32_BIOS_SIGN_ID).
>
> The documentation says that the contents of the IA32_BIOS_SIGN_ID MSR will
> be changed by the cpuid instruction when a microcode update is installed in
> the processor, and left unchanged otherwise. If it is changed, it will be
> set to the revision of the microcode running in the processor.
>
> Since we write a zero to the MSR before the cpuid(1), we use zero to detect
> the lack of change on the MSR. AFAIK, we *must* do it this way: Intel SDM
> vol 3A, section 9.11.7.
>
> The result is that we cannot detect whether a microcode with a revision of
> zero has been installed succesfully or not. This alone is already grounds
> for rejecting such a microcode update IMHO.
I don't think there will ever be a valid distributed microcode with a
revision of 0. We probably should ask someone from Intel to confirm but
I think this is a non-issue: you will have some microcode revision > 0
always loaded from the BIOS.
And even if you manipulated the headers, I think the correct revision is
contained in the encrypted part too so it'll come out in MSR 0x8B after
a successful update even with a corrupted header.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Tue, 29 Jul 2014, Borislav Petkov wrote:
> > Anyway, a "total_size % 1024" test makes it impossible for an userspace tool
> > to split a microcode data file with multiple signatures into several
> > microcodes with a single signature and no extended signature table.
>
> So on Intel a userspace tool is splitting the blob into chunks and
> correcting total_size too?
I've never seen such a tool. That said, the only reason iucode-tool doesn't
know how to do this, is because I don't think we will ever see microcode
with extended signature tables in the wild.
However, Trying to make it possible for such a tool to work is the only
possible explanation for what is written in the Intel SDM vol 3A page 9-30.
It appears to be the whole point of that extra per-signature checksum inside
the extended signature table.
The Intel SDM specifically mentions "creating a patched microcode update"
from a microcode with extended signatures, and "removal of the extended
signature table" on page 9-30 (last row of table 9-6).
> > That kind of split seems to be the whole point of adding per-entry checksums
> > to the extended signatures (hinted at by the Intel SDM, vol3A, page 9-30,
> > last row of table 9-6).
> >
> > It seems safer to just not test for it:
> >
> > * FreeBSD seems to just WRMSR directly to the processor whatever crap you
> > feed it, no checks done whatsoever. Userspace has to do everything.
>
> CPU wouldn't load corrupted microcode anyway, you know that.
Yeah, I do.
> > * TianoCore UDK2014 doesn't test for total_size % 1024.
> >
> > * OpenSolaris doesn't check for total_size % 1024 either.
> >
> > Given that mess, I'd rather we don't be the only ones that will refuse a
> > microcode that is not a multiple of 1024 bytes in size.
>
> So is the recommendation in the just for fun?
It probably made life easier for the bizantine tools used by either Intel
itself or their BIOS partners in 1990, or something of that sort.
> And AFAICT, the only reason why we don't check it is the split, correct?
The only _documented_ reason why we might not want to check it is the split,
yes.
But we'd be the only ones doing that check, if we implement it.
> So why do we need that split again? Is Intel microcode so big so that we
> can't keep it in a single blob, the exact same way it comes from them?
Those extended tables are only useful to pack microcode in FLASH, really.
It must have been a concern in the early 2000's.
The split would remove the extended signature table, so stuff that cannot
deal with it properly can be fed a more compatible version of the microcode
container... As long as they don't care for the total_size % 1024, that is.
Supposedly, we don't need to care about split microcode if we implemented
extended signature tables correctly. Only, there is no way at the moment
for us to even really trust the Intel SDM to be correct, as Intel's own UEFI
reference code (TianoCore UDK2014) does something entirely different from
what is written in the Intel SDM...
> > > > + /* check some of the metadata */
> > > > + if (mc_header->rev == 0) { /* reserved for silicon microcode */
> > > > + if (print_err)
> > > > + pr_err("error! Restricted revision 0 in microcode data file\n");
> > > > + return -EINVAL;
> > > > + }
> > >
> > > What is "factory-provided" microcode? What is this check supposed to
> > > accomplish?
> >
> > Factory-provided is whatever microcode is hardwired in silicon and active
> > after processor power up/hard reset.
>
> I think you mean the microcode that's in the BIOS. There's no "hardwired"
> microcode AFAIK.
Are you sure of that? I've seen reports of a few older processors (some
desktop and even some Xeons) running with revision 0 microcode (i.e. no
updates installed) on BIOS mod sites, when the BIOS was missing a microcode
update for that processor. And it worked well enough for Win XP to run.
Maybe it is buggy-as-all-heck microcode, or missing half the features...
> > The procedure to check whether a microcode update was installed is this:
> >
> > Load MSR 0x8B (IA32_BIOS_SIGN_ID) with a zero.
> > cpuid(1)
> > Check value in MSR 0x8B (IA32_BIOS_SIGN_ID).
> >
> > The documentation says that the contents of the IA32_BIOS_SIGN_ID MSR will
> > be changed by the cpuid instruction when a microcode update is installed in
> > the processor, and left unchanged otherwise. If it is changed, it will be
> > set to the revision of the microcode running in the processor.
> >
> > Since we write a zero to the MSR before the cpuid(1), we use zero to detect
> > the lack of change on the MSR. AFAIK, we *must* do it this way: Intel SDM
> > vol 3A, section 9.11.7.
> >
> > The result is that we cannot detect whether a microcode with a revision of
> > zero has been installed succesfully or not. This alone is already grounds
> > for rejecting such a microcode update IMHO.
>
> I don't think there will ever be a valid distributed microcode with a
> revision of 0. We probably should ask someone from Intel to confirm but
I agree with you that we will never get such a microcode update from Intel.
> I think this is a non-issue: you will have some microcode revision > 0
> always loaded from the BIOS.
That one I know to be false, unfortunately. Although it usually happens
when you add a processor model that is much newer than the motherboard :-)
Still, while it is a non-issue in the sense that we most probably will never
get an official microcode update from Intel with a revision of zero, it
still exposes unconfortable boundary conditions in the driver code. And
THAT is the reason I proposed to reject any such microcode updates.
> And even if you manipulated the headers, I think the correct revision is
> contained in the encrypted part too so it'll come out in MSR 0x8B after
> a successful update even with a corrupted header.
As far as I know, you're correct. We might even want to detect that and
warn when it happens, as it is one easy to implement microcode downgrade
attack that anyone could do.
--
"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