Message ID | 1679829.1686785273@warthog.procyon.org.uk |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp289488vqr; Wed, 14 Jun 2023 16:35:26 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6FMCew73pMsdR9DAzVeBpWXvWIK/UOB/CsKZWvhRfMLauYIljTGaGTT3zGxV9cWE3Yo7g5 X-Received: by 2002:a17:907:1c97:b0:973:ff4d:d01e with SMTP id nb23-20020a1709071c9700b00973ff4dd01emr18129157ejc.31.1686785726338; Wed, 14 Jun 2023 16:35:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686785726; cv=none; d=google.com; s=arc-20160816; b=J2T3RfDze82yO1x/oFap6QDzXDMGpYtMZh9S3kOm7U6yzPPW7flpt5jTwXKH4fOmeF 7NSWzIB+PBVDtz5RW5XbRiPGvTpevBW4uF5Yo3gHNnK9dwKMxquIItAJfK8FCUc5rkAf SyKf5/g3Seu5RKYRU0oksZDfRoTVItWDeoqutsZWu2rY6HEAHoESTt/PeNcOXSYLH5JW d1sTsMfLBX1xoFuGYBkzGVbNUgKcUHggcbGC3sH9K861Do7z2jSeHeg0b4r15ymLwrEw dDkak1oLzcd21T7R6ddVRrXCai++A4NB5Yl195RPe8oW2xEx2GvmKzr9Ufd/5gS85VZV EAIw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:from:content-transfer-encoding :content-id:mime-version:subject:cc:to:organization:dkim-signature; bh=aH8zwLa8Ht2nXaE8fo3wqv7zLHm1tw4+70R/cDKjh3s=; b=YIa3qfYa65yEMU1h52kc7JXbPPM6OMFRIr5YhYhIdcT3M5jKHRY8M8EEcaRgB0ghGu z1OGqmUqJjmFoqEuQ5lEUn9+tt2ciXBw26RRSXtpdsjhFzlRXMkjDYAHpdqa1uKHC8fY 2Ez0k2XkF5fI3oFxCa1k7AMOpARJGxqOozxw3yW7ZdlQRZNCYiHGi6hrUYKiKJA4i8DE Td1q6DLzNvup0RnJbEa1yHd+AltfzaG3TMS2MfV3qDfF0a5YxAMoq0N47kpXbLNBVUfg 62EF8HMmnUnGA1f8iBfNGU9q9gAtMkD9f0w1LXsRB5iMW66V8fT042Z0NhtzALF2OAsy wd1A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Pib7553m; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n4-20020a17090625c400b00974e7637ea3si8053130ejb.702.2023.06.14.16.35.02; Wed, 14 Jun 2023 16:35:26 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Pib7553m; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235533AbjFNX2x (ORCPT <rfc822;n2h9z4@gmail.com> + 99 others); Wed, 14 Jun 2023 19:28:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59840 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229516AbjFNX2v (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 14 Jun 2023 19:28:51 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AB1571FD7 for <linux-kernel@vger.kernel.org>; Wed, 14 Jun 2023 16:28:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686785280; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aH8zwLa8Ht2nXaE8fo3wqv7zLHm1tw4+70R/cDKjh3s=; b=Pib7553m97gLDb/s0E3V+7jCoW58jZS1t2ygyRC3W7QMAac1p0Sq5dX/2Dl2zYWSUouRHl y6nssujR7lIzBoi4HSx69+yp+7uUIAEugKCZzc1pZlqRo0ix07AtEinXcrul7CHlH7RP4N LsA5s3QmdWxiYniv6S3TZ9b4qMWeA1s= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-85-n6p0z6GaO3ei54QFL73rZQ-1; Wed, 14 Jun 2023 19:27:56 -0400 X-MC-Unique: n6p0z6GaO3ei54QFL73rZQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1504A296A603; Wed, 14 Jun 2023 23:27:56 +0000 (UTC) Received: from warthog.procyon.org.uk (unknown [10.42.28.67]) by smtp.corp.redhat.com (Postfix) with ESMTP id 559E240C6F5E; Wed, 14 Jun 2023 23:27:54 +0000 (UTC) Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 To: netdev@vger.kernel.org cc: dhowells@redhat.com, syzbot+13a08c0bf4d212766c3c@syzkaller.appspotmail.com, syzbot+14234ccf6d0ef629ec1a@syzkaller.appspotmail.com, syzbot+4e2e47f32607d0f72d43@syzkaller.appspotmail.com, syzbot+472626bb5e7c59fb768f@syzkaller.appspotmail.com, Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Jens Axboe <axboe@kernel.dk>, Matthew Wilcox <willy@infradead.org>, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH net-next] crypto: af_alg/hash: Fix recvmsg() after sendmsg(MSG_MORE) MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <1679811.1686785244.1@warthog.procyon.org.uk> Content-Transfer-Encoding: quoted-printable From: David Howells <dhowells@redhat.com> Date: Thu, 15 Jun 2023 00:27:53 +0100 Message-ID: <1679829.1686785273@warthog.procyon.org.uk> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768723029709760524?= X-GMAIL-MSGID: =?utf-8?q?1768723029709760524?= |
Series |
[net-next] crypto: af_alg/hash: Fix recvmsg() after sendmsg(MSG_MORE)
|
|
Commit Message
David Howells
June 14, 2023, 11:27 p.m. UTC
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(-)
Comments
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)) {