Re: [PATCH v2 04/14] crypto: caam - check key length

[Horia Geanta]

On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Check key length to solve the extra tests that expect -EINVAL to be
> returned when the key size is not valid.
> 
> Validated AES keylen for skcipher and ahash.
> 
Also aead was updated.

> The check_aes_keylen function is added in a common file, to be used
> also for caam/qi and caam/qi2.
> 
[...]
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 28d55a0..6ac59b1 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
[...]
> @@ -683,10 +691,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,
>  {
>  	struct caam_ctx *ctx = crypto_aead_ctx(aead);
>  	struct device *jrdev = ctx->jrdev;
> +	int err;
>  
>  	if (keylen < 4)
>  		return -EINVAL;
>  
This is no longer needed, check_aes_keylen() catches this case too.

> +	err = check_aes_keylen(keylen - 4);
> +	if (err) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return err;
> +	}
> +
>  	print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
> @@ -707,10 +722,17 @@ static int rfc4543_setkey(struct crypto_aead *aead,
>  {
>  	struct caam_ctx *ctx = crypto_aead_ctx(aead);
>  	struct device *jrdev = ctx->jrdev;
> +	int err;
>  
>  	if (keylen < 4)
>  		return -EINVAL;
>  
Same here, check_aes_keylen() handles this case.

> +	err = check_aes_keylen(keylen - 4);
> +	if (err) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return err;
> +	}
> +
>  	print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
[...]
> diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
> index 66531d6..46097e3 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -385,6 +385,12 @@ static int gcm_setkey(struct crypto_aead *aead,
>  	struct device *jrdev = ctx->jrdev;
>  	int ret;
>  
> +	ret = check_aes_keylen(keylen);
> +	if (ret) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return ret;
> +	}
> +
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
> @@ -483,6 +489,12 @@ static int rfc4106_setkey(struct crypto_aead *aead,
>  	if (keylen < 4)
>  		return -EINVAL;
>  
Same here, check_aes_keylen() handles this case.

> +	ret = check_aes_keylen(keylen - 4);
> +	if (ret) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return ret;
> +	}
> +
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
> @@ -585,6 +597,12 @@ static int rfc4543_setkey(struct crypto_aead *aead,
>  	if (keylen < 4)
>  		return -EINVAL;
>  
Same here, check_aes_keylen() handles this case.

> +	ret = check_aes_keylen(keylen - 4);
> +	if (ret) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return ret;
> +	}
> +
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
[...]
> +static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
> +			       const u8 *key, unsigned int keylen)
> +{
> +	u32 tmp[DES_EXPKEY_WORDS];
> +
> +	if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &
CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI needs to select CRYPTO_DES,
such that des_ekey is available.

> +	    CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {
> +		crypto_skcipher_set_flags(skcipher,
> +					  CRYPTO_TFM_RES_WEAK_KEY);
> +		return -EINVAL;
> +	}
> +
> +	return skcipher_setkey(skcipher, key, keylen, 0);
>  }
>  
>  static int xts_skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
[...]
> diff --git a/drivers/crypto/caam/caamalg_qi2.c b/drivers/crypto/caam/caamalg_qi2.c
> index bc370af..da4abf1 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
[...]
> @@ -817,10 +823,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,
>  {
>  	struct caam_ctx *ctx = crypto_aead_ctx(aead);
>  	struct device *dev = ctx->dev;
> +	int ret;
>  
>  	if (keylen < 4)
>  		return -EINVAL;
>  
Same here, check_aes_keylen() handles this case.

> +	ret = check_aes_keylen(keylen - 4);
> +	if (ret) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return ret;
> +	}
> +
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
> @@ -911,10 +924,17 @@ static int rfc4543_setkey(struct crypto_aead *aead,
>  {
>  	struct caam_ctx *ctx = crypto_aead_ctx(aead);
>  	struct device *dev = ctx->dev;
> +	int ret;
>  
>  	if (keylen < 4)
>  		return -EINVAL;
>  
Same here, check_aes_keylen() handles this case.

> +	ret = check_aes_keylen(keylen - 4);
> +	if (ret) {
> +		crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return ret;
> +	}
> +
>  	print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>  			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>  
[...]
> +static int chacha20_skcipher_setkey(struct crypto_skcipher *skcipher,
> +				    const u8 *key, unsigned int keylen)
> +{
> +	return skcipher_setkey(skcipher, key, keylen, 0);
> +}
Missing check for key length.

> +
> +static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
> +			       const u8 *key, unsigned int keylen)
> +{
> +	u32 tmp[DES3_EDE_EXPKEY_WORDS];
> +	struct crypto_tfm *tfm = crypto_skcipher_tfm(skcipher);
> +
> +	if (keylen == DES3_EDE_KEY_SIZE &&
> +	    __des3_ede_setkey(tmp, &tfm->crt_flags, key, DES3_EDE_KEY_SIZE)) {
> +		return -EINVAL;
> +	}
> +
> +	if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &
CRYPTO_DEV_FSL_DPAA2_CAAM needs to select CRYPTO_DES,
such that __des3_ede_setkey and des_ekey are available.

> +	    CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {
> +		crypto_skcipher_set_flags(skcipher,
> +					  CRYPTO_TFM_RES_WEAK_KEY);
> +		return -EINVAL;
> +	}
> +
> +	return skcipher_setkey(skcipher, key, keylen, 0);
>  }
[...]
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 73abefa..2ec4bad 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
> @@ -62,6 +62,7 @@
>  #include "desc_constr.h"
>  #include "jr.h"
>  #include "error.h"
> +#include "common_if.h"
>  #include "sg_sw_sec4.h"
>  #include "key_gen.h"
>  #include "caamhash_desc.h"
> @@ -501,6 +502,9 @@ static int axcbc_setkey(struct crypto_ahash *ahash, const u8 *key,
>  	struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
>  	struct device *jrdev = ctx->jrdev;
>  
> +	if (keylen != AES_KEYSIZE_128)
> +		return -EINVAL;
> +
CRYPTO_TFM_RES_BAD_KEY_LEN flag should be set on the error path.

>  	memcpy(ctx->key, key, keylen);
>  	dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, DMA_TO_DEVICE);
>  	ctx->adata.keylen = keylen;
> @@ -515,6 +519,13 @@ static int acmac_setkey(struct crypto_ahash *ahash, const u8 *key,
>  			unsigned int keylen)
>  {
>  	struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
> +	int err;
> +
> +	err = check_aes_keylen(keylen);
> +	if (err) {
> +		crypto_ahash_set_flags(ahash, CRYPTO_TFM_RES_BAD_KEY_LEN);
> +		return err;
> +	}
>  
>  	/* key is immediate data for all cmac shared descriptors */
>  	ctx->adata.key_virt = key;

Horia