linux-kernel - Re: [PATCH] x86/fpu: Fix irq_fpu_usable() to return false during CPU onlining

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: <aCrrMEN01O7FWY6V@gmail.com>
Date: Mon, 19 May 2025 10:26:24 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
	linux-crypto@...r.kernel.org, linux-pm@...r.kernel.org,
	Borislav Petkov <bp@...en8.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ayush Jain <Ayush.Jain3@....com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH] x86/fpu: Fix irq_fpu_usable() to return false during CPU
 onlining


* Eric Biggers <ebiggers@...nel.org> wrote:

> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -42,12 +42,15 @@ struct fpu_state_config fpu_user_cfg __ro_after_init;
>   * Represents the initial FPU state. It's mostly (but not completely) zeroes,
>   * depending on the FPU hardware format:
>   */
>  struct fpstate init_fpstate __ro_after_init;
>  
> -/* Track in-kernel FPU usage */
> -static DEFINE_PER_CPU(bool, in_kernel_fpu);
> +/*
> + * Track FPU initialization and kernel-mode usage. 'true' means the FPU is
> + * initialized and is not currently being used by the kernel:
> + */
> +DEFINE_PER_CPU(bool, kernel_fpu_allowed);

So this is a nice independent cleanup, regardless of the CPU 
bootstrapping bug it fixes. The fuzzy/negated meaning of in_kernel_fpu 
always bothered me a bit, and your patch makes this condition a bit 
cleaner, plus it defaults to 'disabled' on zero-initialization, which 
is a bonus.

>  void kernel_fpu_end(void)
>  {
> -	WARN_ON_FPU(!this_cpu_read(in_kernel_fpu));
> +	/* Toggle kernel_fpu_allowed back to true: */
> +	WARN_ON_FPU(this_cpu_read(kernel_fpu_allowed));
> +	this_cpu_write(kernel_fpu_allowed, true);
>  
> -	this_cpu_write(in_kernel_fpu, false);
>  	if (!irqs_disabled())
>  		fpregs_unlock();

In addition to this fix, feel free to also send your x86 irqs-enabled 
FPU model optimization series on top, Ard says it shouldn't cause 
fundamental problems on EFI.

>  }
>  EXPORT_SYMBOL_GPL(kernel_fpu_end);
>  
> diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
> index 6bb3e35c40e24..99db41bf9fa6b 100644
> --- a/arch/x86/kernel/fpu/init.c
> +++ b/arch/x86/kernel/fpu/init.c
> @@ -49,10 +49,13 @@ static void fpu__init_cpu_generic(void)
>   */
>  void fpu__init_cpu(void)
>  {
>  	fpu__init_cpu_generic();
>  	fpu__init_cpu_xstate();
> +
> +	/* Start allowing kernel-mode FPU: */
> +	this_cpu_write(kernel_fpu_allowed, true);

Since this goes outside the regular kernel_fpu_begin()/end() methods, 
could you please also add an WARN_ON_FPU() check to make sure it was 
false before? x86 CPU init code is still a bit of spaghetti at times.

> @@ -1186,10 +1186,16 @@ void cpu_disable_common(void)
>  {
>  	int cpu = smp_processor_id();
>  
>  	remove_siblinginfo(cpu);
>  
> +	/*
> +	 * Stop allowing kernel-mode FPU. This is needed so that if the CPU is
> +	 * brought online again, the initial state is not allowed:
> +	 */
> +	this_cpu_write(kernel_fpu_allowed, false);

Ditto, an WARN_ON_FPU() would be nice: if kernel FPU is disabled at 
this point then something's fishy.

Thanks,

	Ingo