| Message ID | 1679829.1686785273@warthog.procyon.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] crypto: af_alg/hash: Fix recvmsg() after sendmsg(MSG_MORE) | expand |
On Thu, Jun 15, 2023 at 12:27:53AM +0100, David Howells wrote: > > If an AF_ALG socket bound to a hashing algorithm is sent a zero-length > message with MSG_MORE set and then recvmsg() is called without first > sending another message without MSG_MORE set to end the operation, an oops > will occur because the crypto context and result doesn't now get set up in > advance because hash_sendmsg() now defers that as long as possible in the > hope that it can use crypto_ahash_digest() - and then because the message > is zero-length, it the data wrangling loop is skipped. > > Fix this by always making a pass of the loop, even in the case that no data > is provided to the sendmsg(). > > Fix also extract_iter_to_sg() to handle a zero-length iterator by returning > 0 immediately. > > Whilst we're at it, remove the code to create a kvmalloc'd scatterlist if > we get more than ALG_MAX_PAGES - this shouldn't happen. I don't think this is right. If it's a zero-length message with MSG_MORE set, it should be ignored until a recvmsg(2) call is made. In any case, this patch doesn't fix all the syzbot reports. We need to think about reverting this change if it can't be fixed in time. Thanks,
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> In any case, this patch doesn't fix all the syzbot reports.
One of them I can't actually reproduce locally, but I have two more patches
that might fix it.
David
On Thu, Jun 15, 2023 at 12:27:53AM +0100, David Howells wrote: > > If an AF_ALG socket bound to a hashing algorithm is sent a zero-length > message with MSG_MORE set and then recvmsg() is called without first > sending another message without MSG_MORE set to end the operation, an oops > will occur because the crypto context and result doesn't now get set up in > advance because hash_sendmsg() now defers that as long as possible in the > hope that it can use crypto_ahash_digest() - and then because the message > is zero-length, it the data wrangling loop is skipped. > > Fix this by always making a pass of the loop, even in the case that no data > is provided to the sendmsg(). > > Fix also extract_iter_to_sg() to handle a zero-length iterator by returning > 0 immediately. > > Whilst we're at it, remove the code to create a kvmalloc'd scatterlist if > we get more than ALG_MAX_PAGES - this shouldn't happen. > > Fixes: c662b043cdca ("crypto: af_alg/hash: Support MSG_SPLICE_PAGES") > Reported-by: syzbot+13a08c0bf4d212766c3c@syzkaller.appspotmail.com > Link: https://lore.kernel.org/r/000000000000b928f705fdeb873a@google.com/ > Reported-by: syzbot+14234ccf6d0ef629ec1a@syzkaller.appspotmail.com > Link: https://lore.kernel.org/r/000000000000c047db05fdeb8790@google.com/ > Reported-by: syzbot+4e2e47f32607d0f72d43@syzkaller.appspotmail.com > Link: https://lore.kernel.org/r/000000000000bcca3205fdeb87fb@google.com/ > Reported-by: syzbot+472626bb5e7c59fb768f@syzkaller.appspotmail.com > Link: https://lore.kernel.org/r/000000000000b55d8805fdeb8385@google.com/ > Signed-off-by: David Howells <dhowells@redhat.com> > Tested-by: syzbot+13a08c0bf4d212766c3c@syzkaller.appspotmail.com > Tested-by: syzbot+14234ccf6d0ef629ec1a@syzkaller.appspotmail.com > Tested-by: syzbot+4e2e47f32607d0f72d43@syzkaller.appspotmail.com > Tested-by: syzbot+472626bb5e7c59fb768f@syzkaller.appspotmail.com > cc: Herbert Xu <herbert@gondor.apana.org.au> > cc: "David S. Miller" <davem@davemloft.net> > cc: Eric Dumazet <edumazet@google.com> > cc: Jakub Kicinski <kuba@kernel.org> > cc: Paolo Abeni <pabeni@redhat.com> > cc: Jens Axboe <axboe@kernel.dk> > cc: Matthew Wilcox <willy@infradead.org> > cc: linux-crypto@vger.kernel.org > cc: netdev@vger.kernel.org > --- > crypto/algif_hash.c | 21 +++++---------------- > lib/scatterlist.c | 2 +- > 2 files changed, 6 insertions(+), 17 deletions(-) Acked-by: Herbert Xu <herbert@gondor.apana.org.au> Thanks,
Can you have a look at: https://lore.kernel.org/r/415439.1686877276@warthog.procyon.org.uk/ I'm proposing that as an alternative to this patch. David
On Fri, Jun 16, 2023 at 11:37:58AM +0100, David Howells wrote: > Can you have a look at: > > https://lore.kernel.org/r/415439.1686877276@warthog.procyon.org.uk/ > > I'm proposing that as an alternative to this patch. It'd be easier to comment on it if you sent it by email. Anyway, why did you remove the condition on hash_free_result? We free the result if it's not needed, not to clear the previous hash. So by doing it uncondtionally you will simply end up freeing and reallocating the result for no good reason. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> It'd be easier to comment on it if you sent it by email.
Done. Could you repost your comments against that?
Thanks,
David
Herbert Xu <herbert@gondor.apana.org.au> wrote: > Anyway, why did you remove the condition on hash_free_result? > We free the result if it's not needed, not to clear the previous > hash. So by doing it uncondtionally you will simply end up > freeing and reallocating the result for no good reason. The free here: if (!continuing) { if ((msg->msg_flags & MSG_MORE)) hash_free_result(sk, ctx); only happens in the following case: send(hashfd, "", 0, 0); send(hashfd, "", 0, MSG_MORE); <--- by this and the patch changes how this case works if no data is given. In Linus's tree, it will create a result, init the crypto and finalise it in hash_sendmsg(); with this patch that case is then handled by hash_recvmsg(). If you consider the following sequence: send(hashfd, "", 0, 0); send(hashfd, "", 0, 0); send(hashfd, "", 0, 0); send(hashfd, "", 0, 0); Upstream, the first one will create a result and then each of them will init and finalise a hash, whereas with my patch, the first one will release any outstanding result and then none of them will do any crypto ops. However, as, with my patch hash_sendmsg() no longer calculated a result, it has to clear the result pointer because the logic inside hash_recvmsg() relies on the result pointer to indicate that there is a result. Instead, hash_recvmsg() concocts the result - something it has to be able to do anyway in case someone calls recvmsg() without first supplying data. David
On Mon, Jun 19, 2023 at 05:47:26PM +0100, David Howells wrote: > > The free here: > > if (!continuing) { > if ((msg->msg_flags & MSG_MORE)) > hash_free_result(sk, ctx); > > only happens in the following case: > > send(hashfd, "", 0, 0); > send(hashfd, "", 0, MSG_MORE); <--- by this Yes and that's what I'm complaining about. > and the patch changes how this case works if no data is given. In Linus's > tree, it will create a result, init the crypto and finalise it in > hash_sendmsg(); with this patch that case is then handled by hash_recvmsg(). > If you consider the following sequence: > > send(hashfd, "", 0, 0); > send(hashfd, "", 0, 0); > send(hashfd, "", 0, 0); > send(hashfd, "", 0, 0); > > Upstream, the first one will create a result and then each of them will init > and finalise a hash, whereas with my patch, the first one will release any > outstanding result and then none of them will do any crypto ops. This is correct. If MSG_MORE is not set, then the hash will be finalised. In which case if there is already a result allocated then we should reuse it and not free it. If MSG_MORE is set, then we can delay the allocation of the result, in which case it makes sense to free any previous results since the next request may not come for a very long time (or perhaps even never). Cheers,
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c index dfb048cefb60..1176533a55c9 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -83,26 +83,14 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg, ctx->more = false; - while (msg_data_left(msg)) { + do { ctx->sgl.sgt.sgl = ctx->sgl.sgl; ctx->sgl.sgt.nents = 0; ctx->sgl.sgt.orig_nents = 0; err = -EIO; npages = iov_iter_npages(&msg->msg_iter, max_pages); - if (npages == 0) - goto unlock_free; - - if (npages > ARRAY_SIZE(ctx->sgl.sgl)) { - err = -ENOMEM; - ctx->sgl.sgt.sgl = - kvmalloc(array_size(npages, - sizeof(*ctx->sgl.sgt.sgl)), - GFP_KERNEL); - if (!ctx->sgl.sgt.sgl) - goto unlock_free; - } - sg_init_table(ctx->sgl.sgl, npages); + sg_init_table(ctx->sgl.sgl, max_t(size_t, npages, 1)); ctx->sgl.need_unpin = iov_iter_extract_will_pin(&msg->msg_iter); @@ -111,7 +99,8 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg, if (err < 0) goto unlock_free; len = err; - sg_mark_end(ctx->sgl.sgt.sgl + ctx->sgl.sgt.nents - 1); + if (len > 0) + sg_mark_end(ctx->sgl.sgt.sgl + ctx->sgl.sgt.nents - 1); if (!msg_data_left(msg)) { err = hash_alloc_result(sk, ctx); @@ -148,7 +137,7 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg, copied += len; af_alg_free_sg(&ctx->sgl); - } + } while (msg_data_left(msg)); ctx->more = msg->msg_flags & MSG_MORE; err = 0; diff --git a/lib/scatterlist.c b/lib/scatterlist.c index e97d7060329e..77a7b18ee751 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -1340,7 +1340,7 @@ ssize_t extract_iter_to_sg(struct iov_iter *iter, size_t maxsize, struct sg_table *sgtable, unsigned int sg_max, iov_iter_extraction_t extraction_flags) { - if (maxsize == 0) + if (!maxsize || !iter->count) return 0; switch (iov_iter_type(iter)) {
If an AF_ALG socket bound to a hashing algorithm is sent a zero-length message with MSG_MORE set and then recvmsg() is called without first sending another message without MSG_MORE set to end the operation, an oops will occur because the crypto context and result doesn't now get set up in advance because hash_sendmsg() now defers that as long as possible in the hope that it can use crypto_ahash_digest() - and then because the message is zero-length, it the data wrangling loop is skipped. Fix this by always making a pass of the loop, even in the case that no data is provided to the sendmsg(). Fix also extract_iter_to_sg() to handle a zero-length iterator by returning 0 immediately. Whilst we're at it, remove the code to create a kvmalloc'd scatterlist if we get more than ALG_MAX_PAGES - this shouldn't happen. Fixes: c662b043cdca ("crypto: af_alg/hash: Support MSG_SPLICE_PAGES") Reported-by: syzbot+13a08c0bf4d212766c3c@syzkaller.appspotmail.com Link: https://lore.kernel.org/r/000000000000b928f705fdeb873a@google.com/ Reported-by: syzbot+14234ccf6d0ef629ec1a@syzkaller.appspotmail.com Link: https://lore.kernel.org/r/000000000000c047db05fdeb8790@google.com/ Reported-by: syzbot+4e2e47f32607d0f72d43@syzkaller.appspotmail.com Link: https://lore.kernel.org/r/000000000000bcca3205fdeb87fb@google.com/ Reported-by: syzbot+472626bb5e7c59fb768f@syzkaller.appspotmail.com Link: https://lore.kernel.org/r/000000000000b55d8805fdeb8385@google.com/ Signed-off-by: David Howells <dhowells@redhat.com> Tested-by: syzbot+13a08c0bf4d212766c3c@syzkaller.appspotmail.com Tested-by: syzbot+14234ccf6d0ef629ec1a@syzkaller.appspotmail.com Tested-by: syzbot+4e2e47f32607d0f72d43@syzkaller.appspotmail.com Tested-by: syzbot+472626bb5e7c59fb768f@syzkaller.appspotmail.com cc: Herbert Xu <herbert@gondor.apana.org.au> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: linux-crypto@vger.kernel.org cc: netdev@vger.kernel.org --- crypto/algif_hash.c | 21 +++++---------------- lib/scatterlist.c | 2 +- 2 files changed, 6 insertions(+), 17 deletions(-)