linux-kernel - Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200420093129.GA21839@zn.tnic>
Date:   Mon, 20 Apr 2020 11:31:29 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Evalds Iodzevics <evalds.iodzevics@...il.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        tglx@...utronix.de, Ben Hutchings <ben@...adent.org.uk>
Subject: Re: [PATCH] x86/microcode/intel: replace sync_core() with
 native_cpuid_reg(eax)

On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> sync_core() always jums past cpuid instruction on 32 bit machines
> because data structure boot_cpu_data are not populated so early in boot.

I'm guessing because boot_cpu_data.cpuid_level is not properly set and
very early code in head_32.S sets it to -1 temporarily until the highest
CPUID level has been detected (or not).

But the microcode loading happens *before* that.

I'd probably be interested how you trigger that but since the backport
got changed (see below) and yours is correcting it to the upstream
variant then perhaps maybe I don't care that much. :)

> It depends on commit 5dedade6dfa243c130b85d1e4daba6f027805033 for
> native_cpuid_reg(eax) definitions
> 
> This patch is for 4.4 but also should apply to 4.9
> 
> Signed-off-by: Evalds Iodzevics <evalds.iodzevics@...il.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 90343ba50485..92ce9c8a508b 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -60,7 +60,7 @@ static inline u32 intel_get_microcode_revision(void)
>  	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
>  
>  	/* As documented in the SDM: Do a CPUID 1 here */
> -	sync_core();
> +	native_cpuid_eax(1);
>  
>  	/* get the current revision from MSR 0x8B */
>  	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
> -- 

Hrm, the original patch of mine did use native_cpuid_eax():

4167709bbf82 ("x86/microcode/intel: Add a helper which gives the microcode revision")

but the backport:

commit 98cc1464cfd6edf9dc7fa96aaaf596aae952029b
Author: Borislav Petkov <bp@...e.de>
Date:   Mon Jan 9 12:41:45 2017 +0100

    x86/microcode/intel: Add a helper which gives the microcode revision
    
    commit 4167709bbf826512a52ebd6aafda2be104adaec9 upstream.
    
    Since on Intel we're required to do CPUID(1) first, before reading
    the microcode revision MSR, let's add a special helper which does the
    required steps so that we don't forget to do them next time, when we
    want to read the microcode revision.
    
    Signed-off-by: Borislav Petkov <bp@...e.de>
    Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@alien8.de
    Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
    [bwh: Backported to 4.4:
     - Don't touch prev_rev variable in apply_microcode()
     - Keep using sync_core(), which will alway includes the necessary CPUID
     	^^^^^^^^^^^^^^^^^^^

decided to use sync_core() for whatever reason. Perhaps because the
native_cpuid* things weren't there. Adding Ben to Cc.

I believe this is the background info Greg needed to figure out *why*
you're doing this.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette