Re: [PATCH v2 6/7] crypto: omap-aes: Add support for GCM mode

Hi Herbert,
On Wednesday 08 July 2015 09:48 AM, Herbert Xu wrote:

On Tue, Jul 07, 2015 at 09:01:48PM +0530, Lokesh Vutla wrote:

quoted

+static int omap_aes_gcm_copy_buffers(struct omap_aes_dev *dd,
+				     struct aead_request *req)

[..snip..]

quoted

+static int do_encrypt_iv(struct aead_request *req, u32 *tag)
+{
+	struct scatterlist iv_sg;
+	struct ablkcipher_request *ablk_req;
+	struct crypto_ablkcipher *tfm;
+	struct tcrypt_result result;
+	struct omap_aes_ctx *ctx = crypto_aead_ctx(crypto_aead_reqtfm(req));
+	int ret = 0;
+
+	tfm = crypto_alloc_ablkcipher("ctr(aes)", 0, 0);

Ugh, you cannot allocate crypto transforms in the data path.  You
should allocate it in init instead.  Also using ctr(aes) is overkill.
Just use aes and do the xor by hand.

quoted

+static int omap_aes_gcm_crypt(struct aead_request *req, unsigned long mode)
+{
+	struct omap_aes_ctx *ctx = crypto_aead_ctx(crypto_aead_reqtfm(req));
+	struct omap_aes_reqctx *rctx = aead_request_ctx(req);
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+	unsigned int authlen = crypto_aead_authsize(aead);
+	struct omap_aes_dev *dd;
+	__be32 counter = cpu_to_be32(1);
+	int err;
+
+	memset(ctx->auth_tag, 0, sizeof(ctx->auth_tag));

The ctx is shared memory and you must not write to it as multiple
requests can be called on the same tfm.  Use rctx instead.

May be a dumb question. 
If you don't mind can you elaborate more on the usage of rctx and ctx
in the driver?

Thanks and regards,
Lokesh

quoted

+	memcpy(req->iv + 12, &counter, 4);

The IV is only 12 bytes long so you're corrupting memory here.
You should use rctx here too.

quoted

+	if (req->assoclen + req->cryptlen == 0) {
+		scatterwalk_map_and_copy(ctx->auth_tag, req->dst, 0, authlen,
+					 1);
+		return 0;
+	}

How can this be right? Did you enable the selftest?

Cheers,