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

[Holger Dengler <dengler@linux.ibm.com>]

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.

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

> +	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;

>  };
>  
>  /*
[...]> @@ -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.

[...]

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com