[PATCH 0/3] crypto: AF_ALG - AEAD memory handling fixes

[PATCH 0/3] crypto: AF_ALG - AEAD memory handling fixes

[Stephan Mueller]

Hi Herbert,

The first patch is unchanged compared to a previous submission to the
mailing list. It is rolled into this patch set to have a common reference
for the AEAD user space interface changes. Therefore, please disregard
the old patch submission.

The third patch should go into stable as this fixes a kernel crash that
is present in current kernel versions.

The patch set can be tested with the HEAD branch of libkcapi found at [1].
If the patch set is accepted, the development branch will be released as
0.13.0.

[1] https://github.com/smuellerDD/libkcapi

Stephan Mueller (3):
  crypto: AF_ALG - fix AEAD tag memory handling
  crypto: AF_ALG - disregard AAD buffer space for output
  crypto: AF_ALG - fix AEAD AIO handling of zero buffer

 crypto/algif_aead.c | 215 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 166 insertions(+), 49 deletions(-)

-- 
2.7.4

[PATCH 1/3] crypto: AF_ALG - fix AEAD tag memory handling

[Stephan Mueller]

For encryption, the AEAD ciphers require AAD || PT as input and generate
AAD || CT || Tag as output and vice versa for decryption. Prior to this
patch, the AF_ALG interface for AEAD ciphers requires the buffer to be
present as input for encryption. Similarly, the output buffer for
decryption required the presence of the tag buffer too. This implies
that the kernel reads / writes data buffers from/to kernel space
even though this operation is not required.

This patch changes the AF_ALG AEAD interface to be consistent with the
in-kernel AEAD cipher requirements.

In addition, the code now handles the situation where the provided
output buffer is too small by reducing the size of the processed
input buffer accordingly. Due to this handling, he changes are
transparent to user space with one exception: the return code of recv
indicates the mount of output buffer. That output buffer has a different
size compared to before the patch which implies that the return code of
recv will also be different. For example, a decryption operation uses 16
bytes AAD, 16 bytes CT and 16 bytes tag, the AF_ALG AEAD interface
before showed a recv return code of 48 (bytes) whereas after this patch,
the return code is 32 since the tag is not returned any more.

Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/algif_aead.c | 77 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 22 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 80a0f1a..c54bcb8 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -81,7 +81,11 @@ static inline bool aead_sufficient_data(struct aead_ctx *ctx)
 {
 	unsigned as = crypto_aead_authsize(crypto_aead_reqtfm(&ctx->aead_req));
 
-	return ctx->used >= ctx->aead_assoclen + as;
+	/*
+	 * The minimum amount of memory needed for an AEAD cipher is
+	 * the AAD and in case of decryption the tag.
+	 */
+	return ctx->used >= ctx->aead_assoclen + (ctx->enc ? 0 : as);
 }
 
 static void aead_reset_ctx(struct aead_ctx *ctx)
@@ -426,12 +430,15 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 			goto unlock;
 	}
 
-	used = ctx->used;
-	outlen = used;
-
 	if (!aead_sufficient_data(ctx))
 		goto unlock;
 
+	used = ctx->used;
+	if (ctx->enc)
+		outlen = used + as;
+	else
+		outlen = used - as;
+
 	req = sock_kmalloc(sk, reqlen, GFP_KERNEL);
 	if (unlikely(!req))
 		goto unlock;
@@ -445,7 +452,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	aead_request_set_ad(req, ctx->aead_assoclen);
 	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				  aead_async_cb, sk);
-	used -= ctx->aead_assoclen + (ctx->enc ? as : 0);
+	used -= ctx->aead_assoclen;
 
 	/* take over all tx sgls from ctx */
 	areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * sgl->cur,
@@ -461,7 +468,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	areq->tsgls = sgl->cur;
 
 	/* create rx sgls */
-	while (iov_iter_count(&msg->msg_iter)) {
+	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
 				      (outlen - usedpages));
 
@@ -491,16 +498,20 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 
 		last_rsgl = rsgl;
 
-		/* we do not need more iovecs as we have sufficient memory */
-		if (outlen <= usedpages)
-			break;
-
 		iov_iter_advance(&msg->msg_iter, err);
 	}
-	err = -EINVAL;
+
 	/* ensure output buffer is sufficiently large */
-	if (usedpages < outlen)
-		goto free;
+	if (usedpages < outlen) {
+		size_t less = outlen - usedpages;
+
+		if (used < less) {
+			err = -EINVAL;
+			goto unlock;
+		}
+		used -= less;
+		outlen -= less;
+	}
 
 	aead_request_set_crypt(req, areq->tsgl, areq->first_rsgl.sgl.sg, used,
 			       areq->iv);
@@ -571,6 +582,7 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 			goto unlock;
 	}
 
+	/* data length provided by caller via sendmsg/sendpage */
 	used = ctx->used;
 
 	/*
@@ -585,16 +597,27 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 	if (!aead_sufficient_data(ctx))
 		goto unlock;
 
-	outlen = used;
+	/*
+	 * Calculate the minimum output buffer size holding the result of the
+	 * cipher operation. When encrypting data, the receiving buffer is
+	 * larger by the tag length compared to the input buffer as the
+	 * encryption operation generates the tag. For decryption, the input
+	 * buffer provides the tag which is consumed resulting in only the
+	 * plaintext without a buffer for the tag returned to the caller.
+	 */
+	if (ctx->enc)
+		outlen = used + as;
+	else
+		outlen = used - as;
 
 	/*
 	 * The cipher operation input data is reduced by the associated data
 	 * length as this data is processed separately later on.
 	 */
-	used -= ctx->aead_assoclen + (ctx->enc ? as : 0);
+	used -= ctx->aead_assoclen;
 
 	/* convert iovecs of output buffers into scatterlists */
-	while (iov_iter_count(&msg->msg_iter)) {
+	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
 				      (outlen - usedpages));
 
@@ -621,16 +644,26 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 
 		last_rsgl = rsgl;
 
-		/* we do not need more iovecs as we have sufficient memory */
-		if (outlen <= usedpages)
-			break;
 		iov_iter_advance(&msg->msg_iter, err);
 	}
 
-	err = -EINVAL;
 	/* ensure output buffer is sufficiently large */
-	if (usedpages < outlen)
-		goto unlock;
+	if (usedpages < outlen) {
+		size_t less = outlen - usedpages;
+
+		if (used < less) {
+			err = -EINVAL;
+			goto unlock;
+		}
+
+		/*
+		 * Caller has smaller output buffer than needed, reduce
+		 * the input data length to be processed to fit the provided
+		 * output buffer.
+		 */
+		used -= less;
+		outlen -= less;
+	}
 
 	sg_mark_end(sgl->sg + sgl->cur - 1);
 	aead_request_set_crypt(&ctx->aead_req, sgl->sg, ctx->first_rsgl.sgl.sg,
-- 
2.7.4

[PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

The kernel crypto API AEAD cipher operation generates output such that
space for the AAD is reserved in the output buffer without being
touched. The processed ciphertext/plaintext is appended to the reserved
AAD buffer.

The user space interface followed that approach. However, this is a
violation of the POSIX read definition which requires that any read data
is placed at the beginning of the caller-provided buffer. As the kernel
crypto API would leave room for the AAD, the old approach did not fully
comply with the POSIX specification.

The patch changes the user space AF_ALG AEAD interface such that the
processed ciphertext/plaintext are now placed at the beginning of the
user buffer provided with the read system call. That means the user
space interface now deviates from the in-kernel output buffer handling.

For the cipher operation, the AAD buffer provided during input is
pointed to by a new SGL which is chained with the output buffer SGL.
With this approach, only pointers to one copy of the AAD are maintained
to avoid data duplication.

With this solution, the caller must not use sendpage with the exact same
buffers for input and output. The following rationale applies: When
the caller sends the same buffer for input/output to the sendpage
operation, the cipher operation now will write the ciphertext to the
beginning of the buffer where the AAD used to be. The subsequent tag
calculation will now use the data it finds where the AAD is expected.
As the cipher operation has already replaced the AAD with the ciphertext,
the tag calculation will take the ciphertext as AAD and thus calculate
a wrong tag.

Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/algif_aead.c | 143 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 113 insertions(+), 30 deletions(-)

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index c54bcb8..0212cc2 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -32,6 +32,7 @@ struct aead_sg_list {
 struct aead_async_rsgl {
 	struct af_alg_sgl sgl;
 	struct list_head list;
+	bool new_page;
 };
 
 struct aead_async_req {
@@ -405,6 +406,61 @@ static void aead_async_cb(struct crypto_async_request *_req, int err)
 	iocb->ki_complete(iocb, err, err);
 }
 
+/**
+ * scatterwalk_get_part() - get subset a scatterlist
+ *
+ * @dst: destination SGL to receive the pointers from source SGL
+ * @src: source SGL
+ * @len: data length in bytes to get from source SGL
+ * @max_sgs: number of SGs present in dst SGL to prevent overstepping boundaries
+ *
+ * @return: number of SG entries in dst
+ */
+static inline int scatterwalk_get_part(struct scatterlist *dst,
+				       struct scatterlist *src,
+				       unsigned int len, unsigned int max_sgs)
+{
+	/* leave one SG entry for chaining */
+	unsigned int j = 1;
+
+	while (len && j < max_sgs) {
+		unsigned int todo = min_t(unsigned int, len, src->length);
+
+		sg_set_page(dst, sg_page(src), todo, src->offset);
+		if (src->length >= len) {
+			sg_mark_end(dst);
+			break;
+		}
+		len -= todo;
+		j++;
+		src = sg_next(src);
+		dst = sg_next(dst);
+	}
+
+	return j;
+}
+
+static inline int aead_alloc_rsgl(struct sock *sk, struct aead_async_rsgl **ret)
+{
+	struct aead_async_rsgl *rsgl =
+				sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
+	if (unlikely(!rsgl))
+		return -ENOMEM;
+	*ret = rsgl;
+	return 0;
+}
+
+static inline int aead_get_rsgl_areq(struct sock *sk,
+				     struct aead_async_req *areq,
+				     struct aead_async_rsgl **ret)
+{
+	if (list_empty(&areq->list)) {
+		*ret = &areq->first_rsgl;
+		return 0;
+	} else
+		return aead_alloc_rsgl(sk, ret);
+}
+
 static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 			      int flags)
 {
@@ -433,7 +489,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	if (!aead_sufficient_data(ctx))
 		goto unlock;
 
-	used = ctx->used;
+	used = ctx->used - ctx->aead_assoclen;
 	if (ctx->enc)
 		outlen = used + as;
 	else
@@ -452,7 +508,6 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	aead_request_set_ad(req, ctx->aead_assoclen);
 	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				  aead_async_cb, sk);
-	used -= ctx->aead_assoclen;
 
 	/* take over all tx sgls from ctx */
 	areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * sgl->cur,
@@ -467,21 +522,26 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 
 	areq->tsgls = sgl->cur;
 
+	/* set AAD buffer */
+	err = aead_get_rsgl_areq(sk, areq, &rsgl);
+	if (err)
+		goto free;
+	list_add_tail(&rsgl->list, &areq->list);
+	sg_init_table(rsgl->sgl.sg, ALG_MAX_PAGES);
+	rsgl->sgl.npages = scatterwalk_get_part(rsgl->sgl.sg, sgl->sg,
+						ctx->aead_assoclen,
+						ALG_MAX_PAGES);
+	rsgl->new_page = false;
+	last_rsgl = rsgl;
+
 	/* create rx sgls */
 	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
 				      (outlen - usedpages));
 
-		if (list_empty(&areq->list)) {
-			rsgl = &areq->first_rsgl;
-
-		} else {
-			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
-			if (unlikely(!rsgl)) {
-				err = -ENOMEM;
-				goto free;
-			}
-		}
+		err = aead_get_rsgl_areq(sk, areq, &rsgl);
+		if (err)
+			goto free;
 		rsgl->sgl.npages = 0;
 		list_add_tail(&rsgl->list, &areq->list);
 
@@ -491,6 +551,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 			goto free;
 
 		usedpages += err;
+		rsgl->new_page = true;
 
 		/* chain the new scatterlist with previous one */
 		if (last_rsgl)
@@ -507,7 +568,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 
 		if (used < less) {
 			err = -EINVAL;
-			goto unlock;
+			goto free;
 		}
 		used -= less;
 		outlen -= less;
@@ -531,7 +592,8 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 
 free:
 	list_for_each_entry(rsgl, &areq->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
+		if (rsgl->new_page)
+			af_alg_free_sg(&rsgl->sgl);
 		if (rsgl != &areq->first_rsgl)
 			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
 	}
@@ -545,6 +607,16 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 	return err ? err : outlen;
 }
 
+static inline int aead_get_rsgl_ctx(struct sock *sk, struct aead_ctx *ctx,
+				    struct aead_async_rsgl **ret)
+{
+	if (list_empty(&ctx->list)) {
+		*ret = &ctx->first_rsgl;
+		return 0;
+	} else
+		return aead_alloc_rsgl(sk, ret);
+}
+
 static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 {
 	struct sock *sk = sock->sk;
@@ -582,9 +654,6 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 			goto unlock;
 	}
 
-	/* data length provided by caller via sendmsg/sendpage */
-	used = ctx->used;
-
 	/*
 	 * Make sure sufficient data is present -- note, the same check is
 	 * is also present in sendmsg/sendpage. The checks in sendpage/sendmsg
@@ -598,6 +667,12 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 		goto unlock;
 
 	/*
+	 * The cipher operation input data is reduced by the associated data
+	 * as the destination buffer will not hold the AAD.
+	 */
+	used = ctx->used - ctx->aead_assoclen;
+
+	/*
 	 * Calculate the minimum output buffer size holding the result of the
 	 * cipher operation. When encrypting data, the receiving buffer is
 	 * larger by the tag length compared to the input buffer as the
@@ -611,25 +686,29 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 		outlen = used - as;
 
 	/*
-	 * The cipher operation input data is reduced by the associated data
-	 * length as this data is processed separately later on.
+	 * Pre-pend the AAD buffer from the source SGL to the destination SGL.
+	 * As the AAD buffer is not touched by the AEAD operation, the source
+	 * SG buffers remain unchanged.
 	 */
-	used -= ctx->aead_assoclen;
+	err = aead_get_rsgl_ctx(sk, ctx, &rsgl);
+	if (err)
+		goto unlock;
+	list_add_tail(&rsgl->list, &ctx->list);
+	sg_init_table(rsgl->sgl.sg, ALG_MAX_PAGES);
+	rsgl->sgl.npages = scatterwalk_get_part(rsgl->sgl.sg, sgl->sg,
+						ctx->aead_assoclen,
+						ALG_MAX_PAGES);
+	rsgl->new_page = false;
+	last_rsgl = rsgl;
 
 	/* convert iovecs of output buffers into scatterlists */
 	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
 				      (outlen - usedpages));
 
-		if (list_empty(&ctx->list)) {
-			rsgl = &ctx->first_rsgl;
-		} else {
-			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
-			if (unlikely(!rsgl)) {
-				err = -ENOMEM;
-				goto unlock;
-			}
-		}
+		err = aead_get_rsgl_ctx(sk, ctx, &rsgl);
+		if (err)
+			goto unlock;
 		rsgl->sgl.npages = 0;
 		list_add_tail(&rsgl->list, &ctx->list);
 
@@ -637,7 +716,10 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 		err = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, seglen);
 		if (err < 0)
 			goto unlock;
+
 		usedpages += err;
+		rsgl->new_page = true;
+
 		/* chain the new scatterlist with previous one */
 		if (last_rsgl)
 			af_alg_link_sg(&last_rsgl->sgl, &rsgl->sgl);
@@ -688,7 +770,8 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
 
 unlock:
 	list_for_each_entry_safe(rsgl, tmp, &ctx->list, list) {
-		af_alg_free_sg(&rsgl->sgl);
+		if (rsgl->new_page)
+			af_alg_free_sg(&rsgl->sgl);
 		if (rsgl != &ctx->first_rsgl)
 			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
 		list_del(&rsgl->list);
-- 
2.7.4

[PATCH 3/3] crypto: AF_ALG - fix AEAD AIO handling of zero buffer

[Stephan Mueller]

Handle the case when the caller provided a zero buffer to
sendmsg/sendpage. Such scenario is legal for AEAD ciphers when no
plaintext / ciphertext and no AAD is provided and the caller only
requests the generation of the tag value.

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

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 0212cc2..d52e8b2 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -510,12 +510,13 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
 				  aead_async_cb, sk);
 
 	/* take over all tx sgls from ctx */
-	areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * sgl->cur,
+	areq->tsgl = sock_kmalloc(sk,
+				  sizeof(*areq->tsgl) * max_t(u32, sgl->cur, 1),
 				  GFP_KERNEL);
 	if (unlikely(!areq->tsgl))
 		goto free;
 
-	sg_init_table(areq->tsgl, sgl->cur);
+	sg_init_table(areq->tsgl, max_t(u32, sgl->cur, 1));
 	for (i = 0; i < sgl->cur; i++)
 		sg_set_page(&areq->tsgl[i], sg_page(&sgl->sg[i]),
 			    sgl->sg[i].length, sgl->sg[i].offset);
-- 
2.7.4

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Mat Martineau]

Stephan,

On Thu, 10 Nov 2016, Stephan Mueller wrote:

> The kernel crypto API AEAD cipher operation generates output such that
> space for the AAD is reserved in the output buffer without being
> touched. The processed ciphertext/plaintext is appended to the reserved
> AAD buffer.
>
> The user space interface followed that approach. However, this is a
> violation of the POSIX read definition which requires that any read data
> is placed at the beginning of the caller-provided buffer. As the kernel
> crypto API would leave room for the AAD, the old approach did not fully
> comply with the POSIX specification.
>
> The patch changes the user space AF_ALG AEAD interface such that the
> processed ciphertext/plaintext are now placed at the beginning of the
> user buffer provided with the read system call. That means the user
> space interface now deviates from the in-kernel output buffer handling.
>
> For the cipher operation, the AAD buffer provided during input is
> pointed to by a new SGL which is chained with the output buffer SGL.
> With this approach, only pointers to one copy of the AAD are maintained
> to avoid data duplication.
>
> With this solution, the caller must not use sendpage with the exact same
> buffers for input and output. The following rationale applies: When
> the caller sends the same buffer for input/output to the sendpage
> operation, the cipher operation now will write the ciphertext to the
> beginning of the buffer where the AAD used to be. The subsequent tag
> calculation will now use the data it finds where the AAD is expected.
> As the cipher operation has already replaced the AAD with the ciphertext,
> the tag calculation will take the ciphertext as AAD and thus calculate
> a wrong tag.

If it's not much overhead, I suggest checking for this condition and 
returning an error.

Other than that, I've done a quick test of the patches using sendmsg() and 
read() and found that they work as expected.

Thanks,
Mat



> Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
> crypto/algif_aead.c | 143 +++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 113 insertions(+), 30 deletions(-)
>
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index c54bcb8..0212cc2 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -32,6 +32,7 @@ struct aead_sg_list {
> struct aead_async_rsgl {
> 	struct af_alg_sgl sgl;
> 	struct list_head list;
> +	bool new_page;
> };
>
> struct aead_async_req {
> @@ -405,6 +406,61 @@ static void aead_async_cb(struct crypto_async_request *_req, int err)
> 	iocb->ki_complete(iocb, err, err);
> }
>
> +/**
> + * scatterwalk_get_part() - get subset a scatterlist
> + *
> + * @dst: destination SGL to receive the pointers from source SGL
> + * @src: source SGL
> + * @len: data length in bytes to get from source SGL
> + * @max_sgs: number of SGs present in dst SGL to prevent overstepping boundaries
> + *
> + * @return: number of SG entries in dst
> + */
> +static inline int scatterwalk_get_part(struct scatterlist *dst,
> +				       struct scatterlist *src,
> +				       unsigned int len, unsigned int max_sgs)
> +{
> +	/* leave one SG entry for chaining */
> +	unsigned int j = 1;
> +
> +	while (len && j < max_sgs) {
> +		unsigned int todo = min_t(unsigned int, len, src->length);
> +
> +		sg_set_page(dst, sg_page(src), todo, src->offset);
> +		if (src->length >= len) {
> +			sg_mark_end(dst);
> +			break;
> +		}
> +		len -= todo;
> +		j++;
> +		src = sg_next(src);
> +		dst = sg_next(dst);
> +	}
> +
> +	return j;
> +}
> +
> +static inline int aead_alloc_rsgl(struct sock *sk, struct aead_async_rsgl **ret)
> +{
> +	struct aead_async_rsgl *rsgl =
> +				sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
> +	if (unlikely(!rsgl))
> +		return -ENOMEM;
> +	*ret = rsgl;
> +	return 0;
> +}
> +
> +static inline int aead_get_rsgl_areq(struct sock *sk,
> +				     struct aead_async_req *areq,
> +				     struct aead_async_rsgl **ret)
> +{
> +	if (list_empty(&areq->list)) {
> +		*ret = &areq->first_rsgl;
> +		return 0;
> +	} else
> +		return aead_alloc_rsgl(sk, ret);
> +}
> +
> static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
> 			      int flags)
> {
> @@ -433,7 +489,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
> 	if (!aead_sufficient_data(ctx))
> 		goto unlock;
>
> -	used = ctx->used;
> +	used = ctx->used - ctx->aead_assoclen;
> 	if (ctx->enc)
> 		outlen = used + as;
> 	else
> @@ -452,7 +508,6 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
> 	aead_request_set_ad(req, ctx->aead_assoclen);
> 	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> 				  aead_async_cb, sk);
> -	used -= ctx->aead_assoclen;
>
> 	/* take over all tx sgls from ctx */
> 	areq->tsgl = sock_kmalloc(sk, sizeof(*areq->tsgl) * sgl->cur,
> @@ -467,21 +522,26 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
>
> 	areq->tsgls = sgl->cur;
>
> +	/* set AAD buffer */
> +	err = aead_get_rsgl_areq(sk, areq, &rsgl);
> +	if (err)
> +		goto free;
> +	list_add_tail(&rsgl->list, &areq->list);
> +	sg_init_table(rsgl->sgl.sg, ALG_MAX_PAGES);
> +	rsgl->sgl.npages = scatterwalk_get_part(rsgl->sgl.sg, sgl->sg,
> +						ctx->aead_assoclen,
> +						ALG_MAX_PAGES);
> +	rsgl->new_page = false;
> +	last_rsgl = rsgl;
> +
> 	/* create rx sgls */
> 	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
> 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
> 				      (outlen - usedpages));
>
> -		if (list_empty(&areq->list)) {
> -			rsgl = &areq->first_rsgl;
> -
> -		} else {
> -			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
> -			if (unlikely(!rsgl)) {
> -				err = -ENOMEM;
> -				goto free;
> -			}
> -		}
> +		err = aead_get_rsgl_areq(sk, areq, &rsgl);
> +		if (err)
> +			goto free;
> 		rsgl->sgl.npages = 0;
> 		list_add_tail(&rsgl->list, &areq->list);
>
> @@ -491,6 +551,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
> 			goto free;
>
> 		usedpages += err;
> +		rsgl->new_page = true;
>
> 		/* chain the new scatterlist with previous one */
> 		if (last_rsgl)
> @@ -507,7 +568,7 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
>
> 		if (used < less) {
> 			err = -EINVAL;
> -			goto unlock;
> +			goto free;
> 		}
> 		used -= less;
> 		outlen -= less;
> @@ -531,7 +592,8 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
>
> free:
> 	list_for_each_entry(rsgl, &areq->list, list) {
> -		af_alg_free_sg(&rsgl->sgl);
> +		if (rsgl->new_page)
> +			af_alg_free_sg(&rsgl->sgl);
> 		if (rsgl != &areq->first_rsgl)
> 			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
> 	}
> @@ -545,6 +607,16 @@ static int aead_recvmsg_async(struct socket *sock, struct msghdr *msg,
> 	return err ? err : outlen;
> }
>
> +static inline int aead_get_rsgl_ctx(struct sock *sk, struct aead_ctx *ctx,
> +				    struct aead_async_rsgl **ret)
> +{
> +	if (list_empty(&ctx->list)) {
> +		*ret = &ctx->first_rsgl;
> +		return 0;
> +	} else
> +		return aead_alloc_rsgl(sk, ret);
> +}
> +
> static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
> {
> 	struct sock *sk = sock->sk;
> @@ -582,9 +654,6 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
> 			goto unlock;
> 	}
>
> -	/* data length provided by caller via sendmsg/sendpage */
> -	used = ctx->used;
> -
> 	/*
> 	 * Make sure sufficient data is present -- note, the same check is
> 	 * is also present in sendmsg/sendpage. The checks in sendpage/sendmsg
> @@ -598,6 +667,12 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
> 		goto unlock;
>
> 	/*
> +	 * The cipher operation input data is reduced by the associated data
> +	 * as the destination buffer will not hold the AAD.
> +	 */
> +	used = ctx->used - ctx->aead_assoclen;
> +
> +	/*
> 	 * Calculate the minimum output buffer size holding the result of the
> 	 * cipher operation. When encrypting data, the receiving buffer is
> 	 * larger by the tag length compared to the input buffer as the
> @@ -611,25 +686,29 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
> 		outlen = used - as;
>
> 	/*
> -	 * The cipher operation input data is reduced by the associated data
> -	 * length as this data is processed separately later on.
> +	 * Pre-pend the AAD buffer from the source SGL to the destination SGL.
> +	 * As the AAD buffer is not touched by the AEAD operation, the source
> +	 * SG buffers remain unchanged.
> 	 */
> -	used -= ctx->aead_assoclen;
> +	err = aead_get_rsgl_ctx(sk, ctx, &rsgl);
> +	if (err)
> +		goto unlock;
> +	list_add_tail(&rsgl->list, &ctx->list);
> +	sg_init_table(rsgl->sgl.sg, ALG_MAX_PAGES);
> +	rsgl->sgl.npages = scatterwalk_get_part(rsgl->sgl.sg, sgl->sg,
> +						ctx->aead_assoclen,
> +						ALG_MAX_PAGES);
> +	rsgl->new_page = false;
> +	last_rsgl = rsgl;
>
> 	/* convert iovecs of output buffers into scatterlists */
> 	while (outlen > usedpages && iov_iter_count(&msg->msg_iter)) {
> 		size_t seglen = min_t(size_t, iov_iter_count(&msg->msg_iter),
> 				      (outlen - usedpages));
>
> -		if (list_empty(&ctx->list)) {
> -			rsgl = &ctx->first_rsgl;
> -		} else {
> -			rsgl = sock_kmalloc(sk, sizeof(*rsgl), GFP_KERNEL);
> -			if (unlikely(!rsgl)) {
> -				err = -ENOMEM;
> -				goto unlock;
> -			}
> -		}
> +		err = aead_get_rsgl_ctx(sk, ctx, &rsgl);
> +		if (err)
> +			goto unlock;
> 		rsgl->sgl.npages = 0;
> 		list_add_tail(&rsgl->list, &ctx->list);
>
> @@ -637,7 +716,10 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
> 		err = af_alg_make_sg(&rsgl->sgl, &msg->msg_iter, seglen);
> 		if (err < 0)
> 			goto unlock;
> +
> 		usedpages += err;
> +		rsgl->new_page = true;
> +
> 		/* chain the new scatterlist with previous one */
> 		if (last_rsgl)
> 			af_alg_link_sg(&last_rsgl->sgl, &rsgl->sgl);
> @@ -688,7 +770,8 @@ static int aead_recvmsg_sync(struct socket *sock, struct msghdr *msg, int flags)
>
> unlock:
> 	list_for_each_entry_safe(rsgl, tmp, &ctx->list, list) {
> -		af_alg_free_sg(&rsgl->sgl);
> +		if (rsgl->new_page)
> +			af_alg_free_sg(&rsgl->sgl);
> 		if (rsgl != &ctx->first_rsgl)
> 			sock_kfree_s(sk, rsgl, sizeof(*rsgl));
> 		list_del(&rsgl->list);
> -- 
> 2.7.4
>
>
>

--
Mat Martineau
Intel OTC

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Thu, Nov 10, 2016 at 04:32:03AM +0100, Stephan Mueller wrote:
> The kernel crypto API AEAD cipher operation generates output such that
> space for the AAD is reserved in the output buffer without being
> touched. The processed ciphertext/plaintext is appended to the reserved
> AAD buffer.
> 
> The user space interface followed that approach. However, this is a
> violation of the POSIX read definition which requires that any read data
> is placed at the beginning of the caller-provided buffer. As the kernel
> crypto API would leave room for the AAD, the old approach did not fully
> comply with the POSIX specification.

Nack.  The kernel AEAD API will copy the AD as is, it definitely
does not leave the output untouched unless of course when it is
an in-place operation.  The user-space operation should operate
in the same manner.

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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

Am Samstag, 12. November 2016, 09:55:19 CET schrieb Herbert Xu:

Hi Herbert,

> On Thu, Nov 10, 2016 at 04:32:03AM +0100, Stephan Mueller wrote:
> > The kernel crypto API AEAD cipher operation generates output such that
> > space for the AAD is reserved in the output buffer without being
> > touched. The processed ciphertext/plaintext is appended to the reserved
> > AAD buffer.
> > 
> > The user space interface followed that approach. However, this is a
> > violation of the POSIX read definition which requires that any read data
> > is placed at the beginning of the caller-provided buffer. As the kernel
> > crypto API would leave room for the AAD, the old approach did not fully
> > comply with the POSIX specification.
> 
> Nack.  The kernel AEAD API will copy the AD as is, it definitely
> does not leave the output untouched unless of course when it is
> an in-place operation.  The user-space operation should operate
> in the same manner.

When you have separate buffers, the kernel does not seem to copy the AD over 
to the target buffer.
> 
> Cheers,


Ciao
Stephan

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

Am Freitag, 11. November 2016, 16:26:12 CET schrieb Mat Martineau:

Hi Mat,
> > 
> > With this solution, the caller must not use sendpage with the exact same
> > buffers for input and output. The following rationale applies: When
> > the caller sends the same buffer for input/output to the sendpage
> > operation, the cipher operation now will write the ciphertext to the
> > beginning of the buffer where the AAD used to be. The subsequent tag
> > calculation will now use the data it finds where the AAD is expected.
> > As the cipher operation has already replaced the AAD with the ciphertext,
> > the tag calculation will take the ciphertext as AAD and thus calculate
> > a wrong tag.
> 
> If it's not much overhead, I suggest checking for this condition and
> returning an error.

I can surely look into that. But Herbert's NACK seems to make this patch 
unlikely.
> 
> Other than that, I've done a quick test of the patches using sendmsg() and
> read() and found that they work as expected.
> 
Thanks for testing.

Ciao
Stephan

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Sat, Nov 12, 2016 at 03:03:36AM +0100, Stephan Mueller wrote:
> 
> When you have separate buffers, the kernel does not seem to copy the AD over 
> to the target buffer.

OK we should definitely fix that.

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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

Am Samstag, 12. November 2016, 10:13:02 CET schrieb Herbert Xu:

Hi Herbert,

> On Sat, Nov 12, 2016 at 03:03:36AM +0100, Stephan Mueller wrote:
> > When you have separate buffers, the kernel does not seem to copy the AD
> > over to the target buffer.
> 
> OK we should definitely fix that.

Shall the fix be rolled into the patch together with the fix for the tag value 
as well as the crash fix? Or can we have a stand-alone patch fixing this.

Btw., how do you suggest that should be fixed? I would assume that this needs 
to be fixed on a per-implementation basis. I tested authenc and it copies the 
buffers over. gcm_base(ctr-aes-aesni,ghash-clmulni), rfc4106-gcm-aesni or 
ccm_base(ctr-aes-aesni,aes-aesni) do not copy the AD over.

Ciao
Stephan

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Tue, Nov 15, 2016 at 08:06:47PM +0100, Stephan Mueller wrote:
>
> Shall the fix be rolled into the patch together with the fix for the tag value 
> as well as the crash fix? Or can we have a stand-alone patch fixing this.

I think these are two separate issues and we don't need to fix them
all in one go.

> Btw., how do you suggest that should be fixed? I would assume that this needs 
> to be fixed on a per-implementation basis. I tested authenc and it copies the 
> buffers over. gcm_base(ctr-aes-aesni,ghash-clmulni), rfc4106-gcm-aesni or 
> ccm_base(ctr-aes-aesni,aes-aesni) do not copy the AD over.

We should fix as much as we can and then add a testmgr test to find
the rest.

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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Wed, Nov 16, 2016 at 04:57:42PM +0800, Herbert Xu wrote:
> On Tue, Nov 15, 2016 at 08:06:47PM +0100, Stephan Mueller wrote:
> >
> > Shall the fix be rolled into the patch together with the fix for the tag value 
> > as well as the crash fix? Or can we have a stand-alone patch fixing this.
> 
> I think these are two separate issues and we don't need to fix them
> all in one go.
> 
> > Btw., how do you suggest that should be fixed? I would assume that this needs 
> > to be fixed on a per-implementation basis. I tested authenc and it copies the 
> > buffers over. gcm_base(ctr-aes-aesni,ghash-clmulni), rfc4106-gcm-aesni or 
> > ccm_base(ctr-aes-aesni,aes-aesni) do not copy the AD over.
> 
> We should fix as much as we can and then add a testmgr test to find
> the rest.

Alternatively we can add the copying code to algif_aead when src !=
dst.  I think that's probably the easier fix.

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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

Am Mittwoch, 16. November 2016, 16:57:42 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Nov 15, 2016 at 08:06:47PM +0100, Stephan Mueller wrote:
> > Shall the fix be rolled into the patch together with the fix for the tag
> > value as well as the crash fix? Or can we have a stand-alone patch fixing
> > this.
> I think these are two separate issues and we don't need to fix them
> all in one go.

I will resubmit the patches regarding the tag and the bug fix then.
> 
> > Btw., how do you suggest that should be fixed? I would assume that this
> > needs to be fixed on a per-implementation basis. I tested authenc and it
> > copies the buffers over. gcm_base(ctr-aes-aesni,ghash-clmulni),
> > rfc4106-gcm-aesni or ccm_base(ctr-aes-aesni,aes-aesni) do not copy the AD
> > over.
> 
> We should fix as much as we can and then add a testmgr test to find
> the rest.

One thing occurred to me: The copying of the AD would only be done of src != 
dst. For the AF_ALG interface, I thing we always have src != dst due to the 
user space/kernel space translation. That means the kernel copies the AD 
around even in user space src == dst. Isn't that a waste? I.e. shouldn't we 
handle the AD copying rather in user space than in kernel space?

Ciao
Stephan

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Wed, Nov 16, 2016 at 10:02:59AM +0100, Stephan Mueller wrote:
>
> One thing occurred to me: The copying of the AD would only be done of src != 
> dst. For the AF_ALG interface, I thing we always have src != dst due to the 
> user space/kernel space translation. That means the kernel copies the AD 
> around even in user space src == dst. Isn't that a waste? I.e. shouldn't we 
> handle the AD copying rather in user space than in kernel space?

No that's not the case.  You can do zero-copy, in which case src
would be identical 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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Wed, Nov 16, 2016 at 10:02:59AM +0100, Stephan Mueller wrote:
> Am Mittwoch, 16. November 2016, 16:57:42 CET schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Tue, Nov 15, 2016 at 08:06:47PM +0100, Stephan Mueller wrote:
> > > Shall the fix be rolled into the patch together with the fix for the tag
> > > value as well as the crash fix? Or can we have a stand-alone patch fixing
> > > this.
> > I think these are two separate issues and we don't need to fix them
> > all in one go.
> 
> I will resubmit the patches regarding the tag and the bug fix then.

Also if we go the route of doing the copy in algif_aead then we don't
need to touch the crypto code at all.

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 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Stephan Mueller]

Am Mittwoch, 16. November 2016, 17:04:46 CET schrieb Herbert Xu:

Hi Herbert,

> On Wed, Nov 16, 2016 at 10:02:59AM +0100, Stephan Mueller wrote:
> > One thing occurred to me: The copying of the AD would only be done of src
> > != dst. For the AF_ALG interface, I thing we always have src != dst due
> > to the user space/kernel space translation. That means the kernel copies
> > the AD around even in user space src == dst. Isn't that a waste? I.e.
> > shouldn't we handle the AD copying rather in user space than in kernel
> > space?
> 
> No that's not the case.  You can do zero-copy, in which case src
> would be identical to dst.

The way to go on this topic would be to use the same logic as the authenc 
implementation by using a null cipher for the copy operation. Though, finding 
out whether the src and dst buffers are the same is an interesting 
proposition, because we need to traverse the src and dst SGLs to see whether 
the same pages and same offsets are used. A simple check for src SGL == dst 
SGL will not work for the AF_ALG implementation, because the src SGL will 
always be different from the dst SGL because they are constructed in different 
ways (tsgl will always be different from rsgl). What may be the same are the 
pages and offsets that are pointed to by the SGL in case of zerocopy.

Keeping that in mind, I am wondering whether the authenc() implementation 
should be changed to simply remove the copy operation in there. As there seem 
to be no other AEAD cipher implements that copy operation (at least the major 
CCM and GCM implementations applicable to X86 do not do that), it seems that 
it is not necessary at all for in-kernel users. The authenc implementation 
performs the copy operation of the src SGL if it is different from the dst 
SGL. See the following code used by authenc:

        if (req->src != req->dst) {
                err = crypto_authenc_copy_assoc(req);
                if (err)
                        return err;

                dst = scatterwalk_ffwd(areq_ctx->dst, req->dst, req-
>assoclen);
        }

Thus, the authenc implementation will always copy the AAD over in case of 
AF_ALG even though zerocopy with the same buffers are used.

When the in-kernel users of AEAD seemingly do not care about the copying of 
the AAD, and considering that authenc would not do it right for AF_ALG, I am 
wondering whether we should:

1. remove the AAD copy in authenc to make it en-par with the other AEAD 
implementations

2. re-consider the discussed patch

3. tell users to copy the AAD over if they need it in the dst buffers.

Ciao
Stephan

Re: [PATCH 2/3] crypto: AF_ALG - disregard AAD buffer space for output

[Herbert Xu]

On Sat, Nov 19, 2016 at 10:08:06PM +0100, Stephan Mueller wrote:
> 
> The way to go on this topic would be to use the same logic as the authenc 
> implementation by using a null cipher for the copy operation. Though, finding 
> out whether the src and dst buffers are the same is an interesting 
> proposition, because we need to traverse the src and dst SGLs to see whether 
> the same pages and same offsets are used. A simple check for src SGL == dst 
> SGL will not work for the AF_ALG implementation, because the src SGL will 
> always be different from the dst SGL because they are constructed in different 
> ways (tsgl will always be different from rsgl). What may be the same are the 
> pages and offsets that are pointed to by the SGL in case of zerocopy.

True, you won't be able to use the fast path and will end up doing
the copy.  However, it should still work because we also have an
aliasing check within the null copy code (skcipher_null_crypt).

So for the sake of simplicity, I think we should start with just
copying src to dst unconditionally in algif_aead, and then calling
the underlying crypto code with just dst to invoke the inplace 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