Date:   Wed, 23 Jan 2019 15:33:02 +0100
From:   Sascha Hauer <s.hauer@pengutronix.de>
To:     linux-crypto@vger.kernel.org
Cc:     Horia =?utf-8?Q?Geant=C4=83?= <horia.geanta@nxp.com>,
        Aymen Sghaier <aymen.sghaier@nxp.com>, kernel@pengutronix.de,
        stable@vger.kernel.org
Subject: Re: [PATCH] crypto: caam - fix setting IV after decrypt
Message-ID: <20190123143302.zw7vugj6hvecjifg@pengutronix.de>
References: <20181207113123.3394-1-s.hauer@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181207113123.3394-1-s.hauer@pengutronix.de>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

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 |