Re: [PATCH v1] crypto: s390/phmac - Do not modify the req->nbytes value

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

On 10.10.2025 09:55, Ingo Franzki wrote:
> On 09.10.2025 18:01, Harald Freudenberger wrote:
>> There was a failure reported by the phmac only in combination
>> with dm-crypt where the phmac is used as the integrity check
>> mechanism. A pseudo phmac which was implemented just as an
>> asynchronous wrapper around the synchronous hmac algorithm did
>> not show this failure. After some debugging the reason is clear:
>> The crypto aead layer obvious uses the req->nbytes value after
>> the verification algorithm is through and finished with the
>> request. If the req->nbytes value has been modified the aead
>> layer will react with -EBADMSG to the caller (dm-crypt).

I guess the place where nbytes is used after the verification is in
crypto_authenc_decrypt_tail().  This is called either directly by
crypto_authenc_decrypt() if crypto_ahash_digest() completed synchronously,
or in case or async completion, via the authenc_verify_ahash_done() callback.

crypto_authenc_decrypt_tail() then does:

	scatterwalk_map_and_copy(ihash, req->src, ahreq->nbytes, authsize, 0);

	if (crypto_memneq(ihash, ahreq->result, authsize))
		return -EBADMSG;

Where the scatterwalk_map_and_copy() uses the ahreq->nbytes value to know the
amount of bytes to copy. If this is zero then the copied ihash value is wrong and
thus it returns -EBADMSG.

Not sure if the number of bytes could be saved elsewhere, to allow that the
requests's nbytes can be modified by the digest (if this is desired to be allowed)?
Otherwise, a comment in the header file that nbytes must not be modified might be useful.

>>
>> Unfortunately the phmac implementation used the req->nbytes
>> field on combined operations (finup, digest) to track the
>> state: with req->nbytes > 0 the update needs to be processed,
>> while req->nbytes == 0 means to do the final operation. For
>> this purpose the req->nbytes field was set to 0 after successful
>> update operation. This worked fine and all tests succeeded but
>> only failed with aead use as dm-crypt with verify uses it.
>>
>> Fixed by a slight rework on the phmac implementation. There is
>> now a new field async_op in the request context which tracks
>> the (asynch) operation to process. So the 'state' via req->nbytes
>> is not needed any more and now this field is untouched and may
>> be evaluated even after a request is processed by the phmac
>> implementation.
>>
>> Fixes: cbbc675506cc ("crypto: s390 - New s390 specific protected key hash phmac")
>> Reported-by: Ingo Franzki <ifranzki@linux.ibm.com>
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> ---
>>  arch/s390/crypto/phmac_s390.c | 52 +++++++++++++++++++++++------------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
> 
> I can confirm that this fixes the failures reported at https://lore.kernel.org/dm-devel/fec83aad-c38b-4617-bb9a-0b9827125d79@linux.ibm.com/T/#u
> Also, having the operation in the request context makes the code even more clear IMHO.
> 
> Tested-by: Ingo Franzki <ifranzki@linux.ibm.com>
> Reviewed-by: Ingo Franzki <ifranzki@linux.ibm.com>
> 
> 


-- 
Ingo Franzki
eMail: ifranzki@linux.ibm.com  
Linux on IBM Z Development, Schoenaicher Str. 220, 71032 Boeblingen, Germany

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
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/