Re: [BUGFIX PATCH] staging: ccree: save ciphertext for CTS IV
On Wed, Aug 23, 2017 at 10:47 AM, Stephan Muellerwrote: > Am Mittwoch, 23. August 2017, 09:41:11 CEST schrieb Gilad Ben-Yossef: > > Hi Gilad, > >> >> if (areq) { >> + /* >> + * The crypto API expects us to set the req->info to the last >> + * ciphertext block. For encrypt, simply copy from the result. >> + * For decrypt, we must copy from a saved buffer since this >> + * could be an in-place decryption operation and the src is >> + * lost by this point. >> + */ >> + if (req_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) >> { >> + memcpy(req->info, req_ctx->backup_info, ivsize); >> + kfree(req_ctx->backup_info); >> + } else { >> + scatterwalk_map_and_copy(req->info, req->dst, >> + (req->nbytes - ivsize), >> + ivsize, 0); >> + } >> + >> ablkcipher_request_complete(areq, completion_error); >> return 0; >> } >> @@ -858,7 +876,6 @@ static int ssi_ablkcipher_encrypt(struct >> ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = >> ablkcipher_request_ctx(req); unsigned int ivsize = >> crypto_ablkcipher_ivsize(ablk_tfm); >> >> - req_ctx->backup_info = req->info; >> req_ctx->is_giv = false; >> >> return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, >> req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); >> @@ -871,8 +888,18 @@ static int ssi_ablkcipher_decrypt(struct >> ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = >> ablkcipher_request_ctx(req); unsigned int ivsize = >> crypto_ablkcipher_ivsize(ablk_tfm); >> >> - req_ctx->backup_info = req->info; >> + /* >> + * Allocate and save the last IV sized bytes of the source, which will >> + * be lost in case of in-place decryption and might be needed for CTS. >> + */ >> + req_ctx->backup_info = kmalloc(ivsize, GFP_KERNEL); >> + if (!req_ctx->backup_info) >> + return -ENOMEM; > > Are you sure you do not add a memleak here? You seem to unconditionally > allocate memory but conditionally free it. You are right. I've missed freeing the memory on error code paths. Thank you for catching that. The code involved is badly need of a re-write but I wanted to get the obvious bug fixed first. I will send a revised patch Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [BUGFIX PATCH] staging: ccree: save ciphertext for CTS IV
Am Mittwoch, 23. August 2017, 09:41:11 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > > if (areq) { > + /* > + * The crypto API expects us to set the req->info to the last > + * ciphertext block. For encrypt, simply copy from the result. > + * For decrypt, we must copy from a saved buffer since this > + * could be an in-place decryption operation and the src is > + * lost by this point. > + */ > + if (req_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) { > + memcpy(req->info, req_ctx->backup_info, ivsize); > + kfree(req_ctx->backup_info); > + } else { > + scatterwalk_map_and_copy(req->info, req->dst, > + (req->nbytes - ivsize), > + ivsize, 0); > + } > + > ablkcipher_request_complete(areq, completion_error); > return 0; > } > @@ -858,7 +876,6 @@ static int ssi_ablkcipher_encrypt(struct > ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = > ablkcipher_request_ctx(req); unsigned int ivsize = > crypto_ablkcipher_ivsize(ablk_tfm); > > - req_ctx->backup_info = req->info; > req_ctx->is_giv = false; > > return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, > req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); > @@ -871,8 +888,18 @@ static int ssi_ablkcipher_decrypt(struct > ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = > ablkcipher_request_ctx(req); unsigned int ivsize = > crypto_ablkcipher_ivsize(ablk_tfm); > > - req_ctx->backup_info = req->info; > + /* > + * Allocate and save the last IV sized bytes of the source, which will > + * be lost in case of in-place decryption and might be needed for CTS. > + */ > + req_ctx->backup_info = kmalloc(ivsize, GFP_KERNEL); > + if (!req_ctx->backup_info) > + return -ENOMEM; Are you sure you do not add a memleak here? You seem to unconditionally allocate memory but conditionally free it. Ciao Stephan
[BUGFIX PATCH] staging: ccree: save ciphertext for CTS IV
The crypto API requires saving the last blocks of ciphertext in req->info for use as IV for CTS mode. The ccree driver was not doing this. This patch fixes that. The bug was manifested with cts(cbc(aes)) mode in tcrypt tests. Fixes: 302ef8ebb4b2 ("Add CryptoCell skcipher support") Signed-off-by: Gilad Ben-Yossef--- drivers/staging/ccree/ssi_cipher.c | 31 +-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index af9afea..01011a2 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -24,6 +24,7 @@ #include #include #include +#include #include "ssi_config.h" #include "ssi_driver.h" @@ -697,6 +698,7 @@ static int ssi_blkcipher_complete(struct device *dev, { int completion_error = 0; u32 inflight_counter; + struct ablkcipher_request *req = (struct ablkcipher_request *)areq; ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst); @@ -707,6 +709,22 @@ static int ssi_blkcipher_complete(struct device *dev, ctx_p->drvdata->inflight_counter--; if (areq) { + /* +* The crypto API expects us to set the req->info to the last +* ciphertext block. For encrypt, simply copy from the result. +* For decrypt, we must copy from a saved buffer since this +* could be an in-place decryption operation and the src is +* lost by this point. +*/ + if (req_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) { + memcpy(req->info, req_ctx->backup_info, ivsize); + kfree(req_ctx->backup_info); + } else { + scatterwalk_map_and_copy(req->info, req->dst, +(req->nbytes - ivsize), +ivsize, 0); + } + ablkcipher_request_complete(areq, completion_error); return 0; } @@ -858,7 +876,6 @@ static int ssi_ablkcipher_encrypt(struct ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(req); unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); - req_ctx->backup_info = req->info; req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); @@ -871,8 +888,18 @@ static int ssi_ablkcipher_decrypt(struct ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(req); unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); - req_ctx->backup_info = req->info; + /* +* Allocate and save the last IV sized bytes of the source, which will +* be lost in case of in-place decryption and might be needed for CTS. +*/ + req_ctx->backup_info = kmalloc(ivsize, GFP_KERNEL); + if (!req_ctx->backup_info) + return -ENOMEM; + + scatterwalk_map_and_copy(req_ctx->backup_info, req->src, +(req->nbytes - ivsize), ivsize, 0); req_ctx->is_giv = false; + return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_DECRYPT); } -- 2.1.4