Re: [PATCH][v2] x86/microcode/intel: check cpu stepping and processor flag before saving microcode

[Raj, Ashok]

Hi Boris

On Mon, Nov 16, 2020 at 01:27:35PM +0100, Borislav Petkov wrote:
> ( drop stable@ from Cc because this is not how fixes get added to stable@ )

Stable is still left below. with #v4.10+

Do you want to keep this? Also do you want him to resend or you have that
covered?

> 
> On Fri, Nov 13, 2020 at 09:59:23AM +0800, Chen Yu wrote:
> > Currently scan_microcode() leverages microcode_matches() to check if the
> > microcode matches the CPU by comparing the family and model. However before
> > saving the microcode in scan_microcode(), the processor stepping and flag
> > of the microcode signature should also be considered in order to avoid
> > incompatible update and caused the failure of microcode update.
> 
> This is going in the right direction but needs to take care of one
> more angle. I've extended your fix to the version below. Lemme know if
> something's not clear or still missing.
> 

Seems clear to me, and the commit log cleanup also makes sense.
I don't have a system myself,. Will wait for Chen Yu to confirm if it works
for him as well. 

> Thx.
> 
> ---
> From: Chen Yu <yu.c.chen@xxxxxxxxx>
> Date: Fri, 13 Nov 2020 09:59:23 +0800
> Subject: [PATCH] x86/microcode/intel: Check patch signature before saving microcode for early loading
> 
> Currently, scan_microcode() leverages microcode_matches() to check
> if the microcode matches the CPU by comparing the family and model.
> However, the processor stepping and flags of the microcode signature
> should also be considered when saving a microcode patch for early
> update.
> 
> Use find_matching_signature() in scan_microcode() and get rid of the
> now-unused microcode_matches() which is a good cleanup in itself.
> 
> Complete the verification of the patch being saved for early loading in
> save_microcode_patch() directly. This needs to be done there too because
> save_mc_for_early() will call save_microcode_patch() too.
> 
> The second reason why this needs to be done is because the loader still
> tries to support, at least hypothetically, mixed-steppings systems and
> thus adds all patches to the cache that belong to the same CPU model
> albeit with different steppings.
> 
> For example:
> 
>   microcode: CPU: sig=0x906ec, pf=0x2, rev=0xd6
>   microcode: mc_saved[0]: sig=0x906e9, pf=0x2a, rev=0xd6, total size=0x19400, date = 2020-04-23
>   microcode: mc_saved[1]: sig=0x906ea, pf=0x22, rev=0xd6, total size=0x19000, date = 2020-04-27
>   microcode: mc_saved[2]: sig=0x906eb, pf=0x2, rev=0xd6, total size=0x19400, date = 2020-04-23
>   microcode: mc_saved[3]: sig=0x906ec, pf=0x22, rev=0xd6, total size=0x19000, date = 2020-04-27
>   microcode: mc_saved[4]: sig=0x906ed, pf=0x22, rev=0xd6, total size=0x19400, date = 2020-04-23
> 
> The patch which is being saved for early loading, however, can only be
> the one which fits the CPU this runs on so do the signature verification
> before saving.
> 
>  [ bp: Do signature verification in save_microcode_patch()
>        and rewrite commit message. ]
> 
> Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
> Signed-off-by: Chen Yu <yu.c.chen@xxxxxxxxx>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx #v4.10+
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=208535
> Link: https://lkml.kernel.org/r/20201113015923.13960-1-yu.c.chen@xxxxxxxxx
> ---
>  arch/x86/kernel/cpu/microcode/intel.c | 63 +++++----------------------
>  1 file changed, 10 insertions(+), 53 deletions(-)