Re: [PATCH 3/7] crypto: ccree: add ablkcipher support

[Corentin Labbe]

On Thu, Jan 11, 2018 at 09:17:10AM +0000, Gilad Ben-Yossef wrote:
> Add CryptoCell ablkcipher support
> 

Hello

I have some minor comments:

ablkcipher is deprecated, so you need to use skcipher instead.

> Signed-off-by: Gilad Ben-Yossef <gilad@xxxxxxxxxxxxx>
> ---
>  drivers/crypto/ccree/Makefile        |    2 +-
>  drivers/crypto/ccree/cc_buffer_mgr.c |  125 ++++
>  drivers/crypto/ccree/cc_buffer_mgr.h |   10 +
>  drivers/crypto/ccree/cc_cipher.c     | 1167 ++++++++++++++++++++++++++++++++++
>  drivers/crypto/ccree/cc_cipher.h     |   74 +++
>  drivers/crypto/ccree/cc_driver.c     |   11 +
>  drivers/crypto/ccree/cc_driver.h     |    2 +
>  7 files changed, 1390 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/crypto/ccree/cc_cipher.c
>  create mode 100644 drivers/crypto/ccree/cc_cipher.h
> 
[...]
> +
> +struct tdes_keys {
> +	u8	key1[DES_KEY_SIZE];
> +	u8	key2[DES_KEY_SIZE];
> +	u8	key3[DES_KEY_SIZE];
> +};
> +
> +static const u8 zero_buff[] = {	0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> +				0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> +				0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> +				0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0};
> +

This constant is used nowhere.

> +/* The function verifies that tdes keys are not weak.*/
> +static int cc_verify_3des_keys(const u8 *key, unsigned int keylen)
> +{
> +	struct tdes_keys *tdes_key = (struct tdes_keys *)key;
> +
> +	/* verify key1 != key2 and key3 != key2*/
> +	if ((memcmp((u8 *)tdes_key->key1, (u8 *)tdes_key->key2,
> +		    sizeof(tdes_key->key1)) == 0) ||
> +	    (memcmp((u8 *)tdes_key->key3, (u8 *)tdes_key->key2,
> +		    sizeof(tdes_key->key3)) == 0)) {
> +		return -ENOEXEC;
> +	}
> +
> +	return 0;
> +}

All driver testing 3des key also use des_ekey()

[...]
> +static void cc_cipher_complete(struct device *dev, void *cc_req, int err)
> +{
> +	struct ablkcipher_request *areq = (struct ablkcipher_request *)cc_req;
> +	struct scatterlist *dst = areq->dst;
> +	struct scatterlist *src = areq->src;
> +	struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(areq);
> +	struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
> +	unsigned int ivsize = crypto_ablkcipher_ivsize(tfm);
> +	struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
> +
> +	cc_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);
> +	kfree(req_ctx->iv);

kzfree for all stuff with IV/key

[...]
> +
> +#ifdef CRYPTO_TFM_REQ_HW_KEY
> +
> +static inline bool cc_is_hw_key(struct crypto_tfm *tfm)
> +{
> +	return (crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_HW_KEY);
> +}
> +
> +#else
> +
> +struct arm_hw_key_info {
> +	int hw_key1;
> +	int hw_key2;
> +};
> +
> +static inline bool cc_is_hw_key(struct crypto_tfm *tfm)
> +{
> +	return false;
> +}
> +
> +#endif /* CRYPTO_TFM_REQ_HW_KEY */

I see nowhere any use/documentation of CRYPTO_TFM_REQ_HW_KEY, so a cleaning could be done

Regards