s390: CI failures on all sha kernel modules on linux-next

s390: CI failures on all sha kernel modules on linux-next

[Harald Freudenberger]

Hello Herbert
since commit 572b5c4682c7 "crypto: s390/sha512 - Use API partial block 
handling"
all CI tests related to sha kernel modules on the next kernel fail.

Some investigations shows that modprobe fails with -EINVAL, caused
in shash.c line 477. After some reading through the commit I get the
impression that the root cause is in the sudden increase of the
alg.descsize from 213 bytes to 536 bytes. This is caused by the
modifications of the s390_sha_ctx:

...
  struct s390_sha_ctx {
  	u64 count;		/* message length in bytes */
-	u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
+	union {
+		u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
+		struct {
+			u64 state[SHA512_DIGEST_SIZE];                  <- really big
+			u64 count_hi;
+		} sha512;
+	};
  	int func;		/* KIMD function to use */
  	bool first_message_part;
-	u8 buf[SHA_MAX_BLOCK_SIZE];
  };
...

as part of the patch. Having a close look here gives me the strong
impression that the state here is way too big and it should be:

...
  struct s390_sha_ctx {
  	u64 count;		/* message length in bytes */
-	u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
+	union {
+		u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
+		struct {
+			u64 state[SHA512_DIGEST_SIZE / sizeof(u64)];
+			u64 count_hi;
+		} sha512;
+	};
  	int func;		/* KIMD function to use */
  	bool first_message_part;
-	u8 buf[SHA_MAX_BLOCK_SIZE];
  };
...

instead. With this modifications all our sha modules do load perfect
and the CI tests succeed. Please check and maybe improve your patch.

Thanks and have a nice day
Harald Freudenberger

[PATCH] crypto: s390/sha512 - Fix sha512 state size

[Herbert Xu]

On Tue, Apr 29, 2025 at 10:57:38AM +0200, Harald Freudenberger wrote:
> Hello Herbert
> since commit 572b5c4682c7 "crypto: s390/sha512 - Use API partial block
> handling"
> all CI tests related to sha kernel modules on the next kernel fail.

Thanks for catching this!

>  struct s390_sha_ctx {
>  	u64 count;		/* message length in bytes */
> -	u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
> +	union {
> +		u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
> +		struct {
> +			u64 state[SHA512_DIGEST_SIZE];                  <- really big
> +			u64 count_hi;
> +		} sha512;
> +	};

I was trying to keep this thing from growing over the 360 limit,
hence the union.

Instead I managed to make it go over the limit spectacularly :)

---8<---
The sha512 state size in s390_sha_ctx is out by a factor of 8,
fix it so that it stays below HASH_MAX_DESCSIZE.  Also fix the
state init function which was writing garbage to the state.  Luckily
this is not actually used for anything other than export.

Reported-by: Harald Freudenberger <freude@linux.ibm.com>
Fixes: 572b5c4682c7 ("crypto: s390/sha512 - Use API partial block handling")
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 0a3cc1739144..d757ccbce2b4 100644
--- a/arch/s390/crypto/sha.h
+++ b/arch/s390/crypto/sha.h
@@ -24,7 +24,7 @@ struct s390_sha_ctx {
 	union {
 		u32 state[CPACF_MAX_PARMBLOCK_SIZE / sizeof(u32)];
 		struct {
-			u64 state[SHA512_DIGEST_SIZE];
+			u64 state[SHA512_DIGEST_SIZE / sizeof(u64)];
 			u64 count_hi;
 		} sha512;
 	};
diff --git a/arch/s390/crypto/sha512_s390.c b/arch/s390/crypto/sha512_s390.c
index 14818fcc9cd4..3c5175e6dda6 100644
--- a/arch/s390/crypto/sha512_s390.c
+++ b/arch/s390/crypto/sha512_s390.c
@@ -22,13 +22,13 @@ static int sha512_init(struct shash_desc *desc)
 	struct s390_sha_ctx *ctx = shash_desc_ctx(desc);
 
 	ctx->sha512.state[0] = SHA512_H0;
-	ctx->sha512.state[2] = SHA512_H1;
-	ctx->sha512.state[4] = SHA512_H2;
-	ctx->sha512.state[6] = SHA512_H3;
-	ctx->sha512.state[8] = SHA512_H4;
-	ctx->sha512.state[10] = SHA512_H5;
-	ctx->sha512.state[12] = SHA512_H6;
-	ctx->sha512.state[14] = SHA512_H7;
+	ctx->sha512.state[1] = SHA512_H1;
+	ctx->sha512.state[2] = SHA512_H2;
+	ctx->sha512.state[3] = SHA512_H3;
+	ctx->sha512.state[4] = SHA512_H4;
+	ctx->sha512.state[5] = SHA512_H5;
+	ctx->sha512.state[6] = SHA512_H6;
+	ctx->sha512.state[7] = SHA512_H7;
 	ctx->count = 0;
 	ctx->sha512.count_hi = 0;
 	ctx->func = CPACF_KIMD_SHA_512;
-- 
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: s390/sha512 - Fix sha512 state size

[Ingo Franzki]

On 29.04.2025 11:12, Herbert Xu wrote:


> diff --git a/arch/s390/crypto/sha512_s390.c b/arch/s390/crypto/sha512_s390.c
> index 14818fcc9cd4..3c5175e6dda6 100644
> --- a/arch/s390/crypto/sha512_s390.c
> +++ b/arch/s390/crypto/sha512_s390.c
> @@ -22,13 +22,13 @@ static int sha512_init(struct shash_desc *desc)
>  	struct s390_sha_ctx *ctx = shash_desc_ctx(desc);
>  
>  	ctx->sha512.state[0] = SHA512_H0;
> -	ctx->sha512.state[2] = SHA512_H1;
> -	ctx->sha512.state[4] = SHA512_H2;
> -	ctx->sha512.state[6] = SHA512_H3;
> -	ctx->sha512.state[8] = SHA512_H4;
> -	ctx->sha512.state[10] = SHA512_H5;
> -	ctx->sha512.state[12] = SHA512_H6;
> -	ctx->sha512.state[14] = SHA512_H7;
> +	ctx->sha512.state[1] = SHA512_H1;
> +	ctx->sha512.state[2] = SHA512_H2;
> +	ctx->sha512.state[3] = SHA512_H3;
> +	ctx->sha512.state[4] = SHA512_H4;
> +	ctx->sha512.state[5] = SHA512_H5;
> +	ctx->sha512.state[6] = SHA512_H6;
> +	ctx->sha512.state[7] = SHA512_H7;
>  	ctx->count = 0;
>  	ctx->sha512.count_hi = 0;
>  	ctx->func = CPACF_KIMD_SHA_512;

We still get one selftest failure on sha384:

/proc/crypto:
name         : sha384
driver       : sha384-s390
module       : sha512_s390
priority     : 300
refcnt       : 1
selftest     : unknown   <---------
internal     : no
type         : shash
blocksize    : 128
digestsize   : 48

Syslog:
alg: shash: sha384-s390 test failed (wrong result) on test vector 0, cfg="init+update+final aligned buffer"

Shouldn't the sha384_init() function also use array indexes 0-7 like your fix in sha512_init() ?
It currently is:

 static int sha384_init(struct shash_desc *desc)
 {
 	struct s390_sha_ctx *ctx = shash_desc_ctx(desc);
 
 	*(__u64 *)&ctx->state[0] = SHA384_H0;
 	*(__u64 *)&ctx->state[2] = SHA384_H1;
 	*(__u64 *)&ctx->state[4] = SHA384_H2;
 	*(__u64 *)&ctx->state[6] = SHA384_H3;
 	*(__u64 *)&ctx->state[8] = SHA384_H4;
 	*(__u64 *)&ctx->state[10] = SHA384_H5;
 	*(__u64 *)&ctx->state[12] = SHA384_H6;
 	*(__u64 *)&ctx->state[14] = SHA384_H7;
 	ctx->count = 0;
 	ctx->func = CPACF_KIMD_SHA_512;
 
 	return 0;
 }

-- 
Ingo Franzki
eMail: ifranzki@linux.ibm.com  
Tel: ++49 (0)7031-16-4648
Linux on IBM Z Development, Schoenaicher Str. 220, 71032 Boeblingen, Germany

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM DATA Privacy Statement: https://www.ibm.com/privacy/us/en/

[PATCH] crypto: s390/sha512 - Initialise upper counter to zero for sha384

[Herbert Xu]

On Mon, May 05, 2025 at 02:45:09PM +0200, Ingo Franzki wrote:
>
> Shouldn't the sha384_init() function also use array indexes 0-7 like your fix in sha512_init() ?

It certainly should.  Although that is not the reason why it fails
as the 32-to-64 change is just cosmetic.  The real reason is that
as I overlooked this function the high bits of the counter just isn't
set and contains garbage:

---8<---
Initialise the high bit counter to zero in sha384_init.

Also change the state initialisation to use ctx->sha512.state
instead of ctx->state for consistency.

Fixes: 572b5c4682c7 ("crypto: s390/sha512 - Use API partial block handling")
Reported-by: Ingo Franzki <ifranzki@linux.ibm.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/arch/s390/crypto/sha512_s390.c b/arch/s390/crypto/sha512_s390.c
index 3c5175e6dda6..33711a29618c 100644
--- a/arch/s390/crypto/sha512_s390.c
+++ b/arch/s390/crypto/sha512_s390.c
@@ -86,15 +86,16 @@ static int sha384_init(struct shash_desc *desc)
 {
 	struct s390_sha_ctx *ctx = shash_desc_ctx(desc);
 
-	*(__u64 *)&ctx->state[0] = SHA384_H0;
-	*(__u64 *)&ctx->state[2] = SHA384_H1;
-	*(__u64 *)&ctx->state[4] = SHA384_H2;
-	*(__u64 *)&ctx->state[6] = SHA384_H3;
-	*(__u64 *)&ctx->state[8] = SHA384_H4;
-	*(__u64 *)&ctx->state[10] = SHA384_H5;
-	*(__u64 *)&ctx->state[12] = SHA384_H6;
-	*(__u64 *)&ctx->state[14] = SHA384_H7;
+	ctx->sha512.state[0] = SHA384_H0;
+	ctx->sha512.state[1] = SHA384_H1;
+	ctx->sha512.state[2] = SHA384_H2;
+	ctx->sha512.state[3] = SHA384_H3;
+	ctx->sha512.state[4] = SHA384_H4;
+	ctx->sha512.state[5] = SHA384_H5;
+	ctx->sha512.state[6] = SHA384_H6;
+	ctx->sha512.state[7] = SHA384_H7;
 	ctx->count = 0;
+	ctx->sha512.count_hi = 0;
 	ctx->func = CPACF_KIMD_SHA_512;
 
 	return 0;
-- 
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