Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Horia Geanta <horia.geanta@nxp.com>
To:     Iuliana Prodan <iuliana.prodan@nxp.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Aymen Sghaier <aymen.sghaier@nxp.com>
CC:     "David S. Miller" <davem@davemloft.net>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH v2 04/14] crypto: caam - check key length
Thread-Topic: [PATCH v2 04/14] crypto: caam - check key length
Thread-Index: AQHVPcSq554Fd8UEEU2cDr3CFuYMbg==
Date:   Fri, 19 Jul 2019 14:38:25 +0000
Message-ID: <VI1PR0402MB34855E46247C86307CFE871898CB0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
References: <1563494276-3993-1-git-send-email-iuliana.prodan@nxp.com>
 <1563494276-3993-5-git-send-email-iuliana.prodan@nxp.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fb566b9e-39e8-410b-d40d-08d70c56c1b0
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jul 2019 14:38:25.1217
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: horia.geanta@nxp.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1SPR00MB252
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 7/19/2019 2:58 AM, Iuliana Prodan wrote:=0A=
> Check key length to solve the extra tests that expect -EINVAL to be=0A=
> returned when the key size is not valid.=0A=
> =0A=
> Validated AES keylen for skcipher and ahash.=0A=
> =0A=
Also aead was updated.=0A=
=0A=
> The check_aes_keylen function is added in a common file, to be used=0A=
> also for caam/qi and caam/qi2.=0A=
> =0A=
[...]=0A=
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.=
c=0A=
> index 28d55a0..6ac59b1 100644=0A=
> --- a/drivers/crypto/caam/caamalg.c=0A=
> +++ b/drivers/crypto/caam/caamalg.c=0A=
[...]=0A=
> @@ -683,10 +691,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,=
=0A=
>  {=0A=
>  	struct caam_ctx *ctx =3D crypto_aead_ctx(aead);=0A=
>  	struct device *jrdev =3D ctx->jrdev;=0A=
> +	int err;=0A=
>  =0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
This is no longer needed, check_aes_keylen() catches this case too.=0A=
=0A=
> +	err =3D check_aes_keylen(keylen - 4);=0A=
> +	if (err) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return err;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @"__stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
> @@ -707,10 +722,17 @@ static int rfc4543_setkey(struct crypto_aead *aead,=
=0A=
>  {=0A=
>  	struct caam_ctx *ctx =3D crypto_aead_ctx(aead);=0A=
>  	struct device *jrdev =3D ctx->jrdev;=0A=
> +	int err;=0A=
>  =0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
Same here, check_aes_keylen() handles this case.=0A=
=0A=
> +	err =3D check_aes_keylen(keylen - 4);=0A=
> +	if (err) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return err;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @"__stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
[...]=0A=
> diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caama=
lg_qi.c=0A=
> index 66531d6..46097e3 100644=0A=
> --- a/drivers/crypto/caam/caamalg_qi.c=0A=
> +++ b/drivers/crypto/caam/caamalg_qi.c=0A=
> @@ -385,6 +385,12 @@ static int gcm_setkey(struct crypto_aead *aead,=0A=
>  	struct device *jrdev =3D ctx->jrdev;=0A=
>  	int ret;=0A=
>  =0A=
> +	ret =3D check_aes_keylen(keylen);=0A=
> +	if (ret) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return ret;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
> @@ -483,6 +489,12 @@ static int rfc4106_setkey(struct crypto_aead *aead,=
=0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
Same here, check_aes_keylen() handles this case.=0A=
=0A=
> +	ret =3D check_aes_keylen(keylen - 4);=0A=
> +	if (ret) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return ret;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
> @@ -585,6 +597,12 @@ static int rfc4543_setkey(struct crypto_aead *aead,=
=0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
Same here, check_aes_keylen() handles this case.=0A=
=0A=
> +	ret =3D check_aes_keylen(keylen - 4);=0A=
> +	if (ret) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return ret;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
[...]=0A=
> +static int des_skcipher_setkey(struct crypto_skcipher *skcipher,=0A=
> +			       const u8 *key, unsigned int keylen)=0A=
> +{=0A=
> +	u32 tmp[DES_EXPKEY_WORDS];=0A=
> +=0A=
> +	if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &=0A=
CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI needs to select CRYPTO_DES,=0A=
such that des_ekey is available.=0A=
=0A=
> +	    CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {=0A=
> +		crypto_skcipher_set_flags(skcipher,=0A=
> +					  CRYPTO_TFM_RES_WEAK_KEY);=0A=
> +		return -EINVAL;=0A=
> +	}=0A=
> +=0A=
> +	return skcipher_setkey(skcipher, key, keylen, 0);=0A=
>  }=0A=
>  =0A=
>  static int xts_skcipher_setkey(struct crypto_skcipher *skcipher, const u=
8 *key,=0A=
[...]=0A=
> diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caam=
alg_qi2.c=0A=
> index bc370af..da4abf1 100644=0A=
> --- a/drivers/crypto/caam/caamalg_qi2.c=0A=
> +++ b/drivers/crypto/caam/caamalg_qi2.c=0A=
[...]=0A=
> @@ -817,10 +823,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,=
=0A=
>  {=0A=
>  	struct caam_ctx *ctx =3D crypto_aead_ctx(aead);=0A=
>  	struct device *dev =3D ctx->dev;=0A=
> +	int ret;=0A=
>  =0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
Same here, check_aes_keylen() handles this case.=0A=
=0A=
> +	ret =3D check_aes_keylen(keylen - 4);=0A=
> +	if (ret) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return ret;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
> @@ -911,10 +924,17 @@ static int rfc4543_setkey(struct crypto_aead *aead,=
=0A=
>  {=0A=
>  	struct caam_ctx *ctx =3D crypto_aead_ctx(aead);=0A=
>  	struct device *dev =3D ctx->dev;=0A=
> +	int ret;=0A=
>  =0A=
>  	if (keylen < 4)=0A=
>  		return -EINVAL;=0A=
>  =0A=
Same here, check_aes_keylen() handles this case.=0A=
=0A=
> +	ret =3D check_aes_keylen(keylen - 4);=0A=
> +	if (ret) {=0A=
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return ret;=0A=
> +	}=0A=
> +=0A=
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",=0A=
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);=0A=
>  =0A=
[...]=0A=
> +static int chacha20_skcipher_setkey(struct crypto_skcipher *skcipher,=0A=
> +				    const u8 *key, unsigned int keylen)=0A=
> +{=0A=
> +	return skcipher_setkey(skcipher, key, keylen, 0);=0A=
> +}=0A=
Missing check for key length.=0A=
=0A=
> +=0A=
> +static int des_skcipher_setkey(struct crypto_skcipher *skcipher,=0A=
> +			       const u8 *key, unsigned int keylen)=0A=
> +{=0A=
> +	u32 tmp[DES3_EDE_EXPKEY_WORDS];=0A=
> +	struct crypto_tfm *tfm =3D crypto_skcipher_tfm(skcipher);=0A=
> +=0A=
> +	if (keylen =3D=3D DES3_EDE_KEY_SIZE &&=0A=
> +	    __des3_ede_setkey(tmp, &tfm->crt_flags, key, DES3_EDE_KEY_SIZE)) {=
=0A=
> +		return -EINVAL;=0A=
> +	}=0A=
> +=0A=
> +	if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &=0A=
CRYPTO_DEV_FSL_DPAA2_CAAM needs to select CRYPTO_DES,=0A=
such that __des3_ede_setkey and des_ekey are available.=0A=
=0A=
> +	    CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {=0A=
> +		crypto_skcipher_set_flags(skcipher,=0A=
> +					  CRYPTO_TFM_RES_WEAK_KEY);=0A=
> +		return -EINVAL;=0A=
> +	}=0A=
> +=0A=
> +	return skcipher_setkey(skcipher, key, keylen, 0);=0A=
>  }=0A=
[...]=0A=
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhas=
h.c=0A=
> index 73abefa..2ec4bad 100644=0A=
> --- a/drivers/crypto/caam/caamhash.c=0A=
> +++ b/drivers/crypto/caam/caamhash.c=0A=
> @@ -62,6 +62,7 @@=0A=
>  #include "desc_constr.h"=0A=
>  #include "jr.h"=0A=
>  #include "error.h"=0A=
> +#include "common_if.h"=0A=
>  #include "sg_sw_sec4.h"=0A=
>  #include "key_gen.h"=0A=
>  #include "caamhash_desc.h"=0A=
> @@ -501,6 +502,9 @@ static int axcbc_setkey(struct crypto_ahash *ahash, c=
onst u8 *key,=0A=
>  	struct caam_hash_ctx *ctx =3D crypto_ahash_ctx(ahash);=0A=
>  	struct device *jrdev =3D ctx->jrdev;=0A=
>  =0A=
> +	if (keylen !=3D AES_KEYSIZE_128)=0A=
> +		return -EINVAL;=0A=
> +=0A=
CRYPTO_TFM_RES_BAD_KEY_LEN flag should be set on the error path.=0A=
=0A=
>  	memcpy(ctx->key, key, keylen);=0A=
>  	dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, DMA_TO_DEVICE);=
=0A=
>  	ctx->adata.keylen =3D keylen;=0A=
> @@ -515,6 +519,13 @@ static int acmac_setkey(struct crypto_ahash *ahash, =
const u8 *key,=0A=
>  			unsigned int keylen)=0A=
>  {=0A=
>  	struct caam_hash_ctx *ctx =3D crypto_ahash_ctx(ahash);=0A=
> +	int err;=0A=
> +=0A=
> +	err =3D check_aes_keylen(keylen);=0A=
> +	if (err) {=0A=
> +		crypto_ahash_set_flags(ahash, CRYPTO_TFM_RES_BAD_KEY_LEN);=0A=
> +		return err;=0A=
> +	}=0A=
>  =0A=
>  	/* key is immediate data for all cmac shared descriptors */=0A=
>  	ctx->adata.key_virt =3D key;=0A=
=0A=
Horia=0A=