[GIT PULL] Crypto Fixes for 6.17

[GIT PULL] Crypto Fixes for 6.17

[Herbert Xu]

Hi Linus:

The following changes since commit bf24d64268544379d9a9b5b8efc2bb03967703b3:

  crypto: keembay - Use min() to simplify ocs_create_linked_list_from_sg() (2025-07-27 22:41:45 +1000)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git tags/v6.17-p2

for you to fetch changes up to 9d9b193ed73a65ec47cf1fd39925b09da8216461:

  crypto: hash - Increase HASH_MAX_DESCSIZE for hmac(sha3-224-s390) (2025-08-01 19:40:54 +0800)

----------------------------------------------------------------
This push fixes a regression that breaks hmac(sha3-224-s390).
----------------------------------------------------------------

Herbert Xu (1):
      crypto: hash - Increase HASH_MAX_DESCSIZE for hmac(sha3-224-s390)

 include/crypto/hash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [GIT PULL] Crypto Fixes for 6.17

[Linus Torvalds]

On Fri, 8 Aug 2025 at 08:42, Herbert Xu <herbert@gondor.apana.org.au> 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.

             Linus

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@gondor.apana.org.au> 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@gondor.apana.org.au> 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@gondor.apana.org.au>
> 
> 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

Re: [GIT PULL] Crypto Fixes for 6.17

[Linus Torvalds]

On Sat, 9 Aug 2025 at 21:22, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>
> The actual explanation is given in the email here:

Yeah, that should have been in the commit message somewhere.

And honestly, it should have been in the code too. Having very random
constants in header files with no explanation for them is not great.

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

I'd say that it stems from using random sizes with no logic and the
VLAs were just the *previous* problem case of the same issue.

> 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):

The dynamic check may be the right thing to do regardless, but when
fixing outright bugs, at least document what went wrong and why. Not
just "360 was too small for X, so it is now 361".

                Linus

[PATCH] crypto: hash - Make HASH_MAX_DESCSIZE a bit more obvious

[Herbert Xu]

On Sun, Aug 10, 2025 at 07:51:56AM +0300, Linus Torvalds wrote:
>
> Yeah, that should have been in the commit message somewhere.
> 
> And honestly, it should have been in the code too. Having very random
> constants in header files with no explanation for them is not great.

The patch below should make the constant a bit more obvious.
 
> The dynamic check may be the right thing to do regardless, but when
> fixing outright bugs, at least document what went wrong and why. Not
> just "360 was too small for X, so it is now 361".

The dynamic check has always been there (see commit b68a7ec1e9a3).

So this fix wasn't about a buffer overflow, rather it was to make
s390 sha3-224 work again as it got caught by the dynamic check.

Cheers,

---8<---
Move S390_SHA_CTX_SIZE into crypto/hash.h so that the derivation
of HASH_MAX_DESCSIZE is less cryptic.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/arch/s390/crypto/sha.h b/arch/s390/crypto/sha.h
index cadb4b13622a..b9cd9572dd35 100644
--- a/arch/s390/crypto/sha.h
+++ b/arch/s390/crypto/sha.h
@@ -10,14 +10,15 @@
 #ifndef _CRYPTO_ARCH_S390_SHA_H
 #define _CRYPTO_ARCH_S390_SHA_H
 
+#include <crypto/hash.h>
 #include <crypto/sha2.h>
 #include <crypto/sha3.h>
+#include <linux/build_bug.h>
 #include <linux/types.h>
 
 /* must be big enough for the largest SHA variant */
 #define CPACF_MAX_PARMBLOCK_SIZE	SHA3_STATE_SIZE
 #define SHA_MAX_BLOCK_SIZE		SHA3_224_BLOCK_SIZE
-#define S390_SHA_CTX_SIZE		sizeof(struct s390_sha_ctx)
 
 struct s390_sha_ctx {
 	u64 count;		/* message length in bytes */
@@ -42,4 +43,9 @@ int s390_sha_update_blocks(struct shash_desc *desc, const u8 *data,
 int s390_sha_finup(struct shash_desc *desc, const u8 *src, unsigned int len,
 		   u8 *out);
 
+static inline void __check_s390_sha_ctx_size(void)
+{
+	BUILD_BUG_ON(S390_SHA_CTX_SIZE != sizeof(struct s390_sha_ctx));
+}
+
 #endif
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index ed63b904837d..44d407cb0c90 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -177,14 +177,26 @@ struct shash_desc {
 
 #define HASH_MAX_DIGESTSIZE	 64
 
+/*
+ * The size of a core hash state and a partial block.  The final byte
+ * is the length of the partial block.
+ */
+#define HASH_STATE_AND_BLOCK(state, block) ((state) + (block) + 1)
+
+
 /* Worst case is sha3-224. */
-#define HASH_MAX_STATESIZE	 200 + 144 + 1
+#define HASH_MAX_STATESIZE	 HASH_STATE_AND_BLOCK(200, 144)
+
+/* This needs to match arch/s390/crypto/sha.h. */
+#define S390_SHA_CTX_SIZE	216
 
 /*
  * Worst case is hmac(sha3-224-s390).  Its context is a nested 'shash_desc'
  * containing a 'struct s390_sha_ctx'.
  */
-#define HASH_MAX_DESCSIZE	(sizeof(struct shash_desc) + 361)
+#define SHA3_224_S390_DESCSIZE	HASH_STATE_AND_BLOCK(S390_SHA_CTX_SIZE, 144)
+#define HASH_MAX_DESCSIZE	(sizeof(struct shash_desc) + \
+				 SHA3_224_S390_DESCSIZE)
 #define MAX_SYNC_HASH_REQSIZE	(sizeof(struct ahash_request) + \
 				 HASH_MAX_DESCSIZE)
 
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: hash - Make HASH_MAX_DESCSIZE a bit more obvious

[Linus Torvalds]

On Mon, 11 Aug 2025 at 07:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> The patch below should make the constant a bit more obvious.

Indeed.

It would be good to maybe minimize the on-stack max-sized allocations,
but that's a separate issue. Several hundred bytes is a noticeable
part of the stack, and it's not always clear that it's a shallow stack
with not a lot else going on..

(I just randomly picked the btrfs csum hash to look at, which can
apparently be one of crc32c / xxhash64 / sha256 or blake2b, and which
is then used at bio submission time, and I wouldn't be surprised if it
probably has a pretty deep stack at that point already).

Oh well.

          Linus

Re: [PATCH] crypto: hash - Make HASH_MAX_DESCSIZE a bit more obvious

[Eric Biggers]

On Mon, Aug 11, 2025 at 10:10:44AM +0300, Linus Torvalds wrote:
> On Mon, 11 Aug 2025 at 07:44, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > The patch below should make the constant a bit more obvious.
> 
> Indeed.
> 
> It would be good to maybe minimize the on-stack max-sized allocations,
> but that's a separate issue. Several hundred bytes is a noticeable
> part of the stack, and it's not always clear that it's a shallow stack
> with not a lot else going on..
> 
> (I just randomly picked the btrfs csum hash to look at, which can
> apparently be one of crc32c / xxhash64 / sha256 or blake2b, and which
> is then used at bio submission time, and I wouldn't be surprised if it
> probably has a pretty deep stack at that point already).

HASH_MAX_DESCSIZE has to be enough for *any* algorithm accessible via
the crypto_shash API, which makes HMAC-SHA3-224 be the limiting factor.
By converting users to use the library APIs instead, they will instead
use strongly-typed contexts that are sized correctly for the algorithms
actually being used.  In the btrfs csum case, the applicable sizes are:

    shash_desc + HASH_MAX_DESCSIZE: 377
    blake2b: 232
    sha256: 104
    xxhash64: 76
    crc32c: 4

So the reduction for btrfs will be 377 => 232.  But blake2b is missing a
library API, so I need to add that first.

- Eric

Re: [GIT PULL] Crypto Fixes for 6.17

[pr-tracker-bot]

The pull request you sent on Fri, 8 Aug 2025 13:41:51 +0800:

> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git tags/v6.17-p2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/01b6ba6b097a0ceeef1975ae37c1660fed1b560c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html