Re: [PATCH 2/3] crypto: inside-secure - added support for rfc3686(ctr(aes))

[Antoine Tenart <antoine.tenart@bootlin.com>]

Hi Pascal,

On Fri, Jul 05, 2019 at 08:49:23AM +0200, Pascal van Leeuwen wrote:
> Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>

Could you add a commit message?

> -		/* H/W capabilities selection */
> -		val = EIP197_FUNCTION_RSVD;
> -		val |= EIP197_PROTOCOL_ENCRYPT_ONLY | EIP197_PROTOCOL_HASH_ONLY;
> -		val |= EIP197_PROTOCOL_ENCRYPT_HASH | EIP197_PROTOCOL_HASH_DECRYPT;
> -		val |= EIP197_ALG_DES_ECB | EIP197_ALG_DES_CBC;
> -		val |= EIP197_ALG_3DES_ECB | EIP197_ALG_3DES_CBC;
> -		val |= EIP197_ALG_AES_ECB | EIP197_ALG_AES_CBC;
> -		val |= EIP197_ALG_MD5 | EIP197_ALG_HMAC_MD5;
> -		val |= EIP197_ALG_SHA1 | EIP197_ALG_HMAC_SHA1;
> -		val |= EIP197_ALG_SHA2 | EIP197_ALG_HMAC_SHA2;
> -		writel(val, EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));
> +		/* H/W capabilities selection: just enable everything */
> +		writel(EIP197_FUNCTION_ALL,
> +		       EIP197_PE(priv) + EIP197_PE_EIP96_FUNCTION_EN(pe));

This should be in a separate patch. I'm also not sure about it, as
controlling exactly what algs are enabled in the h/w could prevent
misconfiguration issues in the control descriptors.

> @@ -62,9 +63,9 @@ static void safexcel_skcipher_token(struct safexcel_cipher_ctx *ctx, u8 *iv,
>  				    u32 length)
> -	if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC) {
> +	if (ctx->mode != CONTEXT_CONTROL_CRYPTO_MODE_ECB) {

I think it's better for maintenance and readability to have something
like:

  if (ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CBC ||
      ctx->mode == CONTEXT_CONTROL_CRYPTO_MODE_CTR_LOAD)

> +struct safexcel_alg_template safexcel_alg_ctr_aes = {
> +	.type = SAFEXCEL_ALG_TYPE_SKCIPHER,

Same comment as in patch 1 about the .engines member of the struct.

> +	.alg.skcipher = {
> +		.setkey = safexcel_skcipher_aesctr_setkey,
> +		.encrypt = safexcel_ctr_aes_encrypt,
> +		.decrypt = safexcel_ctr_aes_decrypt,
> +		/* Add 4 to include the 4 byte nonce! */
> +		.min_keysize = AES_MIN_KEY_SIZE + 4,
> +		.max_keysize = AES_MAX_KEY_SIZE + 4,

You could use CTR_RFC3686_NONCE_SIZE here (maybe in other places in the
patch as well).

> +		.ivsize = 8,

And CTR_RFC3686_IV_SIZE here.

Thanks!
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com