| Message ID | 3506033.FskOdlTquT@positron.chronox.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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 -- 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 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,
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 -- 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, 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 -- 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 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,
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 -- 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 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,
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,
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 -- 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, 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,
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,
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 -- 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 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,
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);
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(-)