[PATCH] crypto: fix AEAD tag memory handling

[PATCH] crypto: fix AEAD tag memory handling

[Stephan Mueller]

Hi Herbert,

for this patch, I have updated the testing for libkcapi accordingly and all
tests pass. I will push the libkcapi code 0.12 with that test code change
out shortly. Using the current upstream version of 0.11.1 will show failures
as it expects the correct recv return code. As stated below, that return
code has changed which implies that some of the tests will fail.

---8<---

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 user 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 memory 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, the changes are
transparent to user space with one exception: the return code of recv
indicates the processed of output buffer size. 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

Re: [PATCH] crypto: fix AEAD tag memory handling

[Mat Martineau]

Stephan and Herbert,

On Thu, 27 Oct 2016, Stephan Mueller wrote:

> Hi Herbert,
>
> for this patch, I have updated the testing for libkcapi accordingly and all
> tests pass. I will push the libkcapi code 0.12 with that test code change
> out shortly. Using the current upstream version of 0.11.1 will show failures
> as it expects the correct recv return code. As stated below, that return
> code has changed which implies that some of the tests will fail.
>
> ---8<---
>
> 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 user 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 memory 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, the changes are
> transparent to user space with one exception: the return code of recv
> indicates the processed of output buffer size. 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.

I tested out these changes and found that recv() or read() do not update 
all of the indicated bytes. In your decrypt example where there are 16 
bytes AAD, 16 bytes CT, and 16 bytes tag, read(sk, buf, 32) returns 32. 
However, the first 16 bytes of buf are *unchanged* and the second 16 bytes 
contain the plaintext. In other words, 16 bytes were skipped over and 16 
bytes were read.

I see how this is similar to the documented use of the dst buffer in 
aead_request_set_crypt(), but it is not consistent with POSIX read() 
semantics.


--
Mat Martineau
Intel OTC

Re: [PATCH] crypto: fix AEAD tag memory handling

[Stephan Mueller]

Am Donnerstag, 27. Oktober 2016, 14:42:14 CEST schrieb Mat Martineau:

Hi Mat,

> Stephan and Herbert,
> 
> On Thu, 27 Oct 2016, Stephan Mueller wrote:
> > Hi Herbert,
> > 
> > for this patch, I have updated the testing for libkcapi accordingly and
> > all
> > tests pass. I will push the libkcapi code 0.12 with that test code change
> > out shortly. Using the current upstream version of 0.11.1 will show
> > failures as it expects the correct recv return code. As stated below,
> > that return code has changed which implies that some of the tests will
> > fail.
> > 
> > ---8<---
> > 
> > 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 user 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 memory 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, the changes are
> > transparent to user space with one exception: the return code of recv
> > indicates the processed of output buffer size. 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.
> 
> I tested out these changes and found that recv() or read() do not update
> all of the indicated bytes. In your decrypt example where there are 16
> bytes AAD, 16 bytes CT, and 16 bytes tag, read(sk, buf, 32) returns 32.

Please check the current patch (the one I sent to you as a pre-release did not 
contain the fix for the decryption part -- the patch sent to the list and what 
we discuss here contains the appropriate handling for the decryption side).

With your example using 16 bytes AAD, 16 bytes CT and 16 bytes Tag, the read 
will *only* show the return code of 16 (bytes) now, because only the CT part 
is converted into PT.

Yet, you are completely correct that the first 16 bytes of the AAD are skipped 
by the read call.

Thus, the read call returns exactly the amount of changed bytes, but it 
deviates from the POSIX logic by seeking to the end of the AAD buffer to find 
the offset it shall place the resulting data to.

> However, the first 16 bytes of buf are *unchanged* and the second 16 bytes
> contain the plaintext. In other words, 16 bytes were skipped over and 16
> bytes were read.

Correct. Again, with the current patch we discuss here, the read will return 
16 as it processed 16 bytes.
> 
> I see how this is similar to the documented use of the dst buffer in
> aead_request_set_crypt(), but it is not consistent with POSIX read()
> semantics.

Ciao
Stephan

Re: [PATCH] crypto: fix AEAD tag memory handling

[Mat Martineau]

Stephan,

On Fri, 28 Oct 2016, Stephan Mueller wrote:

> Am Donnerstag, 27. Oktober 2016, 14:42:14 CEST schrieb Mat Martineau:
>
> Hi Mat,
>
>> Stephan and Herbert,
>>
>> On Thu, 27 Oct 2016, Stephan Mueller wrote:
>>> Hi Herbert,
>>>
>>> for this patch, I have updated the testing for libkcapi accordingly and
>>> all
>>> tests pass. I will push the libkcapi code 0.12 with that test code change
>>> out shortly. Using the current upstream version of 0.11.1 will show
>>> failures as it expects the correct recv return code. As stated below,
>>> that return code has changed which implies that some of the tests will
>>> fail.
>>>
>>> ---8<---
>>>
>>> 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 user 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 memory 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, the changes are
>>> transparent to user space with one exception: the return code of recv
>>> indicates the processed of output buffer size. 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.
>>
>> I tested out these changes and found that recv() or read() do not update
>> all of the indicated bytes. In your decrypt example where there are 16
>> bytes AAD, 16 bytes CT, and 16 bytes tag, read(sk, buf, 32) returns 32.
>
> Please check the current patch (the one I sent to you as a pre-release did not
> contain the fix for the decryption part -- the patch sent to the list and what
> we discuss here contains the appropriate handling for the decryption side).
>
> With your example using 16 bytes AAD, 16 bytes CT and 16 bytes Tag, the read
> will *only* show the return code of 16 (bytes) now, because only the CT part
> is converted into PT.
>
> Yet, you are completely correct that the first 16 bytes of the AAD are skipped
> by the read call.
>
> Thus, the read call returns exactly the amount of changed bytes, but it
> deviates from the POSIX logic by seeking to the end of the AAD buffer to find
> the offset it shall place the resulting data to.

I re-built my kernel using the patch from this email thread, and I still 
see the total read() length including the "skipped" AAD byte count. Here's 
an strace excerpt for a decrypt operation with AAD length of 32 and 
plaintext length of 32:

sendmsg(5, {msg_name=NULL, msg_namelen=0, 
msg_iov=[{iov_base="*\200\233\350Zg\306\5ck\240\t\344\177\336 h\321\205\214<P^H~l\243\353w\34\377\316", iov_len=32}, 
{iov_base="\205\256}\336\27\276\31\333\321&\254w\244\244\323h\221)\254}\345s\227\242>f\266\2020\212\220\322"..., 
iov_len=48}], msg_iovlen=2, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3},
{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x4}, {cmsg_len=36, cmsg_level=SOL_ALG, cmsg_type=0x2}],
msg_controllen=88, msg_flags=0}, 0) = 80
read(5, "\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1\1"..., 64) = 64


The libkcapi test behaves similarly (AEAD test 11):

sendmsg(6, {msg_name=NULL, msg_namelen=0, 
msg_iov=[{iov_base="\373{\303\4\243\220\236f\342\340\305\357\225'\22\335\210L\343\3472Aq6\237,]\261\255\304\214}"..., 
iov_len=68}], msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3},
{cmsg_len=40, cmsg_level=SOL_ALG, cmsg_type=0x2}, {cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x4}],
msg_controllen=88, msg_flags=0}, 0) = 68
read(6, "\373{\303\4\243\220\236f\342\340\305\357\225'\22\335\210L\343\3472Aq6\237,]\261\255\304\214}"..., 68) = 64


Even with read() returning only the number of changed bytes, what would 
you think if there was one filesystem that updated the last bytes of a 
buffer in a read() call instead of the first? It's important to maintain 
the standard POSIX semantics and only update the bytes starting at the 
beginning of the buffer.

>
>> However, the first 16 bytes of buf are *unchanged* and the second 16 bytes
>> contain the plaintext. In other words, 16 bytes were skipped over and 16
>> bytes were read.
>
> Correct. Again, with the current patch we discuss here, the read will return
> 16 as it processed 16 bytes.
>>
>> I see how this is similar to the documented use of the dst buffer in
>> aead_request_set_crypt(), but it is not consistent with POSIX read()
>> semantics.

Regards,

--
Mat Martineau
Intel OTC

Re: [PATCH] crypto: fix AEAD tag memory handling

[Stephan Mueller]

Am Freitag, 28. Oktober 2016, 15:21:19 CET schrieb Mat Martineau:

Hi Mat,
> > 
> > Please check the current patch (the one I sent to you as a pre-release did
> > not contain the fix for the decryption part -- the patch sent to the list
> > and what we discuss here contains the appropriate handling for the
> > decryption side).
> > 
> > With your example using 16 bytes AAD, 16 bytes CT and 16 bytes Tag, the
> > read will *only* show the return code of 16 (bytes) now, because only the
> > CT part is converted into PT.
> > 
> > Yet, you are completely correct that the first 16 bytes of the AAD are
> > skipped by the read call.
> > 
> > Thus, the read call returns exactly the amount of changed bytes, but it
> > deviates from the POSIX logic by seeking to the end of the AAD buffer to
> > find the offset it shall place the resulting data to.
> 
> I re-built my kernel using the patch from this email thread, and I still
> see the total read() length including the "skipped" AAD byte count. Here's
> an strace excerpt for a decrypt operation with AAD length of 32 and
> plaintext length of 32:

That is absolutely correct as the patch' intention is to avoid the 
superflowous Tag memory consideration for input data during encryption and for 
output data for decryption. This prevents the kernel from reading/writing 
memory that it does not need to touch.

The patch is not intended for coverting the AAD you reported. Though, I am not 
yet sure about whether we need to cover that aspect. My interpretation is that 
the kernel is responsible for the entire memory of AAD || PT/CT and 
potentially the tag. If the kernel sees that it does not need to change the 
AAD, it will not do that and simply change the parts of the buffer it needs 
to. Then, the kernel reports the changed bytes.

Note, if we change the kernel to operate as you suggest, there is more memory 
re-calculation operation in the kernel as well as in user space. To me, that 
is an invtation to errors. Besides, I see that only as an interpretation of 
how POSIX is to be applied.

However, I can make another proposal: we change the read/recv handler such 
that it returns the actually processed memory, regardless whether it really 
touched it. I.e. read will return the following bytes in its return code:

- encryption: AAD + CT + Tag

- decryption: AAD + PT

Even though read would report that amount of memory, we all know that the AAD 
part will not be actually written. Yet, the kernel returns the amount of bytes 
it processed.

Regardless, all of this discussion revolves around a topic that is separate to 
this patch. I.e. this patch was not intended to handle the AAD. This patch is 
only provided to handle the tag.

...


Ciao
Stephan

Re: [PATCH] crypto: fix AEAD tag memory handling

[Mat Martineau]

Hi Stephan -

On Mon, 31 Oct 2016, Stephan Mueller wrote:

> Am Freitag, 28. Oktober 2016, 15:21:19 CET schrieb Mat Martineau:
>
> Hi Mat,
>>>
>>> Please check the current patch (the one I sent to you as a pre-release did
>>> not contain the fix for the decryption part -- the patch sent to the list
>>> and what we discuss here contains the appropriate handling for the
>>> decryption side).
>>>
>>> With your example using 16 bytes AAD, 16 bytes CT and 16 bytes Tag, the
>>> read will *only* show the return code of 16 (bytes) now, because only the
>>> CT part is converted into PT.
>>>
>>> Yet, you are completely correct that the first 16 bytes of the AAD are
>>> skipped by the read call.
>>>
>>> Thus, the read call returns exactly the amount of changed bytes, but it
>>> deviates from the POSIX logic by seeking to the end of the AAD buffer to
>>> find the offset it shall place the resulting data to.
>>
>> I re-built my kernel using the patch from this email thread, and I still
>> see the total read() length including the "skipped" AAD byte count. Here's
>> an strace excerpt for a decrypt operation with AAD length of 32 and
>> plaintext length of 32:
>
> That is absolutely correct as the patch' intention is to avoid the
> superflowous Tag memory consideration for input data during encryption and for
> output data for decryption. This prevents the kernel from reading/writing
> memory that it does not need to touch.
>
> The patch is not intended for coverting the AAD you reported. Though, I am not
> yet sure about whether we need to cover that aspect. My interpretation is that
> the kernel is responsible for the entire memory of AAD || PT/CT and
> potentially the tag. If the kernel sees that it does not need to change the
> AAD, it will not do that and simply change the parts of the buffer it needs
> to. Then, the kernel reports the changed bytes.
>
> Note, if we change the kernel to operate as you suggest, there is more memory
> re-calculation operation in the kernel as well as in user space. To me, that
> is an invtation to errors.

To be clear: my preference is to have read() copy only PT or CT||Tag bytes 
in to the provided buffer. sendmsg() is for input, read() is for output. 
AAD is input and was passed to the kernel in the sendmsg() call.

My second choice is to write the AAD bytes to the read() buffer in order 
to work better with existing userspace code.


I don't see that there is extra userspace manipulation of memory if the 
read() buffer does not include space for AAD. The userspace program just 
passes a pointer to the location for PT or CT||Tag as the buffer pointer 
for read() - the bytes for AAD may already be in the memory preceding that 
pointer.

> Besides, I see that only as an interpretation of
> how POSIX is to be applied.

We seem to be at an impasse here: I contend that this aspect of POSIX 
read() is not open to interpretation. When read() returns a positive 
number N, that means the *first* N bytes of the buffer were updated. 
Making the programmer second guess which N bytes of the buffer were 
changed depending on which filesystem / socket type / etc is in use for 
that file descriptor is a recipe for serious bugs and seriously breaks 
the "everything is a file" abstraction.

> However, I can make another proposal: we change the read/recv handler such
> that it returns the actually processed memory, regardless whether it really
> touched it. I.e. read will return the following bytes in its return code:
>
> - encryption: AAD + CT + Tag
>
> - decryption: AAD + PT
>
> Even though read would report that amount of memory, we all know that the AAD
> part will not be actually written. Yet, the kernel returns the amount of bytes
> it processed.

This is what I was attempting to document in my previous reply: strace is 
showing that this is already the behavior implemented by the current 
patch.

> Regardless, all of this discussion revolves around a topic that is separate to
> this patch. I.e. this patch was not intended to handle the AAD. This patch is
> only provided to handle the tag.

My main concern is getting the semantics correct and consistent in a 
single patch series. It would be a big problem to explain that AF_ALG AEAD 
read and write works one way in 4.x, another way in 4.y, and some 
different way in 4.z.

--
Mat Martineau
Intel OTC

Re: [PATCH] crypto: fix AEAD tag memory handling

[Stephan Mueller]

Am Montag, 31. Oktober 2016, 16:18:32 CET schrieb Mat Martineau:

Hi Mat,

> My main concern is getting the semantics correct and consistent in a
> single patch series. It would be a big problem to explain that AF_ALG AEAD
> read and write works one way in 4.x, another way in 4.y, and some
> different way in 4.z.

Ok, I will prepare another patch with this one fixed as well.


Ciao
Stephan

Re: [PATCH] crypto: fix AEAD tag memory handling

[Stephan Mueller]

[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

Am Montag, 31. Oktober 2016, 16:18:32 CET schrieb Mat Martineau:

Hi Mat,
> 
> My main concern is getting the semantics correct and consistent in a
> single patch series. It would be a big problem to explain that AF_ALG AEAD
> read and write works one way in 4.x, another way in 4.y, and some
> different way in 4.z.

I do have a patch now available that exactly does what you suggest. See the 
patch attached. It works with the following exception.

In the case of sendpage and using an in-place cipher operation, the patch 
breaks as follows. When the caller sends the same buffer for a 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.

Thus, the only way to avoid that would be to duplicate the AAD into an 
internal buffer. But that would defeat the entire purpose of sendpage.

The patch, however, works with sendmsg as well as sendpage when the src and 
dst buffers are different.

Ciao
Stephan

[-- Attachment #2: fix_read_offset.diff --]
[-- Type: text/x-patch, Size: 7509 bytes --]

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index c54bcb8..c8efd01 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)
@@ -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);

Re: [PATCH] crypto: fix AEAD tag memory handling

[Mat Martineau]

Stephan,

On Wed, 9 Nov 2016, Stephan Mueller wrote:

> Am Montag, 31. Oktober 2016, 16:18:32 CET schrieb Mat Martineau:
>
> Hi Mat,
>>
>> My main concern is getting the semantics correct and consistent in a
>> single patch series. It would be a big problem to explain that AF_ALG AEAD
>> read and write works one way in 4.x, another way in 4.y, and some
>> different way in 4.z.
>
> I do have a patch now available that exactly does what you suggest. See the
> patch attached. It works with the following exception.
>
> In the case of sendpage and using an in-place cipher operation, the patch
> breaks as follows. When the caller sends the same buffer for a 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.
>
> Thus, the only way to avoid that would be to duplicate the AAD into an
> internal buffer. But that would defeat the entire purpose of sendpage.

The use case for an "in place" operation would be to have the ciphertext 
overwrite the plaintext, correct? If the src and dst overlap, does it make 
sense to require the plaintext and ciphertext to be in exactly the same 
location?

> The patch, however, works with sendmsg as well as sendpage when the src and
> dst buffers are different.

Thanks - I tested your patch and found that it works as expected with 
sendmsg.


Regards,

--
Mat Martineau
Intel OTC