linux-kernel - Re: [PATCH 3/3] x86/fpu: Don't support kernel-mode FPU when irqs

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: <20250517183919.GC1239@sol>
Date: Sat, 17 May 2025 11:39:19 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Ingo Molnar <mingo@...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 3/3] x86/fpu: Don't support kernel-mode FPU when
 irqs_disabled()

On Sat, May 17, 2025 at 09:09:01AM +0200, Ingo Molnar wrote:
> 
> * Eric Biggers <ebiggers@...nel.org> wrote:
> 
> > From: Eric Biggers <ebiggers@...gle.com>
> > 
> > Make irq_fpu_usable() return false when irqs_disabled().  That makes the
> > irqs_disabled() checks in kernel_fpu_begin_mask() and kernel_fpu_end()
> > unnecessary, so also remove those.
> > 
> > Rationale:
> > 
> > - There's no known use case for kernel-mode FPU when irqs_disabled().
> 
> Except EFI?

Yes, I remembered that just after sending this...  And EFI does want the ldmxcsr
and fninit, which makes it like actual kernel-mode FPU.  That implies we at
least need to disable BH (and preemption) if it wasn't already disabled.  But if
hardirqs may or may not be disabled already, that means we either need to
conditionally use local_bh_disable()/enable (or preempt_enable()/disable on
PREEMPT_RT) as the current code does, or use local_irq_save()/restore.

If we did the latter, then all EFI calls would run with hardirqs disabled.  It
looks like hardirqs are currently intentionally disabled before some of the EFI
calls, but not all of them.  I'm not sure what the logic is there, and whether
it would be okay to just always disable them.

> 
> >   arm64 and riscv already disallow kernel-mode FPU when irqs_disabled().
> >   __save_processor_state() previously did expect kernel_fpu_begin() and
> >   kernel_fpu_end() to work when irqs_disabled(), but this was a
> >   different use case and not actual kernel-mode FPU use.
> > 
> > - This is more efficient, since one call to irqs_disabled() replaces two
> >   irqs_disabled() and one in_hardirq().
> 
> This is noise compared to the overhead of saving/restoring vector CPU 
> context ...

In practice most calls to kernel_fpu_begin() don't actually do the
save_fpregs_to_fpstate(), since either TIF_NEED_FPU_LOAD is already set or it's
a kthread.  So, the overhead from the other parts like the EFLAGS checks and
ldmxcsr are measurable, especially when processing small amounts of data.

> > - This fixes irq_fpu_usable() to correctly return false during CPU
> >   initialization.  Incorrectly returning true caused the SHA-256 library
> >   code, which is called when loading AMD microcode, to take a
> >   SIMD-optimized code path too early, causing a crash.  By correctly
> >   returning false from irq_fpu_usable(), the generic SHA-256 code
> >   correctly gets used instead.  (Note: SIMD-optimized SHA-256 doesn't
> >   get enabled until subsys_initcall, but CPU hotplug can happen later.)
> 
> Alternatively we could set in_kernel_fpu during CPU bootstrap, and 
> clear it once we know the FPU is usable? This is only a relatively 
> short early boot period, with no scheduling, right?

Yes, if there isn't agreement on this approach we can do that instead.  Say:

- Replace in_kernel_fpu with kernel_fpu_supported, with the opposite meaning
  (so that the initial value of false means "unsupported")
- fpu__init_cpu() sets it to true
- cpu_disable_common() sets it to false