diff mbox

[RFC] how to handle AAD copy operation for algif_aead

Message ID 13406914.zeok8basum@positron.chronox.de (mailing list archive)
State RFC
Delegated to: Herbert Xu
Headers show

Commit Message

Stephan Mueller Feb. 24, 2017, 1:24 p.m. UTC 
Hi Herbert et al,

attached are two patches where each patch has a different approach to copy the 
AAD in the algif_aead operation. I would like to hear your opinion which 
approach should be taken.

The patch 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_separate.patch 
simply copies the AAD over from TX SGL to RX SGL. The pro is that the patch is 
small. The con is that this approach does *not* provide an in-place crypto 
operation.

The patch 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_inplace.patch copies 
the AAD and the PT/CT from TX SGL into the RX SGL. In addition, this patch 
chains the SGL with the tag value part present in the TX SGL to the RX SGL in 
case of decryption. This implies that we have an in-place cipher operation 
operating in the RX SGL. Though, the patch is significantly larger.

(note: the patches are NOT meant for inclusion, but only for discussion -- yet 
both code parts are fully tested with by test framework in libkcapi).

Ciao
Stephan

Comments

Herbert Xu March 9, 2017, 10 a.m. UTC | #1 
On Fri, Feb 24, 2017 at 02:24:47PM +0100, Stephan Müller wrote:
> Hi Herbert et al,
> 
> attached are two patches where each patch has a different approach to copy the 
> AAD in the algif_aead operation. I would like to hear your opinion which 
> approach should be taken.
> 
> The patch 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_separate.patch 
> simply copies the AAD over from TX SGL to RX SGL. The pro is that the patch is 
> small. The con is that this approach does *not* provide an in-place crypto 
> operation.

I prefer this patch with the proviso that it copy the whole thing
instead of just the AD.  That way you can just feed the dst memory
to crypto_aead for in-place operation.  Of course you have to mangle
the tag data onto the dst SG list for decryption but it shouldn't
be too hard.

Thanks,
Stephan Mueller March 9, 2017, 10:02 a.m. UTC | #2 
Am Donnerstag, 9. März 2017, 11:00:11 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Feb 24, 2017 at 02:24:47PM +0100, Stephan Müller wrote:
> > Hi Herbert et al,
> > 
> > attached are two patches where each patch has a different approach to copy
> > the AAD in the algif_aead operation. I would like to hear your opinion
> > which approach should be taken.
> > 
> > The patch 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_separate.patch
> > simply copies the AAD over from TX SGL to RX SGL. The pro is that the
> > patch is small. The con is that this approach does *not* provide an
> > in-place crypto operation.
> 
> I prefer this patch with the proviso that it copy the whole thing
> instead of just the AD.  That way you can just feed the dst memory
> to crypto_aead for in-place operation.  Of course you have to mangle
> the tag data onto the dst SG list for decryption but it shouldn't
> be too hard.

I thought that is exactly the second patch. It copies the entire data to the 
dst SGL and extends the SGL with the tag in case of decryption.

Ciao
Stephan
Herbert Xu March 9, 2017, 10:05 a.m. UTC | #3 
On Thu, Mar 09, 2017 at 11:02:41AM +0100, Stephan Müller wrote:
>
> > > The patch 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_separate.patch
> > > simply copies the AAD over from TX SGL to RX SGL. The pro is that the
> > > patch is small. The con is that this approach does *not* provide an
> > > in-place crypto operation.
> > 
> > I prefer this patch with the proviso that it copy the whole thing
> > instead of just the AD.  That way you can just feed the dst memory
> > to crypto_aead for in-place operation.  Of course you have to mangle
> > the tag data onto the dst SG list for decryption but it shouldn't
> > be too hard.
> 
> I thought that is exactly the second patch. It copies the entire data to the 
> dst SGL and extends the SGL with the tag in case of decryption.

Are you sure? The patch says:

+       /* copy AAD from src to dst */
+       err = crypto_aead_copy_sgl(ctx->null, areq->tsgl,
+                                  areq->first_rsgl.sgl.sg, ctx->aead_assoclen);

Which seems to only copy the AD.

Cheers,
Stephan Mueller March 9, 2017, 10:08 a.m. UTC | #4 
Am Donnerstag, 9. März 2017, 11:05:54 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Mar 09, 2017 at 11:02:41AM +0100, Stephan Müller wrote:
> > > > The patch
> > > > 0001-crypto-algif_aead-copy-AAD-from-src-to-dst_separate.patch
> > > > simply copies the AAD over from TX SGL to RX SGL. The pro is that the
> > > > patch is small. The con is that this approach does *not* provide an
> > > > in-place crypto operation.
> > > 
> > > I prefer this patch with the proviso that it copy the whole thing
> > > instead of just the AD.  That way you can just feed the dst memory
> > > to crypto_aead for in-place operation.  Of course you have to mangle
> > > the tag data onto the dst SG list for decryption but it shouldn't
> > > be too hard.
> > 
> > I thought that is exactly the second patch. It copies the entire data to
> > the dst SGL and extends the SGL with the tag in case of decryption.
> 
> Are you sure? The patch says:
> 
> +       /* copy AAD from src to dst */
> +       err = crypto_aead_copy_sgl(ctx->null, areq->tsgl,
> +                                  areq->first_rsgl.sgl.sg,
> ctx->aead_assoclen);
> 
> Which seems to only copy the AD.

This is the first patch (0001-crypto-algif_aead-copy-AAD-from-src-to-
dst_separate.patch).

The second alternative patch (0001-crypto-algif_aead-copy-AAD-from-src-to-
dst_inplace.patch) does:

+	if (ctx->enc) {
+		/* Copy AAD || PT to RX SGL buffer for in-place operation. */
+		err = crypto_aead_copy_sgl(ctx->null, tsgl->sg,
+					   areq->first_rsgl.sgl.sg, processed);
+		if (err)
+			goto free;
+		aead_pull_tsgl(sk, processed, NULL, 0);
+	} else {
+		/* Copy AAD || CT to RX SGL buffer for in-place operation. */
+		err = crypto_aead_copy_sgl(ctx->null, tsgl->sg,
+					   areq->first_rsgl.sgl.sg, outlen);
+		if (err)
+			goto free;
+
+		/* Create TX SGL for tag and chain it to RX SGL. */
+		areq->tsgl_entries = aead_count_tsgl(sk, processed);
+		if (!areq->tsgl_entries)
+			areq->tsgl_entries = 1;
+		areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) *
+					      areq->tsgl_entries,
+					  GFP_KERNEL);
+		if (!areq->tsgl) {
+			err = -ENOMEM;
+			goto free;
+		}
+		sg_init_table(areq->tsgl, areq->tsgl_entries);
+
+		/* Release TX SGL, except for tag data. */
+		aead_pull_tsgl(sk, processed, areq->tsgl, processed - as);
+
+		/* chain the areq TX SGL holding the tag with RX SGL */
+		if (!last_rsgl) {
+			/* no RX SGL present (e.g. only authentication) */
+			sg_init_table(areq->first_rsgl.sgl.sg, 2);
+			sg_chain(areq->first_rsgl.sgl.sg, 2, areq->tsgl);
+		} else {
+			/* RX SGL present */
+			struct af_alg_sgl *sgl_prev = &last_rsgl->sgl;
+
+			sg_unmark_end(sgl_prev->sg + sgl_prev->npages - 1);
+			sg_chain(sgl_prev->sg, sgl_prev->npages + 1, areq->tsgl);
+		}
 	}

Ciao
Stephan
Herbert Xu March 9, 2017, 10:23 a.m. UTC | #5 
On Thu, Mar 09, 2017 at 11:08:32AM +0100, Stephan Müller wrote:
>
> This is the first patch (0001-crypto-algif_aead-copy-AAD-from-src-to-
> dst_separate.patch).
> 
> The second alternative patch (0001-crypto-algif_aead-copy-AAD-from-src-to-
> dst_inplace.patch) does:

I see.  Yes that looks good.

Thanks,
Stephan Mueller March 9, 2017, 10:27 a.m. UTC | #6 
Am Donnerstag, 9. März 2017, 11:23:16 CET schrieb Herbert Xu:

Hi Herbert,

> I see.  Yes that looks good.

Thank you. I will provide an official patch after the discussion about the 
memory handling is completed.

As the memory handling patch should be considered for stable whereas the AAD 
copy patch should not, I would therefore recommend completing the memory 
handling discussion first.

Ciao
Stephan
diff mbox

Patch

From cbd8a171c56008ce4932de9f0a54926279c6061d Mon Sep 17 00:00:00 2001
From: Stephan Mueller <smueller@chronox.de>
Date: Wed, 22 Feb 2017 17:22:02 +0100
Subject: [PATCH] crypto: algif_aead - copy AAD from src to dst

Use the NULL cipher to copy the AAD from the TX SGL to the RX SGL.

The required null cipher is allocated when allocating other components
used by algif_aead and released when those components are deallocated.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/Kconfig      |  2 ++
 crypto/algif_aead.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 5a51b87..bfa531d 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -1735,6 +1735,8 @@  config CRYPTO_USER_API_AEAD
 	tristate "User-space interface for AEAD cipher algorithms"
 	depends on NET
 	select CRYPTO_AEAD
+	select CRYPTO_BLKCIPHER
+	select CRYPTO_NULL
 	select CRYPTO_USER_API
 	help
 	  This option enables the user-spaces interface for AEAD
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 050a866..98c988b 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -30,6 +30,8 @@ 
 #include <crypto/internal/aead.h>
 #include <crypto/scatterwalk.h>
 #include <crypto/if_alg.h>
+#include <crypto/skcipher.h>
+#include <crypto/null.h>
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/kernel.h>
@@ -82,6 +84,7 @@  struct aead_ctx {
 
 	unsigned int len;	/* Length of allocated memory for this struct */
 	struct crypto_aead *aead_tfm;
+	struct crypto_skcipher *null;
 };
 
 static DECLARE_WAIT_QUEUE_HEAD(aead_aio_finish_wait);
@@ -559,6 +562,20 @@  static void aead_async_cb(struct crypto_async_request *_req, int err)
 	wake_up_interruptible(&aead_aio_finish_wait);
 }
 
+static int crypto_aead_copy_sgl(struct crypto_skcipher *null,
+				struct scatterlist *src,
+				struct scatterlist *dst, unsigned int len)
+{
+	SKCIPHER_REQUEST_ON_STACK(skreq, null);
+
+	skcipher_request_set_tfm(skreq, null);
+	skcipher_request_set_callback(skreq, CRYPTO_TFM_REQ_MAY_BACKLOG,
+				      NULL, NULL);
+	skcipher_request_set_crypt(skreq, src, dst, len, NULL);
+
+	return crypto_skcipher_encrypt(skreq);
+}
+
 static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
 			int flags)
 {
@@ -704,6 +721,12 @@  static int aead_recvmsg(struct socket *sock, struct msghdr *msg, size_t ignored,
 	sg_init_table(areq->tsgl, areq->tsgl_entries);
 	aead_pull_tsgl(sk, processed, areq->tsgl);
 
+	/* copy AAD from src to dst */
+	err = crypto_aead_copy_sgl(ctx->null, areq->tsgl,
+				   areq->first_rsgl.sgl.sg, ctx->aead_assoclen);
+	if (err)
+		goto free;
+
 	/* Initialize the crypto operation */
 	aead_request_set_crypt(&areq->aead_req, areq->tsgl,
 			       areq->first_rsgl.sgl.sg, used, ctx->iv);
@@ -825,6 +848,7 @@  static void aead_sock_destruct(struct sock *sk)
 	wait_event_interruptible(aead_aio_finish_wait, (ctx->inflight == 0));
 
 	aead_pull_tsgl(sk, ctx->used, NULL);
+	crypto_put_default_null_skcipher2();
 	sock_kzfree_s(sk, ctx->iv, ivlen);
 	sock_kfree_s(sk, ctx, ctx->len);
 	af_alg_release_parent(sk);
@@ -836,6 +860,7 @@  static int aead_accept_parent(void *private, struct sock *sk)
 	struct alg_sock *ask = alg_sk(sk);
 	unsigned int len = sizeof(*ctx);
 	unsigned int ivlen = crypto_aead_ivsize(private);
+	struct crypto_skcipher *null;
 
 	ctx = sock_kmalloc(sk, len, GFP_KERNEL);
 	if (!ctx)
@@ -849,6 +874,14 @@  static int aead_accept_parent(void *private, struct sock *sk)
 	}
 	memset(ctx->iv, 0, ivlen);
 
+	null = crypto_get_default_null_skcipher2();
+	if (IS_ERR(null)) {
+		sock_kfree_s(sk, ctx->iv, ivlen);
+		sock_kfree_s(sk, ctx, len);
+		return PTR_ERR(null);
+	}
+	ctx->null = null;
+
 	INIT_LIST_HEAD(&ctx->tsgl_list);
 	ctx->len = len;
 	ctx->used = 0;
-- 
2.9.3