Re: [GIT PULL] Crypto Fixes for 6.17

[Vegard Nossum]

On 09/08/2025 06:32, Linus Torvalds wrote:
> On Fri, 8 Aug 2025 at 08:42, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> > This push fixes a regression that breaks hmac(sha3-224-s390).
>
> _Please_ describe the completely random strange constants, and why they changed.
>
> What is "361", and why did 360 use to work but no longer does?
>
> I've pulled this, because I'm sure it fixes a bug, but neither the
> pull message nor the commit have acceptable explanations.
>
> And honestly, the code should be fixed too. Having a random constant
> like that with no explanation for the completely random value is not
> ok.

The actual explanation is given in the email here:

On Wed, Jul 30, 2025 at 09:11:49AM -0700, Eric Biggers wrote:
> I haven't touched SHA-3 yet.  This is a bug from the following
> commit:
>
> commit 6f90ba7065515d69b24729cf85c45b2add99e638 Author: Herbert Xu
> <herbert@xxxxxxxxxxxxxxxxxxx> Date:   Fri Apr 18 11:00:13 2025 +0800
>
> crypto: s390/sha3 - Use API partial block handling
>
> Use the Crypto API partial block handling.
>
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> That increased the descsize of hmac(sha3-224-s390) from 368 to 369, 
> which made it exceed HASH_MAX_DESCSIZE, causing it to fail to
> register.
(https://lore.kernel.org/all/20250730161149.GA1162@sol/)

This is an anti-pattern of the crypto code that AFAICT ultimately stems
from the removal of VLAs:

commit b68a7ec1e9a3efac53ae26a1658a553825a2375c
Date:   Tue Aug 7 14:18:38 2018 -0700

    crypto: hash - Remove VLA usage

which replaced e.g. crypto_shash_descsize(ctx) by HASH_MAX_DESCSIZE, a
hard coded limit that's supposed to capture the biggest struct you can
possibly put on the stack (SHASH_DESC_ON_STACK() etc.) -- since the
crypto API is stringly typed you cannot know the exact size of the
thing you are requesting ahead of time (the sizes could vary depending
on which implementation the crypto API decides to use).

I call it an anti-pattern because it's not the first time this has had
bugs either:

commit e1354400b25da645c4764ed6844d12f1582c3b66
Date:   Tue May 14 16:13:15 2019 -0700

    crypto: hash - fix incorrect HASH_MAX_DESCSIZE

As a minimal future-proofing fix, maybe we could add something like

BUILD_BUG_ON(sizeof(struct md5_state) <= HASH_MAX_DESCSIZE);

to every hashing algorithm, and/or a dynamic check in the crypto API
(completely untested):

--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -361,6 +361,8 @@ int crypto_register_shash(struct shash_alg *alg)
        struct crypto_alg *base = &alg->base;
        int err;

+       WARN_ON(alg->descsize > HASH_MAX_DESCSIZE);
+
        err = shash_prepare_alg(alg);
        if (err)
                return err;

...or maybe those on-stack users should just do the kmalloc and be done
with it.


Vegard