Re: [PATCH] crypto: s390/sha3 - Use cpu byte-order when exporting

[Ingo Franzki <ifranzki@linux.ibm.com>]

On 23.05.2025 14:28, Herbert Xu wrote:
> On Fri, May 23, 2025 at 02:03:23PM +0200, Ingo Franzki wrote:
>>
>> I hope you can sort it out what belongs to what.
> 
> Please let me know if this patch works or not:

Yes this fixes the selftest failures.

However, I am not sure if the handling of the first_message_part field is correct (or even was correct before your fix).
The first_message_part is essentially there to control the setting of the NIP (No-ICV-Parameter) flag for the KIMD and KLMD instructions in s390_sha_update() and s390_sha_final() in arch/s390/crypto/sha_common.c. That NIP flag is only supported when MSA 12 is available (facility 86). 

It must be 1 until the very first KIMD or KLMD call has been done, and must be resetted to zero afterwards. 
If NIP is set then you can omit the zeroing out of the initial parameter block. This saves a few cycles for the memset, but also for transferring the (all zero) IV to the coprocessor. 

All seems OK, unless that you always set first_message_part to zero at import. So even if an initial state was exported, after an import first_message_part is off. Previous code did retain the stat of the first_message_part field in the exported state as well. 
OK, one can argue that this loss of performance improvement is negligible for a saved initial state situation.....

> 
> ---8<---
> The sha3 partial hash on s390 is in little-endian just like the
> final hash.  However the generic implementation produces native
> or big-endian partial hashes.
> 
> Make s390 sha3 conform to that by doing the byte-swap on export
> and import.
> 
> Reported-by: Ingo Franzki <ifranzki@linux.ibm.com>
> Fixes: 6f90ba706551 ("crypto: s390/sha3 - 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 d757ccbce2b4..cadb4b13622a 100644
> --- a/arch/s390/crypto/sha.h
> +++ b/arch/s390/crypto/sha.h
> @@ -27,6 +27,9 @@ struct s390_sha_ctx {
>  			u64 state[SHA512_DIGEST_SIZE / sizeof(u64)];
>  			u64 count_hi;
>  		} sha512;
> +		struct {
> +			__le64 state[SHA3_STATE_SIZE / sizeof(u64)];
> +		} sha3;
>  	};
>  	int func;		/* KIMD function to use */
>  	bool first_message_part;
> diff --git a/arch/s390/crypto/sha3_256_s390.c b/arch/s390/crypto/sha3_256_s390.c
> index 4a7731ac6bcd..03bb4f4bab70 100644
> --- a/arch/s390/crypto/sha3_256_s390.c
> +++ b/arch/s390/crypto/sha3_256_s390.c
> @@ -35,23 +35,33 @@ static int sha3_256_init(struct shash_desc *desc)
>  static int sha3_256_export(struct shash_desc *desc, void *out)
>  {
>  	struct s390_sha_ctx *sctx = shash_desc_ctx(desc);
> -	struct sha3_state *octx = out;
> +	union {
> +		u8 *u8;
> +		u64 *u64;
> +	} p = { .u8 = out };
> +	int i;
>  
>  	if (sctx->first_message_part) {
> -		memset(sctx->state, 0, sizeof(sctx->state));
> -		sctx->first_message_part = 0;
> +		memset(out, 0, SHA3_STATE_SIZE);
> +		return 0;
>  	}
> -	memcpy(octx->st, sctx->state, sizeof(octx->st));
> +	for (i = 0; i < SHA3_STATE_SIZE / 8; i++)
> +		put_unaligned(le64_to_cpu(sctx->sha3.state[i]), p.u64++);
>  	return 0;
>  }
>  
>  static int sha3_256_import(struct shash_desc *desc, const void *in)
>  {
>  	struct s390_sha_ctx *sctx = shash_desc_ctx(desc);
> -	const struct sha3_state *ictx = in;
> +	union {
> +		const u8 *u8;
> +		const u64 *u64;
> +	} p = { .u8 = in };
> +	int i;
>  
> +	for (i = 0; i < SHA3_STATE_SIZE / 8; i++)
> +		sctx->sha3.state[i] = cpu_to_le64(get_unaligned(p.u64++));
>  	sctx->count = 0;
> -	memcpy(sctx->state, ictx->st, sizeof(ictx->st));
>  	sctx->first_message_part = 0;
>  	sctx->func = CPACF_KIMD_SHA3_256;
>  
> diff --git a/arch/s390/crypto/sha3_512_s390.c b/arch/s390/crypto/sha3_512_s390.c
> index 018f02fff444..a5c9690eecb1 100644
> --- a/arch/s390/crypto/sha3_512_s390.c
> +++ b/arch/s390/crypto/sha3_512_s390.c
> @@ -34,24 +34,33 @@ static int sha3_512_init(struct shash_desc *desc)
>  static int sha3_512_export(struct shash_desc *desc, void *out)
>  {
>  	struct s390_sha_ctx *sctx = shash_desc_ctx(desc);
> -	struct sha3_state *octx = out;
> -
> +	union {
> +		u8 *u8;
> +		u64 *u64;
> +	} p = { .u8 = out };
> +	int i;
>  
>  	if (sctx->first_message_part) {
> -		memset(sctx->state, 0, sizeof(sctx->state));
> -		sctx->first_message_part = 0;
> +		memset(out, 0, SHA3_STATE_SIZE);
> +		return 0;
>  	}
> -	memcpy(octx->st, sctx->state, sizeof(octx->st));
> +	for (i = 0; i < SHA3_STATE_SIZE / 8; i++)
> +		put_unaligned(le64_to_cpu(sctx->sha3.state[i]), p.u64++);
>  	return 0;
>  }
>  
>  static int sha3_512_import(struct shash_desc *desc, const void *in)
>  {
>  	struct s390_sha_ctx *sctx = shash_desc_ctx(desc);
> -	const struct sha3_state *ictx = in;
> +	union {
> +		const u8 *u8;
> +		const u64 *u64;
> +	} p = { .u8 = in };
> +	int i;
>  
> +	for (i = 0; i < SHA3_STATE_SIZE / 8; i++)
> +		sctx->sha3.state[i] = cpu_to_le64(get_unaligned(p.u64++));
>  	sctx->count = 0;
> -	memcpy(sctx->state, ictx->st, sizeof(ictx->st));
>  	sctx->first_message_part = 0;
>  	sctx->func = CPACF_KIMD_SHA3_512;
>  


-- 
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/