[PATCH 1/2] x86, microcode: Do Intel microcode revision check signed v2

[PATCH 1/2] x86, microcode: Do Intel microcode revision check signed v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The Intel SDM Vol 3 9.11.1 Microcode update states that
the update revision field is signed. However we do the comparison
unsigned, as the comparison gets promoted. Change the field
to be signed, so that comparision is really signed.

v2: Change field.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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..ed1884b 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -5,7 +5,7 @@
 
 struct microcode_header_intel {
 	unsigned int            hdrver;
-	unsigned int            rev;
+	int	                rev;
 	unsigned int            date;
 	unsigned int            sig;
 	unsigned int            cksum;
-- 
1.8.3.1

[PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

For testing purposes it can be useful to downgrade microcode.
Normally the driver only allows upgrading.

Add a module_param (default off) that allows downgrading.

Note the module_param can currently not be set for early
ucode update, only for late.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/microcode_intel_lib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/microcode_intel_lib.c b/arch/x86/kernel/microcode_intel_lib.c
index ce69320..18d5325 100644
--- a/arch/x86/kernel/microcode_intel_lib.c
+++ b/arch/x86/kernel/microcode_intel_lib.c
@@ -26,11 +26,16 @@
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 
 #include <asm/microcode_intel.h>
 #include <asm/processor.h>
 #include <asm/msr.h>
 
+static bool allow_downgrade;
+module_param(allow_downgrade, bool, 0644);
+MODULE_PARM_DESC(allow_downgrade, "Allow downgrading microcode");
+
 static inline int
 update_match_cpu(unsigned int csig, unsigned int cpf,
 		 unsigned int sig, unsigned int pf)
@@ -41,6 +46,8 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
 int
 update_match_revision(struct microcode_header_intel *mc_header, int rev)
 {
+	if (allow_downgrade)
+		return 1;
 	return (mc_header->rev <= rev) ? 0 : 1;
 }
 
-- 
1.8.3.1

Re: [PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Henrique de Moraes Holschuh]

On Fri, 24 Jan 2014, Andi Kleen wrote:
> For testing purposes it can be useful to downgrade microcode.
> Normally the driver only allows upgrading.

The code is not prepared to work correctly when downgrading is allowed, in
the presence of shadowed microcode.  When a firmware request results in more
than one microcode for the same cpuid, with overlapping pf_mask, the code
*depends* on the "never downgrade" logic to work.

Shadowed microcode *is* currently distributed by Intel, and as an artifact
of the f-m-s grouping, it is *guaranteed* to trigger the issue.  When the
issue triggers, what microcode will be selected to be uploaded to the core
depends *only* on the order of the microcodes in the firmware file.

I see no documentation of this fact anywhere, and it is *anything* but
obvious.

That extremely obnoxious Intel microcode license forbids anyone to fix the
pf_mask metadata fields to remove shadowing, so we ship that stuff as-is to
in the distros.  It *will* hit users.

Also, since you're going to mess with this, why don't you implement the
correct semanthics for microcode with the sign bit set?  Making it signed
actually makes the current code behaviour worse.

Refer to: http://lkml.org/lkml/2011/3/21/522

Note that I was wrong about negative revision microcode not being found in
the wild.  Intel has shipped entry-level server boards with pre-release
microcode several times, even on BIOS updates, and you're likely to get
access to such pre-release microcode if you're dealing with Intel firmware
partners that has full access to microcode updates.

-- 
  "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 2/2] x86, microcode: Add option to allow downgrading of microcode

[Andi Kleen]

On Sat, Jan 25, 2014 at 02:35:58PM -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 24 Jan 2014, Andi Kleen wrote:
> > For testing purposes it can be useful to downgrade microcode.
> > Normally the driver only allows upgrading.
> 
> The code is not prepared to work correctly when downgrading is allowed, in
> the presence of shadowed microcode.  When a firmware request results in more

As I wrote it's only for testing purposes when you know what you're doing
(typically with a special micro code file)

Your whole argument is irrelevant, as it only applies to normal users
who should never use this option.

> Also, since you're going to mess with this, why don't you implement the
> correct semanthics for microcode with the sign bit set?  Making it signed
> actually makes the current code behaviour worse.
> 
> Refer to: http://lkml.org/lkml/2011/3/21/522

I don't think it makes it worse. In fact I'm essentially implementing
Burt's request "for explicit user action" with the new override option.

Anyways I suppose your rant killed the patch anyways. Congratulations!

-Andi

Re: [PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Henrique de Moraes Holschuh]

On Sat, 25 Jan 2014, Andi Kleen wrote:
> On Sat, Jan 25, 2014 at 02:35:58PM -0200, Henrique de Moraes Holschuh wrote:
> > On Fri, 24 Jan 2014, Andi Kleen wrote:
> > > For testing purposes it can be useful to downgrade microcode.
> > > Normally the driver only allows upgrading.
> > 
> > The code is not prepared to work correctly when downgrading is allowed, in
> > the presence of shadowed microcode.  When a firmware request results in more
> 
> As I wrote it's only for testing purposes when you know what you're doing
> (typically with a special micro code file)
> 
> Your whole argument is irrelevant, as it only applies to normal users
> who should never use this option.

It certainly could become a lot less relevant if any indication is given to
normal users that they should never enable the feature unless they really
know what they're doing.

It is NOT "only for testing purposes" when that fact is written nowhere an
user would see.

> > Also, since you're going to mess with this, why don't you implement the
> > correct semanthics for microcode with the sign bit set?  Making it signed
> > actually makes the current code behaviour worse.
> > 
> > Refer to: http://lkml.org/lkml/2011/3/21/522
> 
> I don't think it makes it worse. In fact I'm essentially implementing
> Burt's request "for explicit user action" with the new override option.

You're correct.  It doesn't make it worse, it makes it better.  Your change
effectively forbids loading pre-release microcode on a box which has
production microcode, unless "downgrade mode" is enabled.

This is different from what Burt requested, but if you're implementing a
downgrade mode, it is certainly a much better behavior.

> Anyways I suppose your rant killed the patch anyways. Congratulations!

You decided to ignore feedback you got weeks ago that required just adding a
comment to the code or to the commit log to address.  Whether or not you
consider said feedback particularly interesting or relevant does NOT give
you the right to just handwave it away only to later blame others should the
patch not get accepted.

-- 
  "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 1/2] x86, microcode: Do Intel microcode revision check signed v2

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The Intel SDM Vol 3 9.11.1 Microcode update states that
the update revision field is signed. However we do the comparison
unsigned, as the comparison gets promoted. Change the field
to be signed, so that comparision is really signed.

v2: Change field.
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 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..ed1884b 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -5,7 +5,7 @@
 
 struct microcode_header_intel {
 	unsigned int            hdrver;
-	unsigned int            rev;
+	int	                rev;
 	unsigned int            date;
 	unsigned int            sig;
 	unsigned int            cksum;
-- 
1.8.3.1

[PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

For testing purposes it can be useful to downgrade microcode.
Normally the driver only allows upgrading.

Add a module_param (default off) that allows downgrading.

Note the module_param can currently not be set for early
ucode update, only for late.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/microcode_intel_lib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/microcode_intel_lib.c b/arch/x86/kernel/microcode_intel_lib.c
index ce69320..18d5325 100644
--- a/arch/x86/kernel/microcode_intel_lib.c
+++ b/arch/x86/kernel/microcode_intel_lib.c
@@ -26,11 +26,16 @@
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 
 #include <asm/microcode_intel.h>
 #include <asm/processor.h>
 #include <asm/msr.h>
 
+static bool allow_downgrade;
+module_param(allow_downgrade, bool, 0644);
+MODULE_PARM_DESC(allow_downgrade, "Allow downgrading microcode");
+
 static inline int
 update_match_cpu(unsigned int csig, unsigned int cpf,
 		 unsigned int sig, unsigned int pf)
@@ -41,6 +46,8 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
 int
 update_match_revision(struct microcode_header_intel *mc_header, int rev)
 {
+	if (allow_downgrade)
+		return 1;
 	return (mc_header->rev <= rev) ? 0 : 1;
 }
 
-- 
1.8.3.1

[PATCH 1/2] x86, microcode: Do Intel microcode revision check signed

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The Intel SDM Vol 3 9.11.1 Microcode update states that
the update revision field is signed. However we do the comparison
unsigned, as the comparison gets promoted.

Use a cast to really do a signed comparison of the microcode
revision.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/microcode_intel_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/microcode_intel_lib.c b/arch/x86/kernel/microcode_intel_lib.c
index ce69320..68503d1 100644
--- a/arch/x86/kernel/microcode_intel_lib.c
+++ b/arch/x86/kernel/microcode_intel_lib.c
@@ -41,7 +41,7 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
 int
 update_match_revision(struct microcode_header_intel *mc_header, int rev)
 {
-	return (mc_header->rev <= rev) ? 0 : 1;
+	return ((int)mc_header->rev <= rev) ? 0 : 1;
 }
 
 int microcode_sanity_check(void *mc, int print_err)
-- 
1.8.3.1

[PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

For testing purposes it can be useful to downgrade microcode.
Normally the driver only allows upgrading.

Add a module_param (default off) that allows downgrading.

Note the module_param can currently not be set for early
ucode update, only for late.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/microcode_intel_lib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/microcode_intel_lib.c b/arch/x86/kernel/microcode_intel_lib.c
index 68503d1..17d63f3 100644
--- a/arch/x86/kernel/microcode_intel_lib.c
+++ b/arch/x86/kernel/microcode_intel_lib.c
@@ -26,11 +26,16 @@
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 
 #include <asm/microcode_intel.h>
 #include <asm/processor.h>
 #include <asm/msr.h>
 
+static bool allow_downgrade;
+module_param(allow_downgrade, bool, 0644);
+MODULE_PARM_DESC(allow_downgrade, "Allow downgrading microcode");
+
 static inline int
 update_match_cpu(unsigned int csig, unsigned int cpf,
 		 unsigned int sig, unsigned int pf)
@@ -41,6 +46,8 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
 int
 update_match_revision(struct microcode_header_intel *mc_header, int rev)
 {
+	if (allow_downgrade)
+		return 1;
 	return ((int)mc_header->rev <= rev) ? 0 : 1;
 }
 
-- 
1.8.3.1

Re: [PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Henrique de Moraes Holschuh]

On Fri, 06 Dec 2013, Andi Kleen wrote:
> For testing purposes it can be useful to downgrade microcode.
> Normally the driver only allows upgrading.

...

>  int
>  update_match_revision(struct microcode_header_intel *mc_header, int rev)
>  {
> +	if (allow_downgrade)
> +		return 1;
>  	return ((int)mc_header->rev <= rev) ? 0 : 1;
>  }

Two points maybe worth some thinking about:

1. We might want to forbid downgrading to version 0 of a microcode.  Version
zero is supposed to mean "running factory-provided microcode" and there is
absolutely no way an attempt to downgrade to version 0 is not an attack of
some sort.

And we know for a fact that is a very unwise idea to let strange microcode
get past the kernel driver and into the microcode update microcode itself.

2. This change has unintended side-effects that ought to be at least
documented:

In "allow downgrade" mode, should you send a microcode pack with several
microcodes to the kernel, and more than one of them might apply to the
running processor (when the pf_flags of two or more of the microcodes are
not disjoint), either the first or the last (I didn't check) will be the
one chosen.

I've seen in the wild microcodes where both rev X and Y (when Y > X) could
be applied to a specific processor, but rev X was intended to more
processors (more bits set in pf_mask) than rev. Y.  I don't think they were
in the same Intel microcode update datafile, but it can happen.  In normal
upgrade-only mode this works correctly, while in your proposed downgrade
mode, it won't.

The upgrade/downgrade rules for negative release microcodes are different,
it might be a good idea to ask someone at Intel to clarify these rules
(including whether release X or Y should be installed when X < Y < 0).  So
far, I've only heard of negative release numbers in the microcodes shipped
inside the BIOS/EFI images of some server boards and embedded products, and
I was told they're reserved for "internal Intel development microcode" or
something like 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

Re: [PATCH 2/2] x86, microcode: Add option to allow downgrading of microcode

[Henrique de Moraes Holschuh]

On Fri, 13 Dec 2013, Henrique de Moraes Holschuh wrote:
> 2. This change has unintended side-effects that ought to be at least
> documented:
> 
> In "allow downgrade" mode, should you send a microcode pack with several
> microcodes to the kernel, and more than one of them might apply to the
> running processor (when the pf_flags of two or more of the microcodes are
> not disjoint), either the first or the last (I didn't check) will be the
> one chosen.
> 
> I've seen in the wild microcodes where both rev X and Y (when Y > X) could
> be applied to a specific processor, but rev X was intended to more
> processors (more bits set in pf_mask) than rev. Y.  I don't think they were
> in the same Intel microcode update datafile, but it can happen.  In normal
> upgrade-only mode this works correctly, while in your proposed downgrade
> mode, it won't.

Well, tracked it down to:

/lib/firmware/intel-ucode/0f-04-0a
  001: sig 0x00000f4a, pf mask 0x5c, 2005-12-14, rev 0x0004, size 2048
  002: sig 0x00000f4a, pf mask 0x5d, 2005-06-10, rev 0x0002, size 2048

/lib/firmware/intel-ucode/0f-04-08
  001: sig 0x00000f48, pf mask 0x5f, 2005-06-30, rev 0x0007, size 3072
  002: sig 0x00000f48, pf mask 0x01, 2006-05-08, rev 0x000c, size 3072
  003: sig 0x00000f48, pf mask 0x02, 2008-01-15, rev 0x000e, size 3072

And these are microcodes still being distributed in the latest Intel
microcode bundle (2013-09-06).  They're for old processors, but still...  if
it happened once and it is still being distributed like that, it can happen
again.

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