Re: [PATCH] x86/microcode: Check if any new features are present after a microcode reload.

[Borislav Petkov]

On Thu, Feb 08, 2018 at 10:53:06AM -0800, Ashok Raj wrote:
> When microcode is loaded later after system boot, new microcode could enumerate
> new features. It might not be possible to use these new features unless they are
> loaded during early boot via initrd or from the BIOS in some cases.
> 
> This patch attempts to simply check if there are any differences in feature set
> and warns user to use early microcode load before using the new features.
> 
> Suggested-by: Andi Kleen <andi.kleen@xxxxxxxxx>
> Signed-off-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: David Woodhouse <dwmw@xxxxxxxxxxxx>
> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> Cc: Tony Luck <tony.luck@xxxxxxxxx>
> Cc: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index f7c55b0..0e07035 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -39,6 +39,7 @@
>  #include <asm/tlbflush.h>
>  #include <asm/setup.h>
>  #include <asm/msr.h>
> +#include "../kernel/cpu/cpu.h" /* get_cpu_cap() */
>  
>  static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
>  
> @@ -776,7 +777,7 @@ static int apply_microcode_intel(int cpu)
>  {
>  	struct microcode_intel *mc;
>  	struct ucode_cpu_info *uci;
> -	struct cpuinfo_x86 *c;
> +	struct cpuinfo_x86 *c, new_cpuinfo;
>  	static int prev_rev;
>  	u32 rev;
>  
> @@ -804,17 +805,23 @@ static int apply_microcode_intel(int cpu)
>  		return -1;
>  	}
>  
> +	c = &cpu_data(cpu);
>  	if (rev != prev_rev) {
>  		pr_info("updated to revision 0x%x, date = %04x-%02x-%02x\n",
>  			rev,
>  			mc->hdr.date & 0xffff,
>  			mc->hdr.date >> 24,
>  			(mc->hdr.date >> 16) & 0xff);
> +		memset(&new_cpuinfo, 0, sizeof (struct cpuinfo_x86));
> +		get_cpu_cap(&new_cpuinfo);
> +		if (memcmp(&c->x86_capability, &new_cpuinfo.x86_capability,
> +			sizeof(new_cpuinfo.x86_capability))) {
> +				pr_warn_once("New features found in loaded microcode, but will be ignored\n");
> +				pr_warn_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
> +		}

A bunch of notes about this:

1. this checking happens per-CPU. Instead, it should happen per CPU
until the first CPU which encounters a CPUID bit change, prints the
warning and then doesn't do it anymore. I'm going to also heavily
assume this will happen immediately on the BSP and we simply forget all
heterogeneous BSP/AP configurations which are simply bollocks.

2. the part until memcmp() needs to be a generic function in
cpu/common.c or so which gets called by the microcode loader. This will
facilitate doing 1. too.

3. (2) needs to happen in the generic microcode driver -
arch/x86/kernel/cpu/microcode/core.c - not in intel.c For that to
happen, you need a prepatch which changes the ->apply_microcode()
functions to return a new value UCODE_UPDATED when the late update
succeeds in the vendor driver.

In the UCODE_UPDATED case, you call the common.c function which checks
whether it has ran already and if not, gets caps, checks and prints. If
it has, you don't need to do anything.

And those three changes sound like three nicely separated patches to me.

Anyway, something along those lines...

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.