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

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

[Konrad Rzeszutek Wilk]

On Fri, Dec 06, 2013 at 01:04:02PM -0800, Andi Kleen wrote:
> 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.

Why not just update the struct?
> 
> 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

[Andi Kleen]

On Tue, Dec 10, 2013 at 10:30:00AM -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Dec 06, 2013 at 01:04:02PM -0800, Andi Kleen wrote:
> > 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.
> 
> Why not just update the struct?

It would need updating various printks I think. So I chose the simpler cast,
as that already solves the comparison problem.

-Andi

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

[H. Peter Anvin]

On 12/10/2013 07:55 AM, Andi Kleen wrote:
> On Tue, Dec 10, 2013 at 10:30:00AM -0500, Konrad Rzeszutek Wilk wrote:
>> On Fri, Dec 06, 2013 at 01:04:02PM -0800, Andi Kleen wrote:
>>> 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.
>>
>> Why not just update the struct?
> 
> It would need updating various printks I think. So I chose the simpler cast,
> as that already solves the comparison problem.
> 
> -Andi
> 

Updating the printks sounds like the right thing, too.

	-hpa

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