[PATCH] crypto: caam - fix setting IV after decrypt

[PATCH] crypto: caam - fix setting IV after decrypt

[Sascha Hauer]

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

Re: [PATCH] crypto: caam - fix setting IV after decrypt

[Horia Geanta]

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

Re: [PATCH] crypto: caam - fix setting IV after decrypt

[Herbert Xu]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: caam - fix setting IV after decrypt

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |