linux-kernel - Re: [PATCH] lib/crypto: blake2b: Limit frame size workaround to GCC

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

Message-ID: <20251124090846.18d02a78@pumpkin>
Date: Mon, 24 Nov 2025 09:08:46 +0000
From: david laight <david.laight@...box.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: Thorsten Blum <thorsten.blum@...ux.dev>, "Jason A. Donenfeld"
 <Jason@...c4.com>, Ard Biesheuvel <ardb@...nel.org>,
 linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/crypto: blake2b: Limit frame size workaround to GCC
 < 12.2 on i386

On Sun, 23 Nov 2025 12:26:29 -0800
Eric Biggers <ebiggers@...nel.org> wrote:

> On Sun, Nov 23, 2025 at 06:58:18PM +0000, david laight wrote:
> > On Sun, 23 Nov 2025 18:00:01 +0100
> > Thorsten Blum <thorsten.blum@...ux.dev> wrote:
> >   
> > > On 23. Nov 2025, at 10:28, david laight wrote:  
> > > > On Sat, 22 Nov 2025 11:55:31 +0100
> > > > Thorsten Blum <thorsten.blum@...ux.dev> wrote:
> > > >     
> > > >> The GCC bug only occurred on i386 and has been resolved since GCC 12.2.
> > > >> Limit the frame size workaround to GCC < 12.2 on i386.
> > > >> 
> > > >> Signed-off-by: Thorsten Blum <thorsten.blum@...ux.dev>
> > > >> ---
> > > >> lib/crypto/Makefile | 4 ++++
> > > >> 1 file changed, 4 insertions(+)
> > > >> 
> > > >> diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile
> > > >> index b5346cebbb55..5ee36a231484 100644
> > > >> --- a/lib/crypto/Makefile
> > > >> +++ b/lib/crypto/Makefile
> > > >> @@ -33,7 +33,11 @@ obj-$(CONFIG_CRYPTO_LIB_GF128MUL) += gf128mul.o
> > > >> 
> > > >> obj-$(CONFIG_CRYPTO_LIB_BLAKE2B) += libblake2b.o
> > > >> libblake2b-y := blake2b.o
> > > >> +ifeq ($(CONFIG_X86_32),y)
> > > >> +ifeq ($(CONFIG_CC_IS_GCC)_$(call gcc-min-version, 120200),y_)
> > > >> CFLAGS_blake2b.o := -Wframe-larger-than=4096 #  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105930
> > > >> +endif # CONFIG_CC_IS_GCC
> > > >> +endif # CONFIG_X86_32    
> > > > 
> > > > Isn't that just going to cause a run-time stack overflow?    
> > > 
> > > My change doesn't cause a runtime stack overflow, it's just a compiler
> > > warning. There's more information in commit 1d3551ced64e ("crypto:
> > > blake2b: effectively disable frame size warning").
> > > 
> > > Given the kernel test robot results with GCC 15.1.0 on m68k, we should
> > > probably make this conditional on GCC (any version). Clang produces much
> > > smaller stack frames and should be fine with the default warning
> > > threshold.  
> > 
> > But if anyone tries to run the kernel they'll need space for the '3k monster stack'.
> > So changing the limit is 'fine' for a test build, but not for a proper build.
> > (Yes this has been wrong since Linus did the original patch in 2022.)
> > 
> > Does allmodconfig set COMPILE_TEST ?
> > If so that could be included in the conditional.
> > 
> > A more interesting question is whether the change can just be removed.
> > I'd guess no one is actively using gcc 12.1 any more.  
> 
> How about we roll up the BLAKE2b rounds loop if !CONFIG_64BIT?

I do wonder about the real benefit of some of the massive loop unrolling
that happens in a lot of these algorithms (not just blake2b).
It might speed up (some) benchmarks, but the 'I-cache busting' effect
may well some down any real uses - especially on small/moderate sized buffers.
Loop unrolling is so 1980s...

And that is an entirely separate issue from any register spills.
If the compiler is going to spill to stack the benefits of unrolling are
likely to disappear - especially on a modern 'out of order' and 'multi issue'
cpu.
On x86 you normally get any 'loop control' for free, normal loop unrolling
is pretty pointless except for very short loops (you can't do a 1 clock loop).

Register pressure on a 32bit cpu doing 64bit operations is immense.
Worse for old architectures with very few registers - x86 can only hold
three 64bit values in registers.
So the compiler ends up spilling 'temporary' values from the middle of
expressions as well as all obvious named variables.

So yes, rolling it up (or not unrolling it) on 32bit is a good idea.

	David

> 
> - Eric