RE: [PATCH 1/2] crypto: caam - add support for gcm(aes)

[tudor.amba...@freescale.com]

On Thu, 9 Oct 2014 17:54:09 +0300
Tudor Ambarus  wrote:

> + /*
> +  * Job Descriptor and Shared Descriptors
> +  * must all fit into the 64-word Descriptor h/w Buffer
> +  */
> + if (DESC_GCM_DEC_LEN + DESC_JOB_IO_LEN +
> + ctx->enckeylen <= CAAM_DESC_BYTES_MAX)
> + keys_fit_inline = true;

we need to reset the encrypt descriptor's keys_fit_inline setting to false 
before re-evaluating for decrypt.
[TA] Agreed. 

> + /* Galois Counter Mode */
> + {
> + .name = "gcm(aes)",
> + .driver_name = "gcm-aes-caam",
> + .blocksize = 1,
> + .type = CRYPTO_ALG_TYPE_AEAD,
> + .template_aead = {
> + .setkey = gcm_setkey,
> + .setauthsize = gcm_setauthsize,
> + .encrypt = aead_encrypt,
> + .decrypt = aead_decrypt,
> + .givencrypt = NULL,
> + .geniv = "",
> + .ivsize = 12,
> + .maxauthsize = 16,

AES_BLOCK_SIZE
[TA] I think we shall not change the blocksize value to AES_BLOCK_SIZE.
GCM uses a block cipher as a stream cipher. It generates encryption blocks, 
which are then XORed with the plaintext blocks to get the ciphertext. Just as 
with other stream ciphers, flipping a bit in the ciphertext produces a flipped 
bit in the plaintext at the same location.


Thanks,

Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] crypto: caam - add support for rfc4106(gcm(aes))

[tudor.amba...@freescale.com]

On Thu, 9 Oct 2014 17:54:10 +0300
Tudor Ambarus  wrote:

> +static int rfc4106_set_sh_desc(struct crypto_aead *aead)
...
> + /*
> +  * Job Descriptor and Shared Descriptors
> +  * must all fit into the 64-word Descriptor h/w Buffer
> +  */
> + if (DESC_RFC4106_DEC_LEN + DESC_JOB_IO_LEN +
> + ctx->enckeylen <= CAAM_DESC_BYTES_MAX)
> + key_fit_inline = true;

we need to reset encrypt descriptor's keys_fit_inline setting to
false before doing this.

Also, the singular of "keys_fit_inline" is "key_fits_inline", but
I'd prefer we not gratuitously rename the variable from the rest of
the driver's keys_fit_inline for consistency's sake, thanks.
[TA] Agreed.

> + /*
> +  * Job Descriptor and Shared Descriptors
> +  * must all fit into the 64-word Descriptor h/w Buffer
> +  */
> + if (DESC_RFC4106_GIVENC_LEN + DESC_JOB_IO_LEN +
> + ctx->split_key_pad_len + ctx->enckeylen <=
> + CAAM_DESC_BYTES_MAX)
> + key_fit_inline = true;

we need to reset the variable here too.
[TA] Agreed.

> +static int rfc4106_setauthsize(struct crypto_aead *authenc,
> +unsigned int authsize)
> +{
> + struct caam_ctx *ctx = crypto_aead_ctx(authenc);
> +
> + switch (authsize) {
> + case 8:
> + case 12:
> + case 16:
> + break;
> + default:
> + return -EINVAL;
> + }

the h/w can handle more authsizes than that, so we
shouldn't be blocking it from doing so here.

[TA] rfc4106 says that "Implementations MUST support a full-length 16-octet 
ICV, and MAY support 8 or 12 octet ICVs, and MUST NOT support other ICV 
lengths."
Do we want to support other ICV lengths?

> @@ -2601,6 +2986,23 @@ static struct caam_alg_template driver_algs[] = {
>  OP_ALG_AAI_HMAC_PRECOMP,
>   .alg_op = OP_ALG_ALGSEL_SHA512 | OP_ALG_AAI_HMAC,
>   },
> + {
> + .name = "rfc4106(gcm(aes))",
> + .driver_name = "rfc4106-gcm-aes-caam",
> + .blocksize = 1,
> + .type = CRYPTO_ALG_TYPE_AEAD,
> + .template_aead = {
> + .setkey = rfc4106_setkey,
> + .setauthsize = rfc4106_setauthsize,
> + .encrypt = aead_encrypt,
> + .decrypt = aead_decrypt,
> + .givencrypt = aead_givencrypt,
> + .geniv = "",
> + .ivsize = 8,
> + .maxauthsize = 16,

AES_BLOCK_SIZE

[TA] I don't think we should change the blocksize value to AES_BLOCK_SIZE.

Thank you,
Tudor

Thanks,

Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html