| Message ID | 4006219.iWaCqWYv1B@positron.chronox.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)