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

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

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