crypto: s390/phmac - Do not modify the req->nbytes value
Subsystems (3)
Harald Freudenberger
maintainerPATCH v2 1/1 crypto: s390/phmac - Do not modify the req->nbytes value
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. However, aead uses the req->nbytes field after a successful hash operation to determine the amount of data to en/decrypt. So an implementation must not modify the nbytes field.
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.
--- arch/s390/crypto/phmac_s390.c | 52 +++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 18 deletions(-)
arch/s390/crypto/phmac_s390.c +34 -18
@@ -169,11 +169,18 @@ struct kmac_sha2_ctx {u64 buflen[2 ]; }; +enum async_op { +OP_NOP =0 , +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; +enum async_op async_op; };/*
@@ -610,6 +617,7 @@ static int phmac_update(struct ahash_request *req)* using engine to serialize requests. */ if (rc ==0 || rc == -EKEYEXPIRED ) { + req_ctx->async_op =OP_UPDATE ;atomic_inc (&tfm_ctx->via_engine_ctr); rc =crypto_transfer_hash_request_to_engine (phmac_crypto_engine, req);if (rc != -EINPROGRESS )
@@ -647,8 +655,7 @@ static int phmac_final(struct ahash_request *req)* using engine to serialize requests. */ if (rc ==0 || rc == -EKEYEXPIRED ) { - req->nbytes =0 ; - req_ctx->final = true; + req_ctx->async_op =OP_FINAL ;atomic_inc (&tfm_ctx->via_engine_ctr); rc =crypto_transfer_hash_request_to_engine (phmac_crypto_engine, req);if (rc != -EINPROGRESS )
@@ -676,13 +683,16 @@ static int phmac_finup(struct ahash_request *req)if (rc) goto out; + req_ctx->async_op =OP_FINUP ; +/* Try synchronous operations if no active engine usage */ if (!atomic_read (&tfm_ctx->via_engine_ctr)) { rc =phmac_kmac_update (req, false);if (rc ==0 ) - req->nbytes =0 ; + req_ctx->async_op =OP_FINAL ; } -if (!rc && !req->nbytes && !atomic_read (&tfm_ctx->via_engine_ctr)) { +if (!rc && req_ctx->async_op ==OP_FINAL && + !atomic_read (&tfm_ctx->via_engine_ctr)) { rc =phmac_kmac_final (req, false);if (rc ==0 ) goto out;
@@ -694,7 +704,7 @@ static int phmac_finup(struct ahash_request *req)* using engine to serialize requests. */ if (rc ==0 || rc == -EKEYEXPIRED ) { - req_ctx->final = true; +/* req->async_op has been set to either OP_FINUP or OP_FINAL */ atomic_inc (&tfm_ctx->via_engine_ctr); rc =crypto_transfer_hash_request_to_engine (phmac_crypto_engine, req);if (rc != -EINPROGRESS )
@@ -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 ; } out: -if (rc || req_ctx->final) +if (rc || req_ctx->async_op ==OP_FINAL )memzero_explicit (kmac_ctx,sizeof (*kmac_ctx));pr_debug ("request complete with rc=%d\n" , rc);local_bh_disable ();
Signature · 1 line
2.43.0
Quoted 16 lines · Harald Freudenberger
The phmac implementation used the
req->nbytesfield on combined operations (finup, digest) to track the state:
withreq->nbytes> 0 the update needs to be processed, whilereq->nbytes== 0 means to do the final operation. For this purpose thereq->nbytesfield was set to 0 after successful update operation. However, aead uses thereq->nbytesfield after a successful hash operation to determine the amount of data to en/decrypt. So an implementation must not modify the nbytes field.Fixed by a slight rework on the phmac implementation. There is now a new field
async_opin the request context which tracks the (asynch) operation to process. So the 'state' viareq->nbytesis 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]>
And please don't forget to pick Ingo's R-b and T-b tags (or let b4 do the job).
Signature · 5 lines
Mit freundlichen Grüßen / Kind regards Holger Dengler -- IBM Systems, Linux on IBM Z Development [email protected]