[PATCH 0/8] x86, microcode: cosmetic and minor issue fixes

[PATCH 0/8] x86, microcode: cosmetic and minor issue fixes

[Henrique de Moraes Holschuh]

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

[PATCH 1/8] x86, microcode, amd: fix missing static declaration

[Henrique de Moraes Holschuh]

gcc warns that a declaration is missing.  Fix it.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 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

[PATCH 2/8] x86, microcode, intel: fix missing static declarations

[Henrique de Moraes Holschuh]

gcc reports that a few declarations are missing.
Fix two obvious ones.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 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

[PATCH 3/8] x86, microcode, intel: fix typos

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

[PATCH 4/8] x86, microcode, intel: fix missing declaration

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

[PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

[PATCH 6/8] x86, microcode, intel: total_size is valid only when data_size != 0

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

[PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

[PATCH 8/8] x86, microcode, intel: correct extended signature checksum verification

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
---
 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

Re: [PATCH 1/8] x86, microcode, amd: fix missing static declaration

[Borislav Petkov]

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 <hmh@hmh.eng.br>

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 2/8] x86, microcode, intel: fix missing static declarations

[Borislav Petkov]

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 <hmh@hmh.eng.br>

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 3/8] x86, microcode, intel: fix typos

[Borislav Petkov]

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 <hmh@hmh.eng.br>

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.
--

Re: [PATCH 4/8] x86, microcode, intel: fix missing declaration

[Borislav Petkov]

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 <hmh@hmh.eng.br>
> ---
>  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.
--

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Borislav Petkov]

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 <hmh@hmh.eng.br>
> ---
>  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.
--

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Henrique de Moraes Holschuh]

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 <hmh@hmh.eng.br>
> > ---
> >  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

Re: [PATCH 4/8] x86, microcode, intel: fix missing declaration

[Henrique de Moraes 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 <hmh@hmh.eng.br>
> > ---
> >  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

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Borislav Petkov]

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.
--

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Henrique de Moraes Holschuh]

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

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Borislav Petkov]

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.
--

Re: [PATCH 5/8] x86, microcode, intel: don't use fields from unknown format header

[Henrique de Moraes Holschuh]

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

[PATCH v2 4/8] x86, microcode, intel: rename apply_microcode and declare it static

[Henrique de Moraes 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 <hmh@hmh.eng.br>
---
 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

Re: [PATCH v2 4/8] x86, microcode, intel: rename apply_microcode and declare it static

[Borislav Petkov]

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 <hmh@hmh.eng.br>
> ---
>  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.
--

Re: [PATCH 6/8] x86, microcode, intel: total_size is valid only when data_size != 0

[Borislav Petkov]

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.
--

Re: [PATCH 6/8] x86, microcode, intel: total_size is valid only when data_size != 0

[Henrique de Moraes Holschuh]

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

Re: [PATCH 6/8] x86, microcode, intel: total_size is valid only when data_size != 0

[Borislav Petkov]

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.
--

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

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 <hmh@hmh.eng.br>
> ---
>  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.
--

Re: [PATCH 6/8] x86, microcode, intel: total_size is valid only when data_size != 0

[Henrique de Moraes Holschuh]

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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes 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 <hmh@hmh.eng.br>
> > ---
> >  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

Re: [PATCH 8/8] x86, microcode, intel: correct extended signature checksum verification

[Henrique de Moraes 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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

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.
--

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Tue, Jul 29, 2014 at 05:25:43PM -0300, Henrique de Moraes Holschuh wrote:

> 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.

Never say never.

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

Well, having an extended structure is for cases where you need to load
more patches, or additional patches or so. I think they're leaving this
option open for the future without having to change every microcode
loader out there.

> 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.

Ok, so what do we end up doing in the end? do we split the blob we get
from Intel and if so, do we adjust total_size?

If we do split into smaller patches, then there's no need for total_size
checking, regardless of the extended signature stuff.

> > 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?

You still didn't answer my question: does the iucode-tool do anything to
the microcode blob and if so, what?

Because I think it would be better if we simply load the microcode blob
we get from Intel unchanged. Like we do on AMD.

> 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...

Why do we care? The only thing we have to adhere is what we get from
Intel. I strongly assume that the blob adheres to the SDM and not to
Tiano.

> > 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...

Most likely.

Regardless, your test "if (mc_header->rev == 0)" is pointless because
if the CPU loads the microcode fine, then it was meant to do so. (I'm
strongly assuming microcode release process has vetted this case). So
this test might hurt more than help.

> 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.

See above.

> 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.

Again, such tests are unreliable for us to deal with - in such cases
we'd leave it to the CPU itself to decide whether to allow microcode
downgrade or not.

Besides, you can remove the microcode loader and do the loading
yourself, bypassing all our checks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

On Mon, 04 Aug 2014, Borislav Petkov wrote:
> On Tue, Jul 29, 2014 at 05:25:43PM -0300, Henrique de Moraes Holschuh wrote:
> > 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.
> 
> Never say never.

Indeed.

Which is why I am actually bothered to fix iucode-tool to handle such
microcode without doing anything too idiotic.  Such changes will be included
in the next release (v1.1).

Still, this extended signature table is a >10-year-old design which has
never been used, and which has a number of sharp corners.  We already know
Intel won't be able to trust the implementations out there to deal with
extended signature tables correctly without a round of fixes anyway, so I
consider it just as likely that they will drop it, and maybe come up with
something better.

> > 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.
> 
> Ok, so what do we end up doing in the end? do we split the blob we get
> from Intel and if so, do we adjust total_size?

The kernel wouldn't need to split anything, but it could *conceivably* have
to deal with microcode that has been subject to such a process.

Which it currently would accept just fine.

I feel this is one point where we'll be more robust if we keep the current
code behavior, and ignore the total_size % 1024 restriction.

> > > 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?
> 
> You still didn't answer my question: does the iucode-tool do anything to
> the microcode blob and if so, what?

iucode-tool will leave the microcode update as is.  It doesn't modify the
microcode header.  It doesn't modify the microcode opaque data.

And it doesn't split a microcode with extended signatures into several
microcodes with the extended signature table deleted, either.

> Because I think it would be better if we simply load the microcode blob
> we get from Intel unchanged. Like we do on AMD.

And like we currently do on Intel.  We agree on this, I don't want the
kernel microcode driver to split anything.

> > 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...
> 
> Why do we care? The only thing we have to adhere is what we get from
> Intel. I strongly assume that the blob adheres to the SDM and not to
> Tiano.

I would hope so as well, but I am a bit more sceptical than you on this.

> Regardless, your test "if (mc_header->rev == 0)" is pointless because
> if the CPU loads the microcode fine, then it was meant to do so. (I'm
> strongly assuming microcode release process has vetted this case). So
> this test might hurt more than help.

Note that we currently will never update to a revision of zero anyway: the
anti-downgrade logic and uc_header->rev being an unsigned type makes it
impossible.

This test doesn't change the current status quo much, we will just reject
the update and return an error instead skipping it silently and returning no
error.

Actually, the Intel SDM may help clear these waters a bit.  I've just read
it once more, and found this on the Intel SDM vol 3A, section 9.11.7, page
9-36:

"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."

Reading a revision of zero really is supposed to mean "no update is present
in the processor", and that's because it must be pre-loaded with a zero
before cpuid is called.

IMHO, this mean that one should be really paranoid over any Intel microcode
update that claims to have a revision of zero.  Intel wouldn't release such
a microcode update except in error, and we can safely assume we want nothing
to do with any such update attempts.

> > 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.
> 
> Again, such tests are unreliable for us to deal with - in such cases
> we'd leave it to the CPU itself to decide whether to allow microcode
> downgrade or not.

I've implemented a rough version of it, and the "tampered revision in the
microcode header" test is actually reliable, except maybe when dealing with
an update to revision zero.  Meh.

Anyway, adding such a test to the early microcode driver requires enhancing
intel_early.c to be able to better report update errors on the BSP first
(which is useful by itself), and that part isn't done yet.

Once it is finished and tested, I will send it and we can discuss whether it
is worthwhile or not to apply it.

> Besides, you can remove the microcode loader and do the loading
> yourself, bypassing all our checks.

Yeah, well, if you have CONFIG_X86_MSR enabled, all bets are off.  Thanks
for reminding me about that one.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Mon, Aug 04, 2014 at 05:18:36PM -0300, Henrique de Moraes Holschuh wrote:
> > Because I think it would be better if we simply load the microcode blob
> > we get from Intel unchanged. Like we do on AMD.
> 
> And like we currently do on Intel.  We agree on this, I don't want the
> kernel microcode driver to split anything.

Ok.

So if we don't split, we can savely check ->total_size % 1024.

If someone tries to load a microcode blob which has been split and so
on, then we should refuse loading. We want to accept microcode from the
vendor and nothing else glued together.

> I would hope so as well, but I am a bit more sceptical than you on this.

Well, if you spot a discrepancy where they diverge from the SDM, you
make sure you scream loudly.

> "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."
> 
> Reading a revision of zero really is supposed to mean "no update is present
> in the processor", and that's because it must be pre-loaded with a zero
> before cpuid is called.
> 
> IMHO, this mean that one should be really paranoid over any Intel microcode
> update that claims to have a revision of zero.  Intel wouldn't release such
> a microcode update except in error, and we can safely assume we want nothing
> to do with any such update attempts.

Ok, then please change the patch to reflect that - it is not "silicon
microcode" anymore but revision 0 is special and means no update was
done. Which is a proper way for the CPU to signal microcode update
status.

> Yeah, well, if you have CONFIG_X86_MSR enabled, all bets are off.  Thanks
> for reminding me about that one.

Yes, the only thing you need is the ability to execute *MSR insns in ring0.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

On Fri, 08 Aug 2014, Borislav Petkov wrote:
> If someone tries to load a microcode blob which has been split and so
> on, then we should refuse loading. We want to accept microcode from the
> vendor and nothing else glued together.

I don't quite get why we should refuse microcode that has been split by
userspace when the Intel SDM explicitly states that tools can do that if
there is a need, but hey, I consider this to be a very minor point now:

In these last two weeks I tried to look around for microcode loader
implementantions, and now I believe we will *never* see microcode with the
current version of the extended signatures specification.  The loaders in
the field are just too broken, Intel might as well come up with a new and
enhanced design that doesn't have so many sharp edges, since nearly everyone
will have to patch their loaders anyway.

I will respin the patch without the %1024 comment.  Note that I never
*removed* any test, we never tested for %1024 in the first place (nor does
anyone else with the possible exception of proprietary tools to write
microcode to FLASH or merge them into a BIOS image).

> > "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."
> > 
> > Reading a revision of zero really is supposed to mean "no update is
> > present in the processor", and that's because it must be pre-loaded
> > with a zero before cpuid is called.
> > 
> > IMHO, this mean that one should be really paranoid over any Intel
> > microcode update that claims to have a revision of zero.  Intel
> > wouldn't release such a microcode update except in error, and we can
> > safely assume we want nothing to do with any such update attempts.
> 
> Ok, then please change the patch to reflect that - it is not "silicon
> microcode" anymore but revision 0 is special and means no update was
> done. Which is a proper way for the CPU to signal microcode update
> status.

Will change the commit log in the respin.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Fri, Aug 08, 2014 at 10:50:57AM -0300, Henrique de Moraes Holschuh wrote:
> On Fri, 08 Aug 2014, Borislav Petkov wrote:
> > If someone tries to load a microcode blob which has been split and so
> > on, then we should refuse loading. We want to accept microcode from the
> > vendor and nothing else glued together.
> 
> I don't quite get why we should refuse microcode

Because the blob from the official location passes internal validation, I'd
strongly assume. Everything else doesn't.

> that has been split by userspace when the Intel SDM explicitly states
> that tools can do that if there is a need,

Where?

> In these last two weeks I tried to look around for microcode loader
> implementantions, and now I believe we will *never* see microcode with the
> current version of the extended signatures specification.  The loaders in
> the field are just too broken, Intel might as well come up with a new and
> enhanced design that doesn't have so many sharp edges, since nearly everyone
> will have to patch their loaders anyway.

You can't just assume that just because implementations are faulty there
- they should adhere to the SDM and it is authoritative. If the extended
signatures are really needed at some point, implementations will have to
be fixed.

> I will respin the patch without the %1024 comment.  Note that I never
> *removed* any test, we never tested for %1024 in the first place

And I'm saying we should if we're loading the official blob.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH 7/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

On Fri, 08 Aug 2014, Borislav Petkov wrote:
> On Fri, Aug 08, 2014 at 10:50:57AM -0300, Henrique de Moraes Holschuh wrote:
> > On Fri, 08 Aug 2014, Borislav Petkov wrote:
> > > If someone tries to load a microcode blob which has been split and so
> > > on, then we should refuse loading. We want to accept microcode from the
> > > vendor and nothing else glued together.
> > 
> > I don't quite get why we should refuse microcode
> 
> Because the blob from the official location passes internal validation, I'd
> strongly assume. Everything else doesn't.
> 
> > that has been split by userspace when the Intel SDM explicitly states
> > that tools can do that if there is a need,
> 
> Where?

Intel SDM vol 3A, last row of table 9-6, page 9-30:

    "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."

Not that there is any such utility in use as far as I know (and as I said,
iucode-tool doesn't do it either, and I won't add it unless it is absolutely
required).

But, as I said, I don't care about this one anymore because I don't believe
it will have any pratical effects...

> You can't just assume that just because implementations are faulty there
> - they should adhere to the SDM and it is authoritative. If the extended
> signatures are really needed at some point, implementations will have to
> be fixed.

... exactly because just about every implementation WILL have to be fixed,
which means someone @intel will show up around here with a patch.

> > I will respin the patch without the %1024 comment.  Note that I never
> > *removed* any test, we never tested for %1024 in the first place
> 
> And I'm saying we should if we're loading the official blob.

I will add a patch doing that.

-- 
  "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

[tip:x86/microcode] x86, microcode, amd: Fix missing static declaration

[tip-bot for Henrique de Moraes Holschuh]

Commit-ID:  1d2ce978d160fa960f12d06bf84e45f47c141272
Gitweb:     http://git.kernel.org/tip/1d2ce978d160fa960f12d06bf84e45f47c141272
Author:     Henrique de Moraes Holschuh <hmh@hmh.eng.br>
AuthorDate: Wed, 23 Jul 2014 17:10:44 -0300
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Thu, 24 Jul 2014 12:21:46 +0200

x86, microcode, amd: Fix missing static declaration

Make locally used variable static.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Link: http://lkml.kernel.org/r/1406146251-8540-1-git-send-email-hmh@hmh.eng.br
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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.

[tip:x86/microcode] x86, microcode, intel: Add missing static declarations

[tip-bot for Henrique de Moraes Holschuh]

Commit-ID:  05a5f76d033f413396bc48ce2f8651b5659bcd31
Gitweb:     http://git.kernel.org/tip/05a5f76d033f413396bc48ce2f8651b5659bcd31
Author:     Henrique de Moraes Holschuh <hmh@hmh.eng.br>
AuthorDate: Wed, 23 Jul 2014 17:10:45 -0300
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Thu, 24 Jul 2014 12:26:52 +0200

x86, microcode, intel: Add missing static declarations

gcc reports that a few declarations are missing.
Fix two obvious ones.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Link: http://lkml.kernel.org/r/1406146251-8540-1-git-send-email-hmh@hmh.eng.br
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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;

[tip:x86/microcode] x86, microcode, intel: Fix typos

[tip-bot for Henrique de Moraes Holschuh]

Commit-ID:  f99b45c3c2aa6960b8d21bb200d144be48a0a783
Gitweb:     http://git.kernel.org/tip/f99b45c3c2aa6960b8d21bb200d144be48a0a783
Author:     Henrique de Moraes Holschuh <hmh@hmh.eng.br>
AuthorDate: Wed, 23 Jul 2014 17:10:46 -0300
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Thu, 24 Jul 2014 12:32:49 +0200

x86, microcode, intel: Fix typos

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 <hmh@hmh.eng.br>
Link: http://lkml.kernel.org/r/1406146251-8540-1-git-send-email-hmh@hmh.eng.br
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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++)

[tip:x86/microcode] x86, microcode, intel: Rename apply_microcode and declare it static

[tip-bot for Henrique de Moraes Holschuh]

Commit-ID:  532ed3740c1ed1583ea3fa6de9410edf0d508563
Gitweb:     http://git.kernel.org/tip/532ed3740c1ed1583ea3fa6de9410edf0d508563
Author:     Henrique de Moraes Holschuh <hmh@hmh.eng.br>
AuthorDate: Thu, 24 Jul 2014 15:23:21 -0300
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Fri, 25 Jul 2014 17:57:51 +0200

x86, microcode, intel: Rename apply_microcode and declare it static

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.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Link: http://lkml.kernel.org/r/1406146251-8540-1-git-send-email-hmh@hmh.eng.br
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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,
 };
 

[tip:x86/microcode] x86, microcode, intel: Fix total_size computation

[tip-bot for Henrique de Moraes Holschuh]

Commit-ID:  ebc14ddcc9454c02439b67f6536628289faaa26e
Gitweb:     http://git.kernel.org/tip/ebc14ddcc9454c02439b67f6536628289faaa26e
Author:     Henrique de Moraes Holschuh <hmh@hmh.eng.br>
AuthorDate: Wed, 23 Jul 2014 17:10:49 -0300
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Mon, 28 Jul 2014 16:08:02 +0200

x86, microcode, intel: Fix total_size computation

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."

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.

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 <hmh@hmh.eng.br>
Link: http://lkml.kernel.org/r/1406146251-8540-1-git-send-email-hmh@hmh.eng.br
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 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 9067166..bbe296e 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)