The crypto API wants the updated IV in req->info after decryption. The updated IV used to be copied correctly to req->info after running the decryption job. Since 115957bb3e59 this is done before running the job so instead of the updated IV only the unmodified input IV is given back to the crypto API.
This was observed running the gcm(aes) selftest which internally uses ctr(aes) implemented by the CAAM engine.
Fixes: 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de Cc: stable@vger.kernel.org --- drivers/crypto/caam/caamalg.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 869f092432de..c05c7938439c 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -917,10 +917,10 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, { struct skcipher_request *req = context; struct skcipher_edesc *edesc; -#ifdef DEBUG struct crypto_skcipher *skcipher = crypto_skcipher_reqtfm(req); int ivsize = crypto_skcipher_ivsize(skcipher);
+#ifdef DEBUG dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err); #endif
@@ -937,6 +937,14 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, edesc->dst_nents > 1 ? 100 : req->cryptlen, 1);
skcipher_unmap(jrdev, edesc, req); + + /* + * The crypto API expects us to set the IV (req->iv) to the last + * ciphertext block. + */ + scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize, + ivsize, 0); + kfree(edesc);
skcipher_request_complete(req, err); @@ -1588,13 +1596,6 @@ static int skcipher_decrypt(struct skcipher_request *req) if (IS_ERR(edesc)) return PTR_ERR(edesc);
- /* - * The crypto API expects us to set the IV (req->iv) to the last - * ciphertext block. - */ - scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize, - ivsize, 0); - /* Create and submit job descriptor*/ init_skcipher_job(req, edesc, false); desc = edesc->hw_desc;
On 12/7/2018 1:32 PM, Sascha Hauer wrote:
The crypto API wants the updated IV in req->info after decryption. The updated IV used to be copied correctly to req->info after running the decryption job. Since 115957bb3e59 this is done before running the job so instead of the updated IV only the unmodified input IV is given back to the crypto API.
Saving IV before running the decryption was done to address in-place cbc decryption - when the last block is overwritten with plaintext before having the chance to copy it.
Current IV update scheme is ok for cbc (IV <- last ciphertext block), however for ctr the counter should be saved instead; this has to be addressed, but IMHO is not the root cause for gcm failure.
This was observed running the gcm(aes) selftest which internally uses ctr(aes) implemented by the CAAM engine.
I don't see how this patch solves the gcm case you are mentioning, i.e. gcm_base(ctr-aes-caam,ghash-generic). The IV, updated by ctr(aes) CAAM implementation, doesn't seem to be used afterwards - in ghash from gcm SW implementation (crypto/gcm.c).
Considering lack of HW coherency on i.MX, I would rather check whether cacheline sharing is the culprit - see previous discussion: https://patchwork.kernel.org/patch/9801965/
Thanks, Horia
Horia Geanta horia.geanta@nxp.com wrote:
On 12/7/2018 1:32 PM, Sascha Hauer wrote:
The crypto API wants the updated IV in req->info after decryption. The updated IV used to be copied correctly to req->info after running the decryption job. Since 115957bb3e59 this is done before running the job so instead of the updated IV only the unmodified input IV is given back to the crypto API.
Saving IV before running the decryption was done to address in-place cbc decryption - when the last block is overwritten with plaintext before having the chance to copy it.
The API expects the IV to be set to the next IV value so that chaining can be performed. This can mean different things depending on the algorithm.
Cheers,
Horia,
On Fri, Dec 07, 2018 at 12:31:23PM +0100, Sascha Hauer wrote:
The crypto API wants the updated IV in req->info after decryption. The updated IV used to be copied correctly to req->info after running the decryption job. Since 115957bb3e59 this is done before running the job so instead of the updated IV only the unmodified input IV is given back to the crypto API.
This was observed running the gcm(aes) selftest which internally uses ctr(aes) implemented by the CAAM engine.
Fixes: 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating")
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de Cc: stable@vger.kernel.org
drivers/crypto/caam/caamalg.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c index 869f092432de..c05c7938439c 100644 --- a/drivers/crypto/caam/caamalg.c +++ b/drivers/crypto/caam/caamalg.c @@ -937,6 +937,14 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err, edesc->dst_nents > 1 ? 100 : req->cryptlen, 1); skcipher_unmap(jrdev, edesc, req);
- /*
* The crypto API expects us to set the IV (req->iv) to the last* ciphertext block.*/- scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
ivsize, 0);
I was wrong. It's not adding the scatterwalk_map_and_copy() here which fixes gcm(aes) selftest. In fact, this has not to be done.
@@ -1588,13 +1596,6 @@ static int skcipher_decrypt(struct skcipher_request *req) if (IS_ERR(edesc)) return PTR_ERR(edesc);
- /*
* The crypto API expects us to set the IV (req->iv) to the last* ciphertext block.*/- scatterwalk_map_and_copy(req->iv, req->src, req->cryptlen - ivsize,
ivsize, 0);
It's the removal of the scatterwalk_map_and_copy() here which fixes things. With the above the initialization vector which gets passed in is overwritten.
Now I don't know enough of the crypto stuff to judge if overwriting the IV always has to be removed or just in some cases, but as a matter of fact removing these lines fixes the gcm(aes) selftest on i.MX6. From 115957bb3e59 ("crypto: caam - fix IV DMA mapping and updating") insmodding tcrypt fails with:
alg: aead: decryption failed on test 1 for gcm_base(ctr-aes-caam,ghash-generic): ret=74 alg: aead: Failed to load transform for gcm(aes): -2 alg: aead: Failed to load transform for rfc4106(gcm(aes)): -2 alg: aead: Failed to load transform for rfc4543(gcm(aes)): -2
With the overwriting removed it works again.
Horia, does this make sense to you or is there more that is wrong here?
Sascha
linux-stable-mirror@lists.linaro.org
-
Herbert Xu -
Horia Geanta -
Sascha Hauer