[PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Hi,

to all driver maintainers: the patches I added are compile tested, but
I do not have the hardware to verify the code. May I ask the respective
hardware maintainers to verify that the code is appropriate and works
as intended? Thanks a lot.

Herbert, this is my proprosal for our discussion around copying the
AAD for algif_aead. Instead of adding the code to algif_aead and wait
until it transpires to all cipher implementations, I thought it would
be more helpful to fix all cipher implementations.

To do so, the AAD copy function found in authenc is extracted to a global
service function. Furthermore, the generic AEAD TFM initialization code
now allocates the null cipher too. This allows us now to only invoke
the AAD copy function in the various implementations without any additional
allocation logic.

The code for x86 and the generic code was tested with libkcapi.

The code for the drivers is compile tested for drivers applicable to
x86 only. All others are neither compile tested nor functionally tested.

Stephan Mueller (13):
  crypto: service function to copy AAD from src to dst
  crypto: gcm_generic - copy AAD during encryption
  crypto: ccm_generic - copy AAD during encryption
  crypto: rfc4106-gcm-aesni - copy AAD during encryption
  crypto: ccm-aes-ce - copy AAD during encryption
  crypto: talitos - copy AAD during encryption
  crypto: picoxcell - copy AAD during encryption
  crypto: ixp4xx - copy AAD during encryption
  crypto: atmel - copy AAD during encryption
  crypto: caam - copy AAD during encryption
  crypto: chelsio - copy AAD during encryption
  crypto: nx - copy AAD during encryption
  crypto: qat - copy AAD during encryption

 arch/arm64/crypto/aes-ce-ccm-glue.c      |  4 ++++
 arch/x86/crypto/aesni-intel_glue.c       |  5 +++++
 crypto/Kconfig                           |  4 ++--
 crypto/aead.c                            | 36 ++++++++++++++++++++++++++++++--
 crypto/authenc.c                         | 36 ++++----------------------------
 crypto/ccm.c                             | 10 +++++++++
 crypto/gcm.c                             | 17 +++++++++++++++
 drivers/crypto/atmel-aes.c               |  6 ++++++
 drivers/crypto/caam/caamalg.c            |  8 +++++++
 drivers/crypto/chelsio/chcr_algo.c       |  5 +++++
 drivers/crypto/ixp4xx_crypto.c           |  6 ++++++
 drivers/crypto/nx/nx-aes-ccm.c           |  4 ++++
 drivers/crypto/nx/nx-aes-gcm.c           | 10 +++++++++
 drivers/crypto/picoxcell_crypto.c        |  5 +++++
 drivers/crypto/qat/qat_common/qat_algs.c |  4 ++++
 drivers/crypto/talitos.c                 |  5 +++++
 include/crypto/aead.h                    |  2 ++
 include/crypto/internal/aead.h           | 12 +++++++++++
 18 files changed, 143 insertions(+), 36 deletions(-)

-- 
2.9.3

[PATCH 01/13] crypto: service function to copy AAD from src to dst

[Stephan Müller]

The service function crypto_aead_copy_ad uses the null cipher to copy
the AAD from teh source to the destination SGL. The copy operation is
prevented when the source and destination SGL is identical.

The required null cipher is allocated during the allocation of the TFM
and released with the TFM. Therefore, a use of the helper function only
requires adding an invocation of this function in the encrypt code path
shortly before the encrypt operation is invoked but after the SGLs are
set with the AEAD request.

The patch converts the authenc implementation to use this service
function instead of its own copy logic.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/Kconfig                 |  4 ++--
 crypto/aead.c                  | 36 ++++++++++++++++++++++++++++++++++--
 crypto/authenc.c               | 36 ++++--------------------------------
 include/crypto/aead.h          |  2 ++
 include/crypto/internal/aead.h | 12 ++++++++++++
 5 files changed, 54 insertions(+), 36 deletions(-)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index 160f08e..f53928a 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -44,6 +44,8 @@ config CRYPTO_AEAD
 	tristate
 	select CRYPTO_AEAD2
 	select CRYPTO_ALGAPI
+	select CRYPTO_BLKCIPHER
+	select CRYPTO_NULL
 
 config CRYPTO_AEAD2
 	tristate
@@ -227,10 +229,8 @@ config CRYPTO_MCRYPTD
 config CRYPTO_AUTHENC
 	tristate "Authenc support"
 	select CRYPTO_AEAD
-	select CRYPTO_BLKCIPHER
 	select CRYPTO_MANAGER
 	select CRYPTO_HASH
-	select CRYPTO_NULL
 	help
 	  Authenc: Combined mode wrapper for IPsec.
 	  This is required for IPSec.
diff --git a/crypto/aead.c b/crypto/aead.c
index 3f5c5ff..e9291e6 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -79,11 +79,31 @@ int crypto_aead_setauthsize(struct crypto_aead *tfm, unsigned int authsize)
 }
 EXPORT_SYMBOL_GPL(crypto_aead_setauthsize);
 
+int crypto_aead_copy_ad(struct aead_request *req)
+{
+	struct crypto_aead *aead = crypto_aead_reqtfm(req);
+	SKCIPHER_REQUEST_ON_STACK(skreq, aead->null);
+
+	/* No copy operation if src and dst are identical. */
+	if (req->src == req->dst)
+		return 0;
+
+	skcipher_request_set_tfm(skreq, aead->null);
+	skcipher_request_set_callback(skreq, aead_request_flags(req),
+				      NULL, NULL);
+	skcipher_request_set_crypt(skreq, req->src, req->dst, req->assoclen,
+				   NULL);
+
+	return crypto_skcipher_encrypt(skreq);
+}
+EXPORT_SYMBOL_GPL(crypto_aead_copy_ad);
+
 static void crypto_aead_exit_tfm(struct crypto_tfm *tfm)
 {
 	struct crypto_aead *aead = __crypto_aead_cast(tfm);
 	struct aead_alg *alg = crypto_aead_alg(aead);
 
+	crypto_put_default_null_skcipher2();
 	alg->exit(aead);
 }
 
@@ -91,14 +111,26 @@ static int crypto_aead_init_tfm(struct crypto_tfm *tfm)
 {
 	struct crypto_aead *aead = __crypto_aead_cast(tfm);
 	struct aead_alg *alg = crypto_aead_alg(aead);
+	struct crypto_skcipher *null;
+	int err;
 
 	aead->authsize = alg->maxauthsize;
 
+	null = crypto_get_default_null_skcipher2();
+	err = PTR_ERR(null);
+	if (IS_ERR(null))
+		return err;
+	aead->null = null;
+
 	if (alg->exit)
 		aead->base.exit = crypto_aead_exit_tfm;
 
-	if (alg->init)
-		return alg->init(aead);
+	if (alg->init) {
+		err = alg->init(aead);
+		if (err)
+			crypto_put_default_null_skcipher2();
+		return err;
+	}
 
 	return 0;
 }
diff --git a/crypto/authenc.c b/crypto/authenc.c
index 875470b..a7a304a 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -14,7 +14,6 @@
 #include <crypto/internal/hash.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/authenc.h>
-#include <crypto/null.h>
 #include <crypto/scatterwalk.h>
 #include <linux/err.h>
 #include <linux/init.h>
@@ -33,7 +32,6 @@ struct authenc_instance_ctx {
 struct crypto_authenc_ctx {
 	struct crypto_ahash *auth;
 	struct crypto_skcipher *enc;
-	struct crypto_skcipher *null;
 };
 
 struct authenc_request_ctx {
@@ -180,21 +178,6 @@ static void crypto_authenc_encrypt_done(struct crypto_async_request *req,
 	authenc_request_complete(areq, err);
 }
 
-static int crypto_authenc_copy_assoc(struct aead_request *req)
-{
-	struct crypto_aead *authenc = crypto_aead_reqtfm(req);
-	struct crypto_authenc_ctx *ctx = crypto_aead_ctx(authenc);
-	SKCIPHER_REQUEST_ON_STACK(skreq, ctx->null);
-
-	skcipher_request_set_tfm(skreq, ctx->null);
-	skcipher_request_set_callback(skreq, aead_request_flags(req),
-				      NULL, NULL);
-	skcipher_request_set_crypt(skreq, req->src, req->dst, req->assoclen,
-				   NULL);
-
-	return crypto_skcipher_encrypt(skreq);
-}
-
 static int crypto_authenc_encrypt(struct aead_request *req)
 {
 	struct crypto_aead *authenc = crypto_aead_reqtfm(req);
@@ -212,13 +195,12 @@ static int crypto_authenc_encrypt(struct aead_request *req)
 	src = scatterwalk_ffwd(areq_ctx->src, req->src, req->assoclen);
 	dst = src;
 
-	if (req->src != req->dst) {
-		err = crypto_authenc_copy_assoc(req);
-		if (err)
-			return err;
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
 
+	if (req->src != req->dst)
 		dst = scatterwalk_ffwd(areq_ctx->dst, req->dst, req->assoclen);
-	}
 
 	skcipher_request_set_tfm(skreq, enc);
 	skcipher_request_set_callback(skreq, aead_request_flags(req),
@@ -317,7 +299,6 @@ static int crypto_authenc_init_tfm(struct crypto_aead *tfm)
 	struct crypto_authenc_ctx *ctx = crypto_aead_ctx(tfm);
 	struct crypto_ahash *auth;
 	struct crypto_skcipher *enc;
-	struct crypto_skcipher *null;
 	int err;
 
 	auth = crypto_spawn_ahash(&ictx->auth);
@@ -329,14 +310,8 @@ static int crypto_authenc_init_tfm(struct crypto_aead *tfm)
 	if (IS_ERR(enc))
 		goto err_free_ahash;
 
-	null = crypto_get_default_null_skcipher2();
-	err = PTR_ERR(null);
-	if (IS_ERR(null))
-		goto err_free_skcipher;
-
 	ctx->auth = auth;
 	ctx->enc = enc;
-	ctx->null = null;
 
 	crypto_aead_set_reqsize(
 		tfm,
@@ -350,8 +325,6 @@ static int crypto_authenc_init_tfm(struct crypto_aead *tfm)
 
 	return 0;
 
-err_free_skcipher:
-	crypto_free_skcipher(enc);
 err_free_ahash:
 	crypto_free_ahash(auth);
 	return err;
@@ -363,7 +336,6 @@ static void crypto_authenc_exit_tfm(struct crypto_aead *tfm)
 
 	crypto_free_ahash(ctx->auth);
 	crypto_free_skcipher(ctx->enc);
-	crypto_put_default_null_skcipher2();
 }
 
 static void crypto_authenc_free(struct aead_instance *inst)
diff --git a/include/crypto/aead.h b/include/crypto/aead.h
index 03b9762..9872f92 100644
--- a/include/crypto/aead.h
+++ b/include/crypto/aead.h
@@ -16,6 +16,7 @@
 #include <linux/crypto.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <crypto/skcipher.h>
 
 /**
  * DOC: Authenticated Encryption With Associated Data (AEAD) Cipher API
@@ -155,6 +156,7 @@ struct crypto_aead {
 	unsigned int authsize;
 	unsigned int reqsize;
 
+	struct crypto_skcipher *null;
 	struct crypto_tfm base;
 };
 
diff --git a/include/crypto/internal/aead.h b/include/crypto/internal/aead.h
index 6ad8e31..01050c0 100644
--- a/include/crypto/internal/aead.h
+++ b/include/crypto/internal/aead.h
@@ -187,5 +187,17 @@ void crypto_unregister_aeads(struct aead_alg *algs, int count);
 int aead_register_instance(struct crypto_template *tmpl,
 			   struct aead_instance *inst);
 
+/**
+ * crypto_aead_copy_ad - copy the AAD from src to dst buffers
+ * @req: AEAD cipher request
+ *
+ * This function is intended for the encrypt operation only as the ciphertext
+ * should be accompanied by the AAD. The copy operation is performed with
+ * the null cipher that is allocated during initialization of the AEAD TFM.
+ *
+ * Return: 0 upon success, < 0 in case of error
+ */
+int crypto_aead_copy_ad(struct aead_request *req);
+
 #endif	/* _CRYPTO_INTERNAL_AEAD_H */
 
-- 
2.9.3

[PATCH 02/13] crypto: gcm_generic - copy AAD during encryption

[Stephan Müller]

>From 8690e52f7e90663b14b2490d37a3b5b1c5a53bfe Mon Sep 17 00:00:00 2001
From: Stephan Mueller <smueller@chronox.de>
Date: Mon, 9 Jan 2017 12:35:35 +0100
Subject: 

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/gcm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/crypto/gcm.c b/crypto/gcm.c
index b7ad808..1ca0b05c 100644
--- a/crypto/gcm.c
+++ b/crypto/gcm.c
@@ -496,11 +496,16 @@ static int crypto_gcm_encrypt(struct aead_request *req)
 	struct crypto_gcm_req_priv_ctx *pctx = crypto_gcm_reqctx(req);
 	struct skcipher_request *skreq = &pctx->u.skreq;
 	u32 flags = aead_request_flags(req);
+	int err;
 
 	crypto_gcm_init_common(req);
 	crypto_gcm_init_crypt(req, req->cryptlen);
 	skcipher_request_set_callback(skreq, flags, gcm_encrypt_done, req);
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return crypto_skcipher_encrypt(skreq) ?:
 	       gcm_encrypt_continue(req, flags);
 }
@@ -866,9 +871,15 @@ static struct aead_request *crypto_rfc4106_crypt(struct aead_request *req)
 
 static int crypto_rfc4106_encrypt(struct aead_request *req)
 {
+	int err;
+
 	if (req->assoclen != 16 && req->assoclen != 20)
 		return -EINVAL;
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	req = crypto_rfc4106_crypt(req);
 
 	return crypto_aead_encrypt(req);
@@ -1099,6 +1110,12 @@ static int crypto_rfc4543_copy_src_to_dst(struct aead_request *req, bool enc)
 
 static int crypto_rfc4543_encrypt(struct aead_request *req)
 {
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return crypto_rfc4543_crypt(req, true);
 }
 
-- 
2.9.3

[PATCH 03/13] crypto: ccm_generic - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/ccm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/crypto/ccm.c b/crypto/ccm.c
index 26b924d..e2b6d3d 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -358,6 +358,10 @@ static int crypto_ccm_encrypt(struct aead_request *req)
 	if (err)
 		return err;
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	err = crypto_ccm_auth(req, sg_next(pctx->src), cryptlen);
 	if (err)
 		return err;
@@ -749,9 +753,15 @@ static struct aead_request *crypto_rfc4309_crypt(struct aead_request *req)
 
 static int crypto_rfc4309_encrypt(struct aead_request *req)
 {
+	int err;
+
 	if (req->assoclen != 16 && req->assoclen != 20)
 		return -EINVAL;
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	req = crypto_rfc4309_crypt(req);
 
 	return crypto_aead_encrypt(req);
-- 
2.9.3

[PATCH 04/13] crypto: rfc4106-gcm-aesni - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 arch/x86/crypto/aesni-intel_glue.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 36ca150..6149018 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -882,6 +882,7 @@ static int rfc4106_encrypt(struct aead_request *req)
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	struct cryptd_aead **ctx = crypto_aead_ctx(tfm);
 	struct cryptd_aead *cryptd_tfm = *ctx;
+	int err;
 
 	tfm = &cryptd_tfm->base;
 	if (irq_fpu_usable() && (!in_atomic() ||
@@ -890,6 +891,10 @@ static int rfc4106_encrypt(struct aead_request *req)
 
 	aead_request_set_tfm(req, tfm);
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return crypto_aead_encrypt(req);
 }
 
-- 
2.9.3

[PATCH 05/13] crypto: ccm-aes-ce - copy AAD during encryption

[Stephan Müller]

>From acad5f6af68d18bc1dbac460bd44f2298902018b Mon Sep 17 00:00:00 2001
From: Stephan Mueller <smueller@chronox.de>
Date: Mon, 9 Jan 2017 14:32:02 +0100
Subject: 

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 arch/arm64/crypto/aes-ce-ccm-glue.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/crypto/aes-ce-ccm-glue.c b/arch/arm64/crypto/aes-ce-ccm-glue.c
index cc5515d..7dfcb1f 100644
--- a/arch/arm64/crypto/aes-ce-ccm-glue.c
+++ b/arch/arm64/crypto/aes-ce-ccm-glue.c
@@ -155,6 +155,10 @@ static int ccm_encrypt(struct aead_request *req)
 	u32 len = req->cryptlen;
 	int err;
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	err = ccm_init_mac(req, mac, len);
 	if (err)
 		return err;
-- 
2.9.3

[PATCH 06/13] crypto: talitos - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/talitos.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 0bba6a1..6a6e343 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1429,6 +1429,11 @@ static int aead_encrypt(struct aead_request *req)
 	struct crypto_aead *authenc = crypto_aead_reqtfm(req);
 	struct talitos_ctx *ctx = crypto_aead_ctx(authenc);
 	struct talitos_edesc *edesc;
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
 
 	/* allocate extended descriptor */
 	edesc = aead_edesc_alloc(req, req->iv, 0, true);
-- 
2.9.3

[PATCH 07/13] crypto: picoxcell - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/picoxcell_crypto.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/picoxcell_crypto.c b/drivers/crypto/picoxcell_crypto.c
index 4757609..ef955d3 100644
--- a/drivers/crypto/picoxcell_crypto.c
+++ b/drivers/crypto/picoxcell_crypto.c
@@ -688,6 +688,11 @@ static int spacc_aead_encrypt(struct aead_request *req)
 {
 	struct crypto_aead *aead = crypto_aead_reqtfm(req);
 	struct spacc_aead *alg = to_spacc_aead(crypto_aead_alg(aead));
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
 
 	return spacc_aead_setup(req, alg->type, 1);
 }
-- 
2.9.3

[PATCH 08/13] crypto: ixp4xx - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/ixp4xx_crypto.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 7868765..20a5bd8 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -1178,6 +1178,12 @@ static int aead_setkey(struct crypto_aead *tfm, const u8 *key,
 
 static int aead_encrypt(struct aead_request *req)
 {
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return aead_perform(req, 1, req->assoclen, req->cryptlen, req->iv);
 }
 
-- 
2.9.3

[PATCH 09/13] crypto: atmel - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/atmel-aes.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 0e3d0d6..48ecf72 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -1752,6 +1752,12 @@ static int atmel_aes_gcm_setauthsize(struct crypto_aead *tfm,
 
 static int atmel_aes_gcm_encrypt(struct aead_request *req)
 {
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return atmel_aes_gcm_crypt(req, AES_FLAGS_ENCRYPT);
 }
 
-- 
2.9.3

[PATCH 10/13] crypto: caam - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/caam/caamalg.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 662fe94..30ad943 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1433,6 +1433,10 @@ static int gcm_encrypt(struct aead_request *req)
 	u32 *desc;
 	int ret = 0;
 
+	ret = crypto_aead_copy_ad(req);
+	if (ret)
+		return ret;
+
 	/* allocate extended descriptor */
 	edesc = aead_edesc_alloc(req, GCM_DESC_JOB_IO_LEN, &all_contig, true);
 	if (IS_ERR(edesc))
@@ -1476,6 +1480,10 @@ static int aead_encrypt(struct aead_request *req)
 	u32 *desc;
 	int ret = 0;
 
+	ret = crypto_aead_copy_ad(req);
+	if (ret)
+		return ret;
+
 	/* allocate extended descriptor */
 	edesc = aead_edesc_alloc(req, AUTHENC_DESC_JOB_IO_LEN,
 				 &all_contig, true);
-- 
2.9.3

[PATCH 11/13] crypto: chelsio - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/chelsio/chcr_algo.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
index 2ed1e24..b3283c0 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -2398,6 +2398,11 @@ static int chcr_aead_encrypt(struct aead_request *req)
 {
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	struct chcr_aead_reqctx *reqctx = aead_request_ctx(req);
+	int err;
+
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
 
 	reqctx->verify = VERIFY_HW;
 
-- 
2.9.3

[PATCH 12/13] crypto: nx - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/nx/nx-aes-ccm.c |  4 ++++
 drivers/crypto/nx/nx-aes-gcm.c | 10 ++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/crypto/nx/nx-aes-ccm.c b/drivers/crypto/nx/nx-aes-ccm.c
index 7038f36..ee570bf 100644
--- a/drivers/crypto/nx/nx-aes-ccm.c
+++ b/drivers/crypto/nx/nx-aes-ccm.c
@@ -428,6 +428,10 @@ static int ccm_nx_encrypt(struct aead_request   *req,
 	unsigned int processed = 0, to_process;
 	int rc = -1;
 
+	rc = crypto_aead_copy_ad(req);
+	if (rc)
+		return rc;
+
 	spin_lock_irqsave(&nx_ctx->lock, irq_flags);
 
 	rc = generate_pat(desc->info, req, nx_ctx, authsize, nbytes, assoclen,
diff --git a/drivers/crypto/nx/nx-aes-gcm.c b/drivers/crypto/nx/nx-aes-gcm.c
index abd465f..0cc0533 100644
--- a/drivers/crypto/nx/nx-aes-gcm.c
+++ b/drivers/crypto/nx/nx-aes-gcm.c
@@ -432,9 +432,14 @@ static int gcm_aes_nx_encrypt(struct aead_request *req)
 {
 	struct nx_gcm_rctx *rctx = aead_request_ctx(req);
 	char *iv = rctx->iv;
+	int err;
 
 	memcpy(iv, req->iv, 12);
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return gcm_aes_nx_crypt(req, 1, req->assoclen);
 }
 
@@ -455,6 +460,7 @@ static int gcm4106_aes_nx_encrypt(struct aead_request *req)
 	struct nx_gcm_rctx *rctx = aead_request_ctx(req);
 	char *iv = rctx->iv;
 	char *nonce = nx_ctx->priv.gcm.nonce;
+	int err;
 
 	memcpy(iv, nonce, NX_GCM4106_NONCE_LEN);
 	memcpy(iv + NX_GCM4106_NONCE_LEN, req->iv, 8);
@@ -462,6 +468,10 @@ static int gcm4106_aes_nx_encrypt(struct aead_request *req)
 	if (req->assoclen < 8)
 		return -EINVAL;
 
+	err = crypto_aead_copy_ad(req);
+	if (err)
+		return err;
+
 	return gcm_aes_nx_crypt(req, 1, req->assoclen - 8);
 }
 
-- 
2.9.3

[PATCH 13/13] crypto: qat - copy AAD during encryption

[Stephan Müller]

Invoke the crypto_aead_copy_ad function during the encryption code path
to copy the AAD from the source to the destination buffer.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/crypto/qat/qat_common/qat_algs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_algs.c b/drivers/crypto/qat/qat_common/qat_algs.c
index 20f35df..6576328 100644
--- a/drivers/crypto/qat/qat_common/qat_algs.c
+++ b/drivers/crypto/qat/qat_common/qat_algs.c
@@ -865,6 +865,10 @@ static int qat_alg_aead_enc(struct aead_request *areq)
 	uint8_t *iv = areq->iv;
 	int ret, ctr = 0;
 
+	ret = crypto_aead_copy_ad(areq);
+	if (ret)
+		return ret;
+
 	ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req);
 	if (unlikely(ret))
 		return ret;
-- 
2.9.3

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Tue, Jan 10, 2017 at 02:36:21AM +0100, Stephan Müller wrote:
> 
> to all driver maintainers: the patches I added are compile tested, but
> I do not have the hardware to verify the code. May I ask the respective
> hardware maintainers to verify that the code is appropriate and works
> as intended? Thanks a lot.
> 
> Herbert, this is my proprosal for our discussion around copying the
> AAD for algif_aead. Instead of adding the code to algif_aead and wait
> until it transpires to all cipher implementations, I thought it would
> be more helpful to fix all cipher implementations.

I think it's too much churn.  Let's get the algif_aead code fixed
up first, and then pursue this later.

BTW, why are you only doing the copy for encryption?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 14:19:36 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Jan 10, 2017 at 02:36:21AM +0100, Stephan Müller wrote:
> > to all driver maintainers: the patches I added are compile tested, but
> > I do not have the hardware to verify the code. May I ask the respective
> > hardware maintainers to verify that the code is appropriate and works
> > as intended? Thanks a lot.
> > 
> > Herbert, this is my proprosal for our discussion around copying the
> > AAD for algif_aead. Instead of adding the code to algif_aead and wait
> > until it transpires to all cipher implementations, I thought it would
> > be more helpful to fix all cipher implementations.
> 
> I think it's too much churn.  Let's get the algif_aead code fixed
> up first, and then pursue this later.

My idea with this patch set was to have only a minimal change to any AEAD 
implementation, i.e. one callback to address this issue.

When addressing the issue in the algif_aead code, and expect that over time 
the AEAD implementations will gain the copy operation, eventually we will copy 
the AAD twice. Of course, this could be prevented, if the algif_aead code 
somehow uses the same SGL for the src and dst AAD.

Some time back I published the patch "crypto: AF_ALG - disregard AAD buffer 
space for output". This patch tried changing the src and dst SGL to remove the 
AAD. Considering this patch trying to change the src and dst SGL structure, I 
expect that the patch to algif_aead to have a common src/dst SGL for the AAD 
to prevent the AAD copy from the AEAD implementations is similar in 
complexity.

Weighing the complexity of such temporary band-aid for algif_aead with the 
addition of one callback to each AEAD implementation (which would need to be 
added some when anyway), I thought it is less complex to add the one callback 
to the AEAD implementations.
> 
> BTW, why are you only doing the copy for encryption?

I was looking at the only AEAD implementation that does the copy operation: 
authenc. There, the copy operation is only performed for encryption. I was 
thinking a bit about why decryption was not covered. I think the answer is the 
following: for encryption, the AAD is definitely needed in the dst buffer as 
the dst buffer with the AAD must be sent to the recipient for decryption. The 
decryption and the associated authentication only works with the AAD. However, 
after decrypting, all the caller wants is the decrypted plaintext only. There 
is no further use of the AAD after the decryption step. Hence, copying the AAD 
to the dst buffer in the decryption step would not serve the caller.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote:
> 
> When addressing the issue in the algif_aead code, and expect that over time 
> the AEAD implementations will gain the copy operation, eventually we will copy 
> the AAD twice. Of course, this could be prevented, if the algif_aead code 
> somehow uses the same SGL for the src and dst AAD.

Why would you copy it twice? You copy everything before you start
and then just do in-place crypto.

> > BTW, why are you only doing the copy for encryption?
> 
> I was looking at the only AEAD implementation that does the copy operation: 
> authenc. There, the copy operation is only performed for encryption. I was 
> thinking a bit about why decryption was not covered. I think the answer is the 
> following: for encryption, the AAD is definitely needed in the dst buffer as 
> the dst buffer with the AAD must be sent to the recipient for decryption. The 
> decryption and the associated authentication only works with the AAD. However, 
> after decrypting, all the caller wants is the decrypted plaintext only. There 
> is no further use of the AAD after the decryption step. Hence, copying the AAD 
> to the dst buffer in the decryption step would not serve the caller.

That's just the current implementation.  If we're going to make
this an API then we should do it for both directions.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 22:57:28 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 12:22:09PM +0100, Stephan Müller wrote:
> > When addressing the issue in the algif_aead code, and expect that over
> > time
> > the AEAD implementations will gain the copy operation, eventually we will
> > copy the AAD twice. Of course, this could be prevented, if the algif_aead
> > code somehow uses the same SGL for the src and dst AAD.
> 
> Why would you copy it twice? You copy everything before you start
> and then just do in-place crypto.
> 

As far as I understand, we have the following AAD copy operations that we 
discuss:

- algif_aead: you suggested to add the AAD copy operation here

- AEAD implementations: over time, the AEAD implementations shall receive the 
AAD copy operation. The AAD copy operation shall only take place if the src 
SGL != dst SGL

The algif_aead code *always* will have the src SGL being different from the 
dst SGL. Thus, any existing AEAD implementation copy will always be performed 
which would be in addition to the algif_aead AAD copy operation. We can only 
avoid the AEAD implementation copy operation, if we add some code to 
algif_aead to make sure that the AAD data is pointed to by src/dst SGL that is 
identical. However, such logic to make src/dst SGL identical is quite complex 
compared to simply use one callback as suggested in the current patch set.

In the followup email, I suggested to add the AAD copy function invocation 
into crypto_aead_encrypt. This way, we can drop the numerous patches to the 
AEAD implementations and yet we can avoid adding such copy operation and src/
dst SGL unification logic to algif_aead.

> > > BTW, why are you only doing the copy for encryption?
> > 
> > I was looking at the only AEAD implementation that does the copy
> > operation:
> > authenc. There, the copy operation is only performed for encryption. I was
> > thinking a bit about why decryption was not covered. I think the answer is
> > the following: for encryption, the AAD is definitely needed in the dst
> > buffer as the dst buffer with the AAD must be sent to the recipient for
> > decryption. The decryption and the associated authentication only works
> > with the AAD. However, after decrypting, all the caller wants is the
> > decrypted plaintext only. There is no further use of the AAD after the
> > decryption step. Hence, copying the AAD to the dst buffer in the
> > decryption step would not serve the caller.
> That's just the current implementation.  If we're going to make
> this an API then we should do it for both directions.

Considering the suggestion above to add the AAD copy call to 
crypto_aead_encrypt, we can add such copy call also to crypto_aead_decrypt.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote:
>
> As far as I understand, we have the following AAD copy operations that we 
> discuss:
> 
> - algif_aead: you suggested to add the AAD copy operation here
> 
> - AEAD implementations: over time, the AEAD implementations shall receive the 
> AAD copy operation. The AAD copy operation shall only take place if the src 
> SGL != dst SGL

If and when that happens we'd simply need to remove the copy from
algif_aead code.  But even if we didn't you wouldn't have two copies
because algif_aead should invoke the in-place code-path after doing
a full copy of src to dst.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 23:27:10 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 04:23:50PM +0100, Stephan Müller wrote:
> > As far as I understand, we have the following AAD copy operations that we
> > discuss:
> > 
> > - algif_aead: you suggested to add the AAD copy operation here
> > 
> > - AEAD implementations: over time, the AEAD implementations shall receive
> > the AAD copy operation. The AAD copy operation shall only take place if
> > the src SGL != dst SGL
> 
> If and when that happens we'd simply need to remove the copy from
> algif_aead code. 

We would only be able to remove it if all AEAD implementations are converted. 
But for the conversion time, we do face that issue.

> But even if we didn't you wouldn't have two copies
> because algif_aead should invoke the in-place code-path after doing
> a full copy of src to dst.

Are you suggesting that the entire data in the src SGL is first copied to the 
dst SGL by algif_aead? If yes, that still requires significant src/dst SGL 
tinkering as we have the tag -- the src SGL for encrypt does not have the tag 
space where the dst SGL for encrypt is required to have the tag size. This is 
vice versa for the decryption operation.

And to me the most elegant solution seems adding the copy operation to 
crypto_aead_[en|de]crypt.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 04:34:39PM +0100, Stephan Müller wrote:
>
> We would only be able to remove it if all AEAD implementations are converted. 
> But for the conversion time, we do face that issue.

It doesn't matter.  Nobody in the kernel uses that.  In fact I
wonder whether we should even do it for the kernel API.  We only
need it for the user-space API because it goes through read/write.

> Are you suggesting that the entire data in the src SGL is first copied to the 
> dst SGL by algif_aead? If yes, that still requires significant src/dst SGL 
> tinkering as we have the tag -- the src SGL for encrypt does not have the tag 
> space where the dst SGL for encrypt is required to have the tag size. This is 
> vice versa for the decryption operation.

It's really only a problem for decryption.  In that case you can
extend the dst SG list to include the tag.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 23:39:24 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 04:34:39PM +0100, Stephan Müller wrote:
> > We would only be able to remove it if all AEAD implementations are
> > converted. But for the conversion time, we do face that issue.
> 
> It doesn't matter.  Nobody in the kernel uses that.  In fact I
> wonder whether we should even do it for the kernel API.  We only
> need it for the user-space API because it goes through read/write.

So you say that we could remove it from authenc() entirely (this is currently 
the only location where such copy operation is found for the encryption 
direction)?

I would concur that the kernel does not need that.
> 
> > Are you suggesting that the entire data in the src SGL is first copied to
> > the dst SGL by algif_aead? If yes, that still requires significant
> > src/dst SGL tinkering as we have the tag -- the src SGL for encrypt does
> > not have the tag space where the dst SGL for encrypt is required to have
> > the tag size. This is vice versa for the decryption operation.
> 
> It's really only a problem for decryption.  In that case you can
> extend the dst SG list to include the tag.

If we only want to solve that for algif_aead, wouldn't it make more sense if 
the user space caller takes care of that (such as libkcapi)? By tinkering with 
the SGLs and copying the data to the dst buffer before the cipher operation 
takes place, I guess we will add performance degradation and more complexity 
in the kernel.

Having such logic in user space would keep the algif_aead cleaner IMHO.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 04:50:46PM +0100, Stephan Müller wrote:
>
> So you say that we could remove it from authenc() entirely (this is currently 
> the only location where such copy operation is found for the encryption 
> direction)?
> 
> I would concur that the kernel does not need that.

authenc needs it for performance reasons.  The kernel API as it
stands basically says that it may or may not copy the AAD.  If
you choose to have a distinct AAD in the dst SG list then either
you throw away the result or you copy it yourself.

> If we only want to solve that for algif_aead, wouldn't it make more sense if 
> the user space caller takes care of that (such as libkcapi)? By tinkering with 
> the SGLs and copying the data to the dst buffer before the cipher operation 
> takes place, I guess we will add performance degradation and more complexity 
> in the kernel.
> 
> Having such logic in user space would keep the algif_aead cleaner IMHO.

We need to have a sane kernel API that respects POSIX.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 23:53:44 CET schrieb Herbert Xu:

Hi Herbert,

> 
> > If we only want to solve that for algif_aead, wouldn't it make more sense
> > if the user space caller takes care of that (such as libkcapi)? By
> > tinkering with the SGLs and copying the data to the dst buffer before the
> > cipher operation takes place, I guess we will add performance degradation
> > and more complexity in the kernel.
> > 
> > Having such logic in user space would keep the algif_aead cleaner IMHO.
> 
> We need to have a sane kernel API that respects POSIX.

I fully agree. Therefore, I was under the impression that disregarding the AAD 
in recvmsg entirely would be most appropriate as offered with the patch 
"crypto: AF_ALG - disregard AAD buffer space for output". In this case we 
would be fully POSIX compliant, the kernel would not copy the AAD (and thus 
perform multiple memcpy operations due to copy_from_user and copy_to_user 
round trips) and leave the AAD copy operation entirely to user space.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:01:13PM +0100, Stephan Müller wrote:
>
> I fully agree. Therefore, I was under the impression that disregarding the AAD 
> in recvmsg entirely would be most appropriate as offered with the patch 
> "crypto: AF_ALG - disregard AAD buffer space for output". In this case we 
> would be fully POSIX compliant, the kernel would not copy the AAD (and thus 
> perform multiple memcpy operations due to copy_from_user and copy_to_user 
> round trips) and leave the AAD copy operation entirely to user space.

Yes but then you'd have to play nasty games to fit this through
the kernel API.  Besides, we could still do in-place crypto even
though you suggested that it's complicated.  It's not really.
All we have to do is walk through the SG list and compare each
page/offset.  For the common case it's going to be a single-entry
list.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 00:06:29 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:01:13PM +0100, Stephan Müller wrote:
> > I fully agree. Therefore, I was under the impression that disregarding the
> > AAD in recvmsg entirely would be most appropriate as offered with the
> > patch "crypto: AF_ALG - disregard AAD buffer space for output". In this
> > case we would be fully POSIX compliant, the kernel would not copy the AAD
> > (and thus perform multiple memcpy operations due to copy_from_user and
> > copy_to_user round trips) and leave the AAD copy operation entirely to
> > user space.
> Yes but then you'd have to play nasty games to fit this through
> the kernel API. 

I would not understand that statement.

With the patch mentioned above that I provided some weeks ago, we have the 
following scenario for an encryption (in case of decryption, it is almost 
identical, just the tag location is reversed):

user calls sendmsg with data buffer/IOVEC: AAD || PT
	-> algif_aead turns this into the src SGL

user calls recvmsg with data buffer/IOVEC: CT || Tag
	-> algif_aead creates the first SG entry in the dst SGL pointing to the 
AAD from the src SGL
	-> algif_aead appends the user buffers to the dst SGL

	-> algif_aead performs its operation and during that operation, only the 
CT and Tag parts are changed

I.e. with the pre-pending of the SG pointing to the AAD from the src SGL to 
the dst SGL we have a clean invocation of the kernel API.

Yet, we avoid copy round trips of the AAD from src to dst in the kernel.

> Besides, we could still do in-place crypto even
> though you suggested that it's complicated.

Is that crypto-in-place operation really a goal we want to achieve if we "buy" 
it with a memcpy of the data from the src SGL to the dst SGL before the crypto 
operation?

Can we really call such implementation a crypto-in-place operation?

> It's not really.

I concur, for encryption the suggested memcpy is not difficult. Only for 
decryption, the memcpy is more challenging.

> All we have to do is walk through the SG list and compare each
> page/offset.  For the common case it's going to be a single-entry
> list.
> 
> Cheers,



Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Thu, Jan 12, 2017 at 05:37:33PM +0100, Stephan Müller wrote:
>
> I would not understand that statement.
> 
> With the patch mentioned above that I provided some weeks ago, we have the 
> following scenario for an encryption (in case of decryption, it is almost 
> identical, just the tag location is reversed):
> 
> user calls sendmsg with data buffer/IOVEC: AAD || PT
> 	-> algif_aead turns this into the src SGL
> 
> user calls recvmsg with data buffer/IOVEC: CT || Tag
> 	-> algif_aead creates the first SG entry in the dst SGL pointing to the 
> AAD from the src SGL
> 	-> algif_aead appends the user buffers to the dst SGL
> 
> 	-> algif_aead performs its operation and during that operation, only the 
> CT and Tag parts are changed
> 
> I.e. with the pre-pending of the SG pointing to the AAD from the src SGL to 
> the dst SGL we have a clean invocation of the kernel API.

But that means you can never invoke the in-place path of the kernel
API, which is the most optimised code path.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 18:23:39 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Jan 12, 2017 at 05:37:33PM +0100, Stephan Müller wrote:
> > I would not understand that statement.
> > 
> > With the patch mentioned above that I provided some weeks ago, we have the
> > following scenario for an encryption (in case of decryption, it is almost
> > identical, just the tag location is reversed):
> > 
> > user calls sendmsg with data buffer/IOVEC: AAD || PT
> > 
> > 	-> algif_aead turns this into the src SGL
> > 
> > user calls recvmsg with data buffer/IOVEC: CT || Tag
> > 
> > 	-> algif_aead creates the first SG entry in the dst SGL pointing to the
> > 
> > AAD from the src SGL
> > 
> > 	-> algif_aead appends the user buffers to the dst SGL
> > 	
> > 	-> algif_aead performs its operation and during that operation, only the
> > 
> > CT and Tag parts are changed
> > 
> > I.e. with the pre-pending of the SG pointing to the AAD from the src SGL
> > to
> > the dst SGL we have a clean invocation of the kernel API.
> 
> But that means you can never invoke the in-place path of the kernel
> API, which is the most optimised code path.

May I ask how the in-place code path can be invoked by algif_aead or 
algif_skcipher? As far as I understand, this code path is only invoked when 
the cipher implementation sees that the src and dst SGLs are identical.

However, both algif interfaces maintain separate src and dst SGLs and always 
invoke the cipher operation with these dissimilar SGLs. Thus, I would infer 
that even when the user invokes zerocopy, the src/dst SGLs are not identical 
and therefore the cipher implementations would not use the in-place code path.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 13, 2017 at 11:58:24AM +0100, Stephan Müller wrote:
>
> May I ask how the in-place code path can be invoked by algif_aead or 
> algif_skcipher? As far as I understand, this code path is only invoked when 
> the cipher implementation sees that the src and dst SGLs are identical.

It's not right now but it isn't difficult to add the code to allow
it by comparing SGLs.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:04:17 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 11:58:24AM +0100, Stephan Müller wrote:
> > May I ask how the in-place code path can be invoked by algif_aead or
> > algif_skcipher? As far as I understand, this code path is only invoked
> > when
> > the cipher implementation sees that the src and dst SGLs are identical.
> 
> It's not right now but it isn't difficult to add the code to allow
> it by comparing SGLs.

Adding such code should IMHO not be impaired by pointing to the AAD held in 
the src SGL by the dst SGL as offered with the older patch mentioned before.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:12:39PM +0100, Stephan Müller wrote:
>
> Adding such code should IMHO not be impaired by pointing to the AAD held in 
> the src SGL by the dst SGL as offered with the older patch mentioned before.

The point is you're turning what could otherwise be a linear SGL
into a non-linear one by forcing the same AAD on both sides.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:14:06 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:12:39PM +0100, Stephan Müller wrote:
> > Adding such code should IMHO not be impaired by pointing to the AAD held
> > in
> > the src SGL by the dst SGL as offered with the older patch mentioned
> > before.
> The point is you're turning what could otherwise be a linear SGL
> into a non-linear one by forcing the same AAD on both sides.

That is correct, but I thought that playing with pointers is always faster 
than doing memcpy. Are you saying that this assumption is not true when we 
somehow have the code to try to perform an in-place operation?

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:19:48PM +0100, Stephan Müller wrote:
> 
> That is correct, but I thought that playing with pointers is always faster 
> than doing memcpy. Are you saying that this assumption is not true when we 
> somehow have the code to try to perform an in-place operation?

If you're doing crypto the overhead in copying the AAD is the
last thing you need to worry about.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:26:23 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:19:48PM +0100, Stephan Müller wrote:
> > That is correct, but I thought that playing with pointers is always faster
> > than doing memcpy. Are you saying that this assumption is not true when we
> > somehow have the code to try to perform an in-place operation?
> 
> If you're doing crypto the overhead in copying the AAD is the
> last thing you need to worry about.

So, the patch set you want to see is:

- remove the AAD copy operation from authenc and not add it to any AEAD 
implementations

- add the AAD copy operation to algif_aead

Shall the null cipher context we need for that be anchored in the crypto_aead 
data structure as done in patch 01 of this series (this would allow an easy 
addition to the copy call to every AEAD implementation if deemed needed) or 
shall it be anchored within algif_aead?

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:30:15PM +0100, Stephan Müller wrote:
>
> So, the patch set you want to see is:
> 
> - remove the AAD copy operation from authenc and not add it to any AEAD 
> implementations

Why would you remove it from authenc?

> - add the AAD copy operation to algif_aead

No just copy everything and then do in-place crypto.

> Shall the null cipher context we need for that be anchored in the crypto_aead 
> data structure as done in patch 01 of this series (this would allow an easy 
> addition to the copy call to every AEAD implementation if deemed needed) or 
> shall it be anchored within algif_aead?

It should be in algif_aead.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 13. Januar 2017, 19:33:24 CET schrieb Herbert Xu:

Hi Herbert,

> On Fri, Jan 13, 2017 at 12:30:15PM +0100, Stephan Müller wrote:
> > So, the patch set you want to see is:
> > 
> > - remove the AAD copy operation from authenc and not add it to any AEAD
> > implementations
> 
> Why would you remove it from authenc?

I thought I understood that you would not want to see it in any 
implementation. But, ok, if you want to leave it.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote:
>
> I thought I understood that you would not want to see it in any 
> implementation. But, ok, if you want to leave it.

If you remove it from authenc then authenc will be broken.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Cyrille Pitchen]

Hi all,

Le 13/01/2017 à 12:39, Herbert Xu a écrit :
> On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote:
>>
>> I thought I understood that you would not want to see it in any 
>> implementation. But, ok, if you want to leave it.
> 
> If you remove it from authenc then authenc will be broken.
> 

Hence if the copy of the associated data is needed in the crypto/authenc.c
driver, then I should also keep this copy in the drivers/crypto/atmel-aes.c
for authenc(hmac(shaX),cbc-aes) algorithms [1], shouldn't I?

If so, should I keep the current not optimized implementation of
atmel_aes_authenc_copy_assoc() or try to use the code extracted by Stephan
from crypto/authenc.c using the null cipher as proposed in this thread?

As said earlier in this thread, copying the associated data is not a so big
deal when compared to the main crypto processing.

For instance, with IPSec ESP with AES in CBC mode, the associated data
layout should be:
Security Parameters Index (SPI): 4 bytes
Sequence Number: 4 bytes
AES IV: 16 bytes

So it's only a 24 byte copy.

I've taken Stephan's other comments into account from his review of the
atmel-authenc driver so I'm preparing a new series but I don't know what to
do for the associated data copy.

Please let me know what you recommend, thanks!

Best regards,

Cyrille


[1] https://lkml.org/lkml/2016/12/22/306

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Freitag, 20. Januar 2017, 18:07:04 CET schrieb Cyrille Pitchen:

Hi Cyrille,

> 
> I've taken Stephan's other comments into account from his review of the
> atmel-authenc driver so I'm preparing a new series but I don't know what to
> do for the associated data copy.
> 
> Please let me know what you recommend, thanks!

The question is where the null context shall be saved. As Herbert mentioned, 
he may not want to have it in crypto_aead.

Herbert, as this implementation also requires the copy, shall we leave the 
null context in crypto_aead? If so, Cyrille can simply use patch 01 from this 
series and apply the change to his code similar to what I did in patches 02 - 
13.

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Herbert Xu]

On Fri, Jan 20, 2017 at 06:07:04PM +0100, Cyrille Pitchen wrote:
> Hi all,
> 
> Le 13/01/2017 à 12:39, Herbert Xu a écrit :
> > On Fri, Jan 13, 2017 at 12:36:56PM +0100, Stephan Müller wrote:
> >>
> >> I thought I understood that you would not want to see it in any 
> >> implementation. But, ok, if you want to leave it.
> > 
> > If you remove it from authenc then authenc will be broken.
> > 
> 
> Hence if the copy of the associated data is needed in the crypto/authenc.c
> driver, then I should also keep this copy in the drivers/crypto/atmel-aes.c
> for authenc(hmac(shaX),cbc-aes) algorithms [1], shouldn't I?
> 
> If so, should I keep the current not optimized implementation of
> atmel_aes_authenc_copy_assoc() or try to use the code extracted by Stephan
> from crypto/authenc.c using the null cipher as proposed in this thread?
> 
> As said earlier in this thread, copying the associated data is not a so big
> deal when compared to the main crypto processing.

Please just ignore this for now.  We have not decided to require
the copying of the AAD in the kernel API.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Stephan Müller]

Am Donnerstag, 12. Januar 2017, 14:19:36 CET schrieb Herbert Xu:

Hi Herbert,

> 
> I think it's too much churn.  Let's get the algif_aead code fixed
> up first, and then pursue this later.

To eliminate the extra churn of having all AEAD implementations changed to 
invoke copy operation, what about adding the callback to crypto_aead_encrypt?

This way, all AEAD implementations benefit from it without having an extra 
call added to each of them?

Ciao
Stephan

Re: [PATCH 00/13] crypto: copy AAD during encrypt for AEAD ciphers

[Cyrille Pitchen]

Hi Stephan,

this series of patches sounds like a good idea. I haven't tested it with
the Atmel AES hardware yet but I have many dummy questions:

Looking at some driver patches in the series, it seems you only add a call
to crypto_aead_copy_ad() but I don't see any removal of the previous driver
specific implementation to perform that copy.

I take the Atmel gcm(aes) driver case aside since looking at the current
code, it seems that I've forgotten to copy that assoc data from req->src
into req->dst scatter list. Then I assume that I was lucky so when I tested
with the test manager or IPSec connections, I always fell in the case where
req->src == req->dst. I thought the test manager also covered the case
req->src != req->dst but I don't remember very well and I haven't checked
recently, sorry!

Then I now look at the default drivers/gcm.c driver and, if I understand
this driver correctly, it doesn't copy the associated data from req->src
into req->dst when req->src != req->dst...
What does it mean? Did I misunderstand the gmc.c driver or is it a bug? Is
that copy the associated data needed after all?
Also looking at the drivers/authenc.c driver, this one does copy the
associated data.
So now I understand why I didn't make the copy for gcm(aes) but did it for
authenc(hmac(shaX),cbc(aes)) in atmel-aes.c: when I add new crypto
algorithms support in the Atmel drivers, I always take the crypto/*.c
drivers as reference.

So finally, what shall we do? copy or not copy? That is my question!

One last question: why do we copy those associated data only for encrypting
requests but not for decrypting requests?

The associated data might still be needed in the req->dst scatter list even
when it only refers to plain data so no other crypto operation is needed
after. However, let's take the example of an IPSec connection with ESP: the
first 8 bytes of the ESP header (4-byte SPI + 4-byte sequence number) are
used as associated data. They must be authenticated but they cannot be
ciphered as we need the plain SPI value to attach some IP packet to the
relevant IPSec session hence knowing the crypto algorithms to be used to
process the network packet.
However, once the received IPSec packet has been decrypted and
authenticated, the sequence number part the the ESP header might still be
needed in req->dst if for some reason the req->src is no longer available
when performing the anti-replay check.
Maybe the issue simply never occurs because req->src is always == req->dst
or maybe because the anti-replay check is always performed before the
crypto stuff. I dunno!

So why not copying the associated data also when processing decrypt
requests too?

Sorry for those newbie questions! I try to improve my understanding and
knowledge of the crypto subsystem and its interaction with the network
subsystem without digging too much in the source code :p

Best regards,

Cyrille

Le 10/01/2017 à 02:36, Stephan Müller a écrit :
> Hi,
> 
> to all driver maintainers: the patches I added are compile tested, but
> I do not have the hardware to verify the code. May I ask the respective
> hardware maintainers to verify that the code is appropriate and works
> as intended? Thanks a lot.
> 
> Herbert, this is my proprosal for our discussion around copying the
> AAD for algif_aead. Instead of adding the code to algif_aead and wait
> until it transpires to all cipher implementations, I thought it would
> be more helpful to fix all cipher implementations.
> 
> To do so, the AAD copy function found in authenc is extracted to a global
> service function. Furthermore, the generic AEAD TFM initialization code
> now allocates the null cipher too. This allows us now to only invoke
> the AAD copy function in the various implementations without any additional
> allocation logic.
> 
> The code for x86 and the generic code was tested with libkcapi.
> 
> The code for the drivers is compile tested for drivers applicable to
> x86 only. All others are neither compile tested nor functionally tested.
> 
> Stephan Mueller (13):
>   crypto: service function to copy AAD from src to dst
>   crypto: gcm_generic - copy AAD during encryption
>   crypto: ccm_generic - copy AAD during encryption
>   crypto: rfc4106-gcm-aesni - copy AAD during encryption
>   crypto: ccm-aes-ce - copy AAD during encryption
>   crypto: talitos - copy AAD during encryption
>   crypto: picoxcell - copy AAD during encryption
>   crypto: ixp4xx - copy AAD during encryption
>   crypto: atmel - copy AAD during encryption
>   crypto: caam - copy AAD during encryption
>   crypto: chelsio - copy AAD during encryption
>   crypto: nx - copy AAD during encryption
>   crypto: qat - copy AAD during encryption
> 
>  arch/arm64/crypto/aes-ce-ccm-glue.c      |  4 ++++
>  arch/x86/crypto/aesni-intel_glue.c       |  5 +++++
>  crypto/Kconfig                           |  4 ++--
>  crypto/aead.c                            | 36 ++++++++++++++++++++++++++++++--
>  crypto/authenc.c                         | 36 ++++----------------------------
>  crypto/ccm.c                             | 10 +++++++++
>  crypto/gcm.c                             | 17 +++++++++++++++
>  drivers/crypto/atmel-aes.c               |  6 ++++++
>  drivers/crypto/caam/caamalg.c            |  8 +++++++
>  drivers/crypto/chelsio/chcr_algo.c       |  5 +++++
>  drivers/crypto/ixp4xx_crypto.c           |  6 ++++++
>  drivers/crypto/nx/nx-aes-ccm.c           |  4 ++++
>  drivers/crypto/nx/nx-aes-gcm.c           | 10 +++++++++
>  drivers/crypto/picoxcell_crypto.c        |  5 +++++
>  drivers/crypto/qat/qat_common/qat_algs.c |  4 ++++
>  drivers/crypto/talitos.c                 |  5 +++++
>  include/crypto/aead.h                    |  2 ++
>  include/crypto/internal/aead.h           | 12 +++++++++++
>  18 files changed, 143 insertions(+), 36 deletions(-)
>