s390: CI failures on all sha kernel modules on linux-next
Subsystems (3)
Harald Freudenberger
maintainerHello 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
Herbert Xu
maintainerPATCH 1/1 crypto: s390/sha512 - Fix sha512 state size
Quoted 4 lines · Harald Freudenberger
Hello Herbert
since commit572b5c4682c7"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!
Review
Quoted 10 lines
struct
s390_sha_ctx{(quoted diff) +7
-1u64 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.
arch/s390/crypto/sha.h +1 -1
@@ -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; };
arch/s390/crypto/sha512_s390.c +7 -7
@@ -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 ;
Signature · 3 lines
Email: Herbert Xu <[email protected]> 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
Quoted 15 lines · Herbert Xu
arch/s390/crypto/sha512_s390.c +0
-7@@ -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 ;
arch/s390/crypto/sha512_s390.c static int sha512_init(struct shash_desc *desc) · line 34
Quoted 10 lines
arch/s390/crypto/sha512_s390.c +7
-0+ 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;
}
Signature · 10 lines
Ingo Franzki eMail: [email protected] 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/
Herbert Xu
maintainerPATCH 1/1 crypto: s390/sha512 - Initialise upper counter to zero for sha384
Quoted 2 lines · Ingo Franzki
Shouldn't the
sha384_init()function also use array indexes 0-7 like your fix insha512_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.
arch/s390/crypto/sha512_s390.c +9 -8
@@ -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 ;
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt