linux-kernel - Re: [PATCH 03/14] crypto: caam - update IV only when crypto operation succeeds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <VI1PR0402MB34855E675A58E1221ACE7B9498C80@VI1PR0402MB3485.eurprd04.prod.outlook.com>
Date:   Thu, 18 Jul 2019 14:59:37 +0000
From:   Horia Geanta <horia.geanta@....com>
To:     Iuliana Prodan <iuliana.prodan@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Aymen Sghaier <aymen.sghaier@....com>
CC:     "David S. Miller" <davem@...emloft.net>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH 03/14] crypto: caam - update IV only when crypto operation
 succeeds

On 7/18/2019 5:45 PM, Iuliana Prodan wrote:
> From: Horia Geantă <horia.geanta@....com>
> 
> skcipher encryption might fail and in some cases, like (invalid) input
> length smaller then block size, updating the IV would lead to panic
> due to copying from a negative offset (req->cryptlen - ivsize).
> 
The commit message is no longer in sync with the code base.

More exactly, after commit
334d37c9e263 ("crypto: caam - update IV using HW support")
there shouldn't be any panic, only a useless IV copy in case HW issued
an error.

> Signed-off-by: Horia Geantă <horia.geanta@....com>
> Signed-off-by: Iuliana Prodan <iuliana.prodan@....com>
> ---
>  drivers/crypto/caam/caamalg.c     | 5 ++---
>  drivers/crypto/caam/caamalg_qi.c  | 4 +++-
>  drivers/crypto/caam/caamalg_qi2.c | 8 ++++++--
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 06b4f2d..28d55a0 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -990,10 +990,9 @@ static void skcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
>  	 * ciphertext block (CBC mode) or last counter (CTR mode).
>  	 * This is used e.g. by the CTS mode.
>  	 */
> -	if (ivsize) {
> +	if (ivsize && !ecode) {
>  		memcpy(req->iv, (u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
>  		       ivsize);
> -
>  		print_hex_dump_debug("dstiv  @"__stringify(__LINE__)": ",
>  				     DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
>  				     edesc->src_nents > 1 ? 100 : ivsize, 1);
> @@ -1030,7 +1029,7 @@ static void skcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
>  	 * ciphertext block (CBC mode) or last counter (CTR mode).
>  	 * This is used e.g. by the CTS mode.
>  	 */
> -	if (ivsize) {
> +	if (ivsize && !ecode) {
>  		memcpy(req->iv, (u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
>  		       ivsize);
>  
> diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
> index ab263b1..66531d6 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -1201,7 +1201,9 @@ static void skcipher_done(struct caam_drv_req *drv_req, u32 status)
>  	 * ciphertext block (CBC mode) or last counter (CTR mode).
>  	 * This is used e.g. by the CTS mode.
>  	 */
> -	memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> +	if (!ecode)
> +		memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +		       ivsize);
>  
>  	qi_cache_free(edesc);
>  	skcipher_request_complete(req, ecode);
> diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
> index 2681581..bc370af 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
> @@ -1358,7 +1358,9 @@ static void skcipher_encrypt_done(void *cbk_ctx, u32 status)
>  	 * ciphertext block (CBC mode) or last counter (CTR mode).
>  	 * This is used e.g. by the CTS mode.
>  	 */
> -	memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> +	if (!ecode)
> +		memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +		       ivsize);
>  
>  	qi_cache_free(edesc);
>  	skcipher_request_complete(req, ecode);
> @@ -1394,7 +1396,9 @@ static void skcipher_decrypt_done(void *cbk_ctx, u32 status)
>  	 * ciphertext block (CBC mode) or last counter (CTR mode).
>  	 * This is used e.g. by the CTS mode.
>  	 */
> -	memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> +	if (!ecode)
> +		memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +		       ivsize);
>  
>  	qi_cache_free(edesc);
>  	skcipher_request_complete(req, ecode);
>