linux-kernel - Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during microcode update

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 for Android: free password hash cracker in your pocket

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

Message-ID: <YvzTsxilM1kUGeFQ@araj-dh-work>
Date:   Wed, 17 Aug 2022 11:40:35 +0000
From:   Ashok Raj <ashok.raj@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
CC:     Borislav Petkov <bp@...en8.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tony Luck <tony.luck@...el.com>,
        Dave Hansen <dave.hansen@...el.com>,
        "LKML Mailing List" <linux-kernel@...r.kernel.org>,
        X86-kernel <x86@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Tom Lendacky <thomas.lendacky@....com>,
        Jacon Jun Pan <jacob.jun.pan@...el.com>,
        Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH v3 3/5] x86/microcode: Avoid any chance of MCE's during
 microcode update

On Wed, Aug 17, 2022 at 09:41:31AM +0200, Ingo Molnar wrote:
> 
> > +void mce_set_mcip(void)
> > +{
> > +	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x1);
> > +}
> > +
> > +void mce_clear_mcip(void)
> > +{
> > +	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x0);
> > +}
> 
> Instead of naming new APIs after how they are doing stuff, please name them 
> after *what* they are doing at the highest level: they disable/enable MCEs.
> 
> Ie. I'd suggest something like:
> 
>      mce_disable()
>      mce_enable()

We actually aren't disabling MCE's we set things up to promote it to a more
severe shutdown if an MCE were to be signaled while in the ucode update
flow.

I'm struggling to find a suitable name. But I agree with what you are
saying.

promote_mce_to_fatal()? I'll take any names that seem fit.

> 
> I'd also suggest to at minimum add a WARN_ON_ONCE() if MSR_IA32_MCG_STATUS 
> is already 1 when we disable it - because whoever wanted it disabled will 
> now be surprised by us enabling them again.

Ok, will add.

> 
> > +	/*
> > +	 * Its dangerous to let MCE while microcode update is in progress.
> 
> s/let MCE while
>  /let MCEs execute while
> 
> > +	 * Its extremely rare and even if happens they are fatal errors.
> > +	 * But reading patched areas before the update is complete can be
> > +	 * leading to unpredictable results. Setting MCIP will guarantee
> 
> s/can be leading to
>  /can lead to
> 
> > +	 * the platform is taken to reset predictively.
> 
> What does 'the platform is taken to reset predictively' mean?

Since we are setting MCG_STATUS.MCIP=1, since MCE's aren't nestable, 
if there is a hardware event trying to signal a MCE, it will turn into a
platform reset. The MCE registers that logged the event will be sticky
and preserve in a warm reset case. BIOS or OS can pickup values after the
reboot is complete.

> 
> Did you mean 'predictibly'/'reliably'?

I don't know the difference, except both are a trustworthy topic :-)

I like predictable, the system is going down.. not reliable :-)
> 
> > +	 */
> > +	mce_set_mcip();
> >  	/*
> >  	 * On an SMT system, it suffices to load the microcode on one sibling of
> >  	 * the core because the microcode engine is shared between the threads.
> > @@ -457,6 +466,7 @@ static int __reload_late(void *info)
> >  	 * loading attempts happen on multiple threads of an SMT core. See
> >  	 * below.
> >  	 */
> > +
> >  	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
> >  		apply_microcode_local(&err);
> >  	else
> 
> Spurious newline added?

It's a gonner !

Cheers,
Ashok