crypto: s390/phmac - Do not modify the req->nbytes value

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

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 <[email protected]>

Signed-off-by Harald Freudenberger <[email protected]>

---
 arch/s390/crypto/phmac_s390.c | 52 +++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 18 deletions(-)

Quoted 9 lines · Harald Freudenberger

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

Quoted 16 lines

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.

Quoted 6 lines

Fixes cbbc675506cc ("crypto: s390 - New s390 specific protected key hash phmac")

Reported-by Ingo Franzki <[email protected]>

Signed-off-by Harald Freudenberger <[email protected]>

---
 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/[email protected]/T/#u
Also, having the operation in the request context makes the code even more clear IMHO.

Tested-by Ingo Franzki <[email protected]>

Reviewed-by Ingo Franzki <[email protected]>

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

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 <[email protected]>

Signed-off-by Harald Freudenberger <[email protected]>

---
 arch/s390/crypto/phmac_s390.c | 52 +++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 18 deletions(-)

arch/s390/crypto/phmac_s390.c +0 -0

index 7ecfdc4fba2d..5d38a48cc45d 100644

arch/s390/crypto/phmac_s390.c +13 -11

 	/*
 	 * 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) {
 			/*

Quoted 5 lines · Harald Freudenberger

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.

Quoted 20 lines

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.

Quoted 3 lines

Fixes cbbc675506cc ("crypto: s390 - New s390 specific protected key hash phmac")

Reported-by Ingo Franzki <[email protected]>

Signed-off-by Harald Freudenberger <[email protected]>

See my comments below.

Quoted 10 lines

---
 arch/s390/crypto/phmac_s390.c | 52 
+++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 18 deletions(-)

arch/s390/crypto/phmac_s390.c +0 -0

index 7ecfdc4fba2d..5d38a48cc45d 100644

arch/s390/crypto/phmac_s390.c struct kmac_sha2_ctx { · line 173

Quoted 5 lines

arch/s390/crypto/phmac_s390.c +2 -0

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

arch/s390/crypto/phmac_s390.c struct kmac_sha2_ctx { · line 183

Quoted 11 lines

arch/s390/crypto/phmac_s390.c +6 -1

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

arch/s390/crypto/phmac_s390.c struct kmac_sha2_ctx { · line 186

Quoted 3 lines

arch/s390/crypto/phmac_s390.c +0 -0

 };
 
 /*

[...]> @@ -855,15 +865,16 @@ static int phmac_do_one_request(struct crypto_engine *engine, void *areq)

Quoted 47 lines

arch/s390/crypto/phmac_s390.c +13 -11

 	/*
 	 * 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) {

arch/s390/crypto/phmac_s390.c +2 -0

 			/*

@@ -897,10 +909,14 @@ static int phmac_do_one_request(struct 
 crypto_engine *engine, void *areq)
 			cond_resched();
 			return -ENOSPC;
 		}
+		break;
+	default:

arch/s390/crypto/phmac_s390.c static int phmac_do_one_request(struct crypto_engine *engine, void *areq) · line 915

Quoted 2 lines

arch/s390/crypto/phmac_s390.c +2 -0

+		/* 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.

[...]

Quoted 1 line · Holger Dengler

On 09/10/2025 18:01, Harald Freudenberger wrote:

Quoted 10 lines

Quoted 1 line

+    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()).

Quoted 3 lines

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.