Re: [BUGFIX PATCH v2] staging: ccree: save ciphertext for CTS IV
Am Mittwoch, 23. August 2017, 12:47:36 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > > Thank you for your persistence. It is appreciated :-) > > If I understood correctly what you are referring to than the buffer is not > allocated in this code path (unless I've missed something): Ah, that is what I was missing :-) Thanks for clarification. No further objections ;-) Ciao Stephan
Re: [BUGFIX PATCH v2] staging: ccree: save ciphertext for CTS IV
On Wed, Aug 23, 2017 at 1:03 PM, Stephan Muellerwrote: > Am Mittwoch, 23. August 2017, 11:12:05 CEST schrieb Gilad Ben-Yossef: > > Hi Gilad, > >> 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 >> --- >> >> Changes from v1: >> - Free memory on error path, as pointed out by Stephan Mueller. >> >> drivers/staging/ccree/ssi_cipher.c | 40 >> ++ 1 file changed, 36 insertions(+), 4 >> deletions(-) >> >> diff --git a/drivers/staging/ccree/ssi_cipher.c >> b/drivers/staging/ccree/ssi_cipher.c index af9afea..8d31a93 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); > > Sorry to be persistent, but what about this code path? Here you do not free > it, yet it is allocated. Thank you for your persistence. It is appreciated :-) If I understood correctly what you are referring to than the buffer is not allocated in this code path (unless I've missed something): The buffer is allocated in ssi_ablkcipher_decrypt() which sets req_ctx->gen_ctx.op_type to DRV_CRYPTO_DIRECTION_DECRYPT. The buffer is not allocated in the parallel function (ssi_ablkcipher_encrypt) which matches the else clause of this if statement. >> + } >> + >> ablkcipher_request_complete(areq, completion_error); >> return 0; >> } >> @@ -739,11 +757,13 @@ static int ssi_blkcipher_process( >> if (unlikely(validate_data_size(ctx_p, nbytes))) { >> SSI_LOG_ERR("Unsupported data size %d.\n", nbytes); >> crypto_tfm_set_flags(tfm, CRYPTO_TFM_RES_BAD_BLOCK_LEN); >> - return -EINVAL; >> + rc = -EINVAL; >> + goto exit_process; >> } >> if (nbytes == 0) { >> /* No data to process is valid */ >> - return 0; >> + rc = 0; >> + goto exit_process; >> } >> /*For CTS in case of data size aligned to 16 use CBC mode*/ >> if (((nbytes % AES_BLOCK_SIZE) == 0) && (ctx_p->cipher_mode == >> DRV_CIPHER_CBC_CTS)) { @@ -818,6 +838,9 @@ static int >> ssi_blkcipher_process( >> if (cts_restore_flag != 0) >> ctx_p->cipher_mode = DRV_CIPHER_CBC_CTS; >> >> + if (rc != -EINPROGRESS) >> + kfree(req_ctx->backup_info); >> + >> return rc; >> } >> >> @@ -858,7 +881,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 +893,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. >> +
Re: [BUGFIX PATCH v2] staging: ccree: save ciphertext for CTS IV
Am Mittwoch, 23. August 2017, 11:12:05 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > 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> --- > > Changes from v1: > - Free memory on error path, as pointed out by Stephan Mueller. > > drivers/staging/ccree/ssi_cipher.c | 40 > ++ 1 file changed, 36 insertions(+), 4 > deletions(-) > > diff --git a/drivers/staging/ccree/ssi_cipher.c > b/drivers/staging/ccree/ssi_cipher.c index af9afea..8d31a93 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); Sorry to be persistent, but what about this code path? Here you do not free it, yet it is allocated. > + } > + > ablkcipher_request_complete(areq, completion_error); > return 0; > } > @@ -739,11 +757,13 @@ static int ssi_blkcipher_process( > if (unlikely(validate_data_size(ctx_p, nbytes))) { > SSI_LOG_ERR("Unsupported data size %d.\n", nbytes); > crypto_tfm_set_flags(tfm, CRYPTO_TFM_RES_BAD_BLOCK_LEN); > - return -EINVAL; > + rc = -EINVAL; > + goto exit_process; > } > if (nbytes == 0) { > /* No data to process is valid */ > - return 0; > + rc = 0; > + goto exit_process; > } > /*For CTS in case of data size aligned to 16 use CBC mode*/ > if (((nbytes % AES_BLOCK_SIZE) == 0) && (ctx_p->cipher_mode == > DRV_CIPHER_CBC_CTS)) { @@ -818,6 +838,9 @@ static int > ssi_blkcipher_process( > if (cts_restore_flag != 0) > ctx_p->cipher_mode = DRV_CIPHER_CBC_CTS; > > + if (rc != -EINPROGRESS) > + kfree(req_ctx->backup_info); > + > return rc; > } > > @@ -858,7 +881,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 +893,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); > } Ciao Stephan
[BUGFIX PATCH v2] 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--- Changes from v1: - Free memory on error path, as pointed out by Stephan Mueller. drivers/staging/ccree/ssi_cipher.c | 40 ++ 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index af9afea..8d31a93 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; } @@ -739,11 +757,13 @@ static int ssi_blkcipher_process( if (unlikely(validate_data_size(ctx_p, nbytes))) { SSI_LOG_ERR("Unsupported data size %d.\n", nbytes); crypto_tfm_set_flags(tfm, CRYPTO_TFM_RES_BAD_BLOCK_LEN); - return -EINVAL; + rc = -EINVAL; + goto exit_process; } if (nbytes == 0) { /* No data to process is valid */ - return 0; + rc = 0; + goto exit_process; } /*For CTS in case of data size aligned to 16 use CBC mode*/ if (((nbytes % AES_BLOCK_SIZE) == 0) && (ctx_p->cipher_mode == DRV_CIPHER_CBC_CTS)) { @@ -818,6 +838,9 @@ static int ssi_blkcipher_process( if (cts_restore_flag != 0) ctx_p->cipher_mode = DRV_CIPHER_CBC_CTS; + if (rc != -EINPROGRESS) + kfree(req_ctx->backup_info); + return rc; } @@ -858,7 +881,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 +893,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