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: <7fb8e9ac69a1c03a673875dc39fcd8f94407abca.camel@decadent.org.uk>
Date:   Mon, 20 Apr 2020 14:33:30 +0100
From:   Ben Hutchings <ben@...adent.org.uk>
To:     Borislav Petkov <bp@...en8.de>,
        Evalds Iodzevics <evalds.iodzevics@...il.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        tglx@...utronix.de
Subject: Re: [PATCH] x86/microcode/intel: replace sync_core() with
 native_cpuid_reg(eax)

On Mon, 2020-04-20 at 11:31 +0200, Borislav Petkov wrote:
> 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.
[...]
> 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.

This commit didn't introduce the call to native_cpuid_eax(), but only
moved it.  So I didn't think it made sense for the backport to change
from sync_core().

If it's important to use native_cpuid_eax() then these older branches
should presumably get backports of:

484d0e5c7943 x86/microcode/intel: Replace sync_core() with native_cpuid()
f3e2a51f568d x86/microcode: Use native CPUID to tickle out microcode revision

Ben.

> I believe this is the background info Greg needed to figure out *why*
> you're doing this.
> 
-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                               A fail-safe circuit will destroy others.


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)