linux-kernel - Re: [PATCH] x86/microcode/intel: Allow late loading only if a min rev is specified

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: <Yw1AtFrRPWhkJ4r8@araj-dh-work>
Date:   Mon, 29 Aug 2022 22:41:56 +0000
From:   Ashok Raj <ashok.raj@...el.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Thomas Gleixner <tglx@...utronix.de>,
        LKML Mailing List <linux-kernel@...r.kernel.org>,
        X86-kernel <x86@...nel.org>, Dave Hansen <dave.hansen@...el.com>,
        Andy Lutomirski <luto@...capital.net>,
        Ingo Molnar <mingo@...nel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Tony Luck <tony.luck@...el.com>,
        Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH] x86/microcode/intel: Allow late loading only if a min
 rev is specified

On Mon, Aug 29, 2022 at 10:31:22PM +0200, Borislav Petkov wrote:
> On Mon, Aug 29, 2022 at 06:04:36PM +0000, Ashok Raj wrote:
> > @@ -886,7 +905,7 @@ static bool is_blacklisted(unsigned int cpu)
> >  }
> >  
> >  static enum ucode_state request_microcode_fw(int cpu, struct device *device,
> > -					     bool refresh_fw)
> > +					     bool late_loading)
> >  {
> 
> Until the refresh_fw's function hasn't been clarified:
> 
> https://lore.kernel.org/all/YwaBim3Xt3Il3KXm@zn.tnic/
> 

I don't know exactly what you mean. 

But I suppose, you mean what refresh_hw is supposed to mean from the existing code?

refresh_hw seems to imply when to update  the copy of the microcode from
the filesystem. Also seems to imply late loading.

its used in the following places.

1. During reload_store() where this is exclicitly due to echo 1 > reload

	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);

	Here passing true makes sense since you are going to do a full
	refresh on all CPUs via late loading.

2. microcode_update_cpu() -> microcode_init_cpu()->request_microcode_fw(false)

   Early loading from resume. So we would use the microcode cache to load
   from.

3. mc_device_add() -> microcode_init_cpu(true)->request_microcode_fw(true)

   This seems like normal CPU hot-add, I'm not sure if refresh_fw=true is
   valid. A new CPU should also use from the cache, but not a full reload
   from filesystem. This could end up with new cpu with an updated ucode
   and older with something that was loaded earlier. Sort of what was fixed
   in:

   commit 7189b3c11903667808029ec9766a6e96de5012a5 (tag: x86_microcode_for_v5.13)

   intel.c doesn't seem to use this parameter at all today. But judging from

   amd.c: request_microcode_amd() 

        /* reload ucode container only on the boot cpu */
        if (!refresh_fw || !bsp)
                return UCODE_OK;

  Seems like it does the right job, since you would refresh only if
  refresh_fw=true and its the bsp. Hence it seems immute to the
  mc_device_add() bug.

  1. Fix mc_device_add() to set refresh_fw() to false.
  2. We can change the parameter to late_loading as you had alluded in
     previous post.
  3. Use that to distinguish if we need to enforce the
     microcode_sanity_check() to validate min_rev. 

Let me know if this is what you meant.

Cheers,
Ashok