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

[Harald Freudenberger <freude@linux.ibm.com>]

On 2025-10-14 11:19, Holger Dengler 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:
> 
> In my opinion, the information up to here should not be part of the
> commit message. If you want to keep it, I would suggest to move it to
> the cover letter.
> 

Ok, will remove this.

>> 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).
>> 
>> 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>
> 
> See my comments below.
> 
>> ---
>>  arch/s390/crypto/phmac_s390.c | 52 
>> +++++++++++++++++++++++------------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
>> 
>> diff --git a/arch/s390/crypto/phmac_s390.c 
>> b/arch/s390/crypto/phmac_s390.c
>> index 7ecfdc4fba2d..5d38a48cc45d 100644
>> --- a/arch/s390/crypto/phmac_s390.c
>> +++ b/arch/s390/crypto/phmac_s390.c
>> @@ -169,11 +169,18 @@ struct kmac_sha2_ctx {
>>  	u64 buflen[2];
>>  };
>> 
>> +enum async_op {
>> +	OP_NOP = 0,
> 
> The async_op element in struct phmac_req_ctx is implicitly initialized
> with OP_NOP. Only the functions update, final and finup will set
> another (valid) operation.  Can it ever happen, that do_one_request()
> is called *before* any of update, final or finup is called? If this is
> a valid case, the OP_NOP should be handled correctly in
> do_one_request(), otherwise we get a -ENOTSUPP (see my comment to
> phmac_do_one_request()).
> 
> If do_one_request() is never called before update/finup/final(), no
> change is required.
> 

Well, do_one_request() is only called via a postponed request pushed
from one of the "front" functions (init/update/final/finup/digest) to
the engine instance. So a request is always first seen by these
functions and these have a chance to update the async_op field.

>> +	OP_UPDATE,
>> +	OP_FINAL,
>> +	OP_FINUP,
>> +};
>> +
>>  /* phmac request context */
>>  struct phmac_req_ctx {
>>  	struct hash_walk_helper hwh;
>>  	struct kmac_sha2_ctx kmac_ctx;
>> -	bool final;
>> +	int async_op;
> 
> I know, that the compiler is happy with an int. But I would prefer to
> use the enum for the element.
> 
> enum async_op async_op;
> 

Catched - my first experiences with C where at a time where enums where
not supported. So I am still not familiar with this kind of stuff :-)

>>  };
>> 
>>  /*
> [...]> @@ -855,15 +865,16 @@ static int phmac_do_one_request(struct
> crypto_engine *engine, void *areq)
>> 
>>  	/*
>>  	 * Three kinds of requests come in here:
>> -	 * update when req->nbytes > 0 and req_ctx->final is false
>> -	 * final when req->nbytes = 0 and req_ctx->final is true
>> -	 * finup when req->nbytes > 0 and req_ctx->final is true
>> -	 * For update and finup the hwh walk needs to be prepared and
>> -	 * up to date but the actual nr of bytes in req->nbytes may be
>> -	 * any non zero number. For final there is no hwh walk needed.
>> +	 * 1. req->async_op == OP_UPDATE with req->nbytes > 0
>> +	 * 2. req->async_op == OP_FINUP with req->nbytes > 0
>> +	 * 3. req->async_op == OP_FINAL
>> +	 * For update and finup the hwh walk has already been prepared
>> +	 * by the caller. For final there is no hwh walk needed.
>>  	 */
>> 
>> -	if (req->nbytes) {
>> +	switch (req_ctx->async_op) {
>> +	case OP_UPDATE:
>> +	case OP_FINUP:
>>  		rc = phmac_kmac_update(req, true);
>>  		if (rc == -EKEYEXPIRED) {
>>  			/*
>> @@ -880,10 +891,11 @@ static int phmac_do_one_request(struct 
>> crypto_engine *engine, void *areq)
>>  			hwh_advance(hwh, rc);
>>  			goto out;
>>  		}
>> -		req->nbytes = 0;
>> -	}
>> -
>> -	if (req_ctx->final) {
>> +		if (req_ctx->async_op == OP_UPDATE)
>> +			break;
>> +		req_ctx->async_op = OP_FINAL;
>> +		fallthrough;
>> +	case OP_FINAL:
>>  		rc = phmac_kmac_final(req, true);
>>  		if (rc == -EKEYEXPIRED) {
>>  			/*
>> @@ -897,10 +909,14 @@ static int phmac_do_one_request(struct 
>> crypto_engine *engine, void *areq)
>>  			cond_resched();
>>  			return -ENOSPC;
>>  		}
>> +		break;
>> +	default:
>> +		/* unknown/unsupported/unimplemented asynch op */
>> +		return -EOPNOTSUPP;
> 
> If it is a valid case, that do_one_request() is called before
> update(), final() or finup() is called, we should handle OP_NOP here
> and not return with an error.
> If do_one_request() is never called before update/finup/final(), no
> change is required.
> 

As wrote above the "front" functions always see a request first before
it is postponed to the engine and appears here. So the OP_NOP case
must not happen here and thus is only covered with the default arm.

> [...]