| Message ID | 2319539.qMqmu7oTgO@positron.chronox.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
On Mon, Dec 05, 2016 at 03:26:19PM +0100, Stephan Mueller wrote: > Hi Herbert, > > Changes v4: restore the old behavior -- if the caller does not provide sufficient > output buffer size, return an error. > > ---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 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. > > 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> Hmm, I don't see the code that copies the AAD over. Did I miss it? Thanks,
Am Mittwoch, 7. Dezember 2016, 19:42:45 CET schrieb Herbert Xu: Hi Herbert, > > Hmm, I don't see the code that copies the AAD over. Did I miss it? You do not miss it. As I mentioned in a previous email, I would like: - to include the current patch around the tag now as it has been on the mailing list for a long time -- that current patch fixes the superfluous copying of the tag value. That fix of the tag handling is unrelated to any AAD logic. However, it changes the user-visible memory structure and therefore the user interface (i.e. during decryption, the returned data is smaller than it used to be and during encryption, less data is required as input). - the AAD handling with the copying over of data shall come in the next development cycle for 4.10. That change is not user visible as it does not change the memory structure and thus the user interface. Also, the AAD change is unrelated to the tag change provided with this patch. 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
On Wed, Dec 07, 2016 at 12:49:25PM +0100, Stephan Müller wrote: > Am Mittwoch, 7. Dezember 2016, 19:42:45 CET schrieb Herbert Xu: > > Hi Herbert, > > > > Hmm, I don't see the code that copies the AAD over. Did I miss it? > > You do not miss it. As I mentioned in a previous email, I would like: > > - to include the current patch around the tag now as it has been on the > mailing list for a long time -- that current patch fixes the superfluous > copying of the tag value. That fix of the tag handling is unrelated to any AAD > logic. However, it changes the user-visible memory structure and therefore the > user interface (i.e. during decryption, the returned data is smaller than it > used to be and during encryption, less data is required as input). > > - the AAD handling with the copying over of data shall come in the next > development cycle for 4.10. That change is not user visible as it does not > change the memory structure and thus the user interface. Also, the AAD change > is unrelated to the tag change provided with this patch. OK, thanks for explaining this Stephan.
On Mon, Dec 05, 2016 at 03:26:19PM +0100, Stephan Mueller wrote: > Hi Herbert, > > Changes v4: restore the old behavior -- if the caller does not provide sufficient > output buffer size, return an error. Patch applied. Thanks.
Am Mittwoch, 7. Dezember 2016, 20:09:14 CET schrieb Herbert Xu: Hi Herbert, > On Mon, Dec 05, 2016 at 03:26:19PM +0100, Stephan Mueller wrote: > > Hi Herbert, > > > > Changes v4: restore the old behavior -- if the caller does not provide > > sufficient output buffer size, return an error. > > Patch applied. Thanks. Thank you. I will prepare the release of libkcapi 0.13.0 now which contains a provision for this user interface change. This provision allows a calling application to execute on older or newer kernels without changing its logic. The library transparently takes care of that user space interface difference. 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 6e95137..4359c9b 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, @@ -462,7 +469,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)); @@ -492,16 +499,14 @@ 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) { + err = -EINVAL; + goto unlock; + } aead_request_set_crypt(req, areq->tsgl, areq->first_rsgl.sgl.sg, used, areq->iv); @@ -572,6 +577,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; /* @@ -586,16 +592,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)); @@ -622,16 +639,14 @@ 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) + if (usedpages < outlen) { + err = -EINVAL; goto unlock; + } sg_mark_end(sgl->sg + sgl->cur - 1); aead_request_set_crypt(&ctx->aead_req, sgl->sg, ctx->first_rsgl.sgl.sg,
Hi Herbert, Changes v4: restore the old behavior -- if the caller does not provide sufficient output buffer size, return an error. ---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 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. 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 | 57 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 21 deletions(-)