linux-kernel - Re: [PATCH 0/5] arm64: Move kernel mode FPSIMD buffer to the stack

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] [day] [month] [year] [list]

Message-ID: <CAMj1kXFtnDj-kDMPKfbj2hJ9FqVt17EstNSHbzfcmAAYZneS6g@mail.gmail.com>
Date: Sat, 20 Sep 2025 15:20:33 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Kees Cook <kees@...nel.org>
Cc: Eric Biggers <ebiggers@...nel.org>, Ard Biesheuvel <ardb+git@...gle.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org, 
	linux-kernel@...r.kernel.org, herbert@...dor.apana.org.au, 
	Marc Zyngier <maz@...nel.org>, Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Catalin Marinas <catalin.marinas@....com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH 0/5] arm64: Move kernel mode FPSIMD buffer to the stack

On Sat, 20 Sept 2025 at 08:42, Kees Cook <kees@...nel.org> wrote:
>
> On Fri, Sep 19, 2025 at 02:32:21PM -0500, Eric Biggers wrote:
> > On Thu, Sep 18, 2025 at 08:35:40AM +0200, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@...nel.org>
> > >
> > > Move the buffer for preserving/restoring the kernel mode FPSIMD state on a
> > > context switch out of struct thread_struct, and onto the stack, so that
> > > the memory cost is not imposed needlessly on all tasks in the system.
> > >
> > > Patches #1 - #3 contains some prepwork so that patch #4 can tighten the
> > > rules around permitted usage patterns of kernel_neon_begin() and
> > > kernel_neon_end(). This permits #5 to provide a stack buffer to
> > > kernel_neon_begin() transparently, in a manner that ensures that it will
> > > remain available until after the associated call to kernel_neon_end()
> > > returns.
> > >
> > > Cc: Marc Zyngier <maz@...nel.org>
> > > Cc: Will Deacon <will@...nel.org>
> > > Cc: Mark Rutland <mark.rutland@....com>
> > > Cc: Kees Cook <keescook@...omium.org>
> > > Cc: Catalin Marinas <catalin.marinas@....com>
> > > Cc: Mark Brown <broonie@...nel.org>
> > >
> > > Ard Biesheuvel (5):
> > >   crypto/arm64: aes-ce-ccm - Avoid pointless yield of the NEON unit
> > >   crypto/arm64: sm4-ce-ccm - Avoid pointless yield of the NEON unit
> > >   crypto/arm64: sm4-ce-gcm - Avoid pointless yield of the NEON unit
> > >   arm64/fpsimd: Require kernel NEON begin/end calls from the same scope
> > >   arm64/fpsimd: Allocate kernel mode FP/SIMD buffers on the stack
> > >
> > >  arch/arm64/crypto/aes-ce-ccm-glue.c |  5 +--
> > >  arch/arm64/crypto/sm4-ce-ccm-glue.c | 10 ++----
> > >  arch/arm64/crypto/sm4-ce-gcm-glue.c | 10 ++----
> > >  arch/arm64/include/asm/neon.h       |  7 ++--
> > >  arch/arm64/include/asm/processor.h  |  2 +-
> > >  arch/arm64/kernel/fpsimd.c          | 34 +++++++++++++-------
> > >  6 files changed, 34 insertions(+), 34 deletions(-)
> >
> > This looks like the right decision: saving 528 bytes per task is
> > significant.  528 bytes is a lot to allocate on the stack too, but
> > functions that use the NEON registers are either leaf functions or very
> > close to being leaf functions, so it should be okay.
> >
> > The implementation is a bit unusual, though:
> >
> >    #define kernel_neon_begin()        do { __kernel_neon_begin(&(struct user_fpsimd_state){})
> >    #define kernel_neon_end()  __kernel_neon_end(); } while (0)
> >
> > It works, but normally macros don't start or end code blocks behind the
> > scenes like this.  Perhaps it should be more like s390's
> > kernel_fpu_begin(), where the caller provides the buffer that the
> > registers are stored in?
>
> We've done stuff like this in the past, but I did wonder if we could use
> any of the cleanup.h style logic to do this instead of the split
> do/while? I think this would work, totally untested:
>
> DEFINE_LOCK_GUARD_0(neon,
>                     __kernel_neon_begin(&(struct user_fpsimd_state){}),
>                     __kernel_neon_end())
> ...
> scoped_guard(neon) {
>         do neon things
> }
>
> It would require a fair bit of refactoring to replace the existing
> begin/end code, though...
>

Yeah, but I think it's worth it. And let's call it simd and reuse that
on other architectures as well.

The problem with the above, though, is that the temporary stack
variable goes out of scope when the constructor function returns. So
probably better to do something like

DEFINE_LOCK_GUARD_1(simd,
                    struct user_fpsimd_state,
                    __kernel_neon_begin(_T->lock),
                    __kernel_neon_end(_T->lock));

#define scoped_simd() scoped_guard(simd, &(struct user_fpsimd_state){})

leaving the possibility for callers to use scoped_guard() directly and
pass their own user_fpsimd_state pointer.

We'd need to add some debug checks in __kernel_neon_end() to ensure
that the provided pointer matches the one that was passed to
__kernel_neon_begin().