linux-kernel - Re: [patch V3 18/30] x86/microcode: Handle "nosmt" correctly

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]

Date:   Fri, 22 Sep 2023 15:42:00 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Nikolay Borisov <nik.borisov@...e.com>
Subject: Re: [patch V3 18/30] x86/microcode: Handle "nosmt" correctly

Just textual nitpicks. Otherwise looks nice.

On Tue, Sep 12, 2023 at 09:58:12AM +0200, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> On CPUs where microcode loading is not NMI safe the SMT sibling which is
> parked in one of the play_dead() variants, these parked CPUs still react
> on NMIs.

s/, these parked CPUs still react/still reacts/.

Simpler.

> So if a NMI hits while the primary thread updates the microcode
> the resulting behaviour is undefined. The default play_dead()
> implementation on modern CPUs is using MWAIT, which is not guaranteed to
> be safe against an microcode update which affects MWAIT.

s/an //

> +/*
> + *  Ensure that all required CPUs which are present and have been booted
> + *  once are online.
> + *
> + *    To pass this check, all primary threads must be online.
> + *
> + *    If the microcode load is not safe against NMI then all SMT threads
> + *    must be online as well because they still react on NMI when they are

s/react on NMI/react to NMIs/

> + *    soft-offlined and parked in one of the play_dead() variants. So if a
> + *    NMI hits while the primary thread updates the microcode the resulting
> + *    behaviour is undefined. The default play_dead() implementation on
> + *    modern CPUs is using MWAIT, which is also not guaranteed to be safe

s/is using/uses/

> --- a/arch/x86/kernel/cpu/microcode/internal.h
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -20,18 +20,17 @@ enum ucode_state {
>  
>  struct microcode_ops {
>  	enum ucode_state (*request_microcode_fw)(int cpu, struct device *dev);
> -
>  	void (*microcode_fini_cpu)(int cpu);
>  
>  	/*
> -	 * The generic 'microcode_core' part guarantees that
> -	 * the callbacks below run on a target cpu when they
> -	 * are being called.
> +	 * The generic 'microcode_core' part guarantees that the callbacks
> +	 * below run on a target cpu when they are being called.

s/cpu/CPU/

while at it.

>  	 * See also the "Synchronization" section in microcode_core.c.
>  	 */
> -	enum ucode_state (*apply_microcode)(int cpu);
> -	int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> -	void (*finalize_late_load)(int result);
> +	enum ucode_state	(*apply_microcode)(int cpu);
> +	int			(*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> +	void			(*finalize_late_load)(int result);
> +	unsigned int		nmi_safe	: 1;
>  };

-- 
Regards/Gruss,
    Boris.

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