Re: [PATCH v1 1/1] s390/crypto: Generate intermediate CV for API partial block handling

On 8/14/26 16:31, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardware Alignment Requirement Violation due to `__packed` attribute
> --
> 
> --- Patch [1]: [PATCH v1 1/1] s390/crypto: Generate intermediate CV for API partial block handling ---
> commit 6ca29d2947b71c2e1420feb8fbee6504922ff37f
> Author: Holger Dengler <[email protected]>
> 
> s390/crypto: Generate intermediate CV for API partial block handling
> 
> This commit updates the internal hash_data() function to conditionally use
> cpacf_kimd() for intermediate chaining values when API partial block handling
> is requested, reserving cpacf_klmd() for the final hash value.
> 
>> diff --git a/arch/s390/crypto/hmac_s390.c b/arch/s390/crypto/hmac_s390.c
>> index f8cd09f341d4b..445fa7bbd9586 100644
>> --- a/arch/s390/crypto/hmac_s390.c
>> +++ b/arch/s390/crypto/hmac_s390.c
>> @@ -150,7 +150,10 @@ static int hash_data(const u8 *in, unsigned int inlen,
>>  
>>  #undef PARAM_INIT
>>  
>> -	cpacf_klmd(func, &param, in, inlen);
>> +	if (final)
>> +		cpacf_klmd(func, &param, in, inlen);
>> +	else
>> +		cpacf_kimd(func, &param, in, inlen);
> 
> [Severity: High]
> This is a pre-existing issue, but could the alignment of the param block passed
> to cpacf_kimd() and cpacf_klmd() trigger a hardware specification exception?
> 
> The param variable is defined earlier in hash_data() with the __packed
> attribute:
> 
> 	union {
> 		struct sha256_paramblock {
> 			u32 h[8];
> 			u64 mbl;
> 		} sha256;
> 		struct sha512_paramblock {
> 			u64 h[8];
> 			u128 mbl;
> 		} sha512;
> 	} __packed param;
> 
> The __packed attribute reduces the compiler's alignment requirement for param
> to 1 byte. The z/Architecture requires doubleword (8-byte) alignment for KIMD
> and KLMD parameter blocks.

This is not true. The z/Architecture (as described in "z/Architecture,
Principles of Operation") does not require any alignment for the address
of the parameter block in general register GR1.

> If the compiler places param on a stack address not divisible by 8, won't this
> cause a kernel panic when passed to these hardware instructions?

There is no specification exception documented, if the parameter block
address in GR1 is not 8-byte aligned. A stack address not divisible by 8
will not cause a specification exception and there for also no kernel crash.

> Since the struct fields inside the union are naturally packed, can we remove
> the __packed attribute to ensure the compiler maintains safe alignment?

The __packed statement may be redundant. But I would prefer to leave it
in the code, because it increases the readability.

[...]

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler