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: [email protected]
Link: https://lore.kernel.org/r/[email protected]/
Reported-by: [email protected]
Link: https://lore.kernel.org/r/[email protected]/
Reported-by: [email protected]
Link: https://lore.kernel.org/r/[email protected]/
Reported-by: [email protected]
Link: https://lore.kernel.org/r/[email protected]/
Signed-off-by: David Howells <[email protected]>
Tested-by: [email protected]
Tested-by: [email protected]
Tested-by: [email protected]
Tested-by: [email protected]
cc: Herbert Xu <[email protected]>
cc: "David S. Miller" <[email protected]>
cc: Eric Dumazet <[email protected]>
cc: Jakub Kicinski <[email protected]>
cc: Paolo Abeni <[email protected]>
cc: Jens Axboe <[email protected]>
cc: Matthew Wilcox <[email protected]>
cc: [email protected]
cc: [email protected]
---
crypto/algif_hash.c | 21 +++++----------------
lib/scatterlist.c | 2 +-
2 files changed, 6 insertions(+), 17 deletions(-)
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)) {
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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu <[email protected]> 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: [email protected]
> Link: https://lore.kernel.org/r/[email protected]/
> Reported-by: [email protected]
> Link: https://lore.kernel.org/r/[email protected]/
> Reported-by: [email protected]
> Link: https://lore.kernel.org/r/[email protected]/
> Reported-by: [email protected]
> Link: https://lore.kernel.org/r/[email protected]/
> Signed-off-by: David Howells <[email protected]>
> Tested-by: [email protected]
> Tested-by: [email protected]
> Tested-by: [email protected]
> Tested-by: [email protected]
> cc: Herbert Xu <[email protected]>
> cc: "David S. Miller" <[email protected]>
> cc: Eric Dumazet <[email protected]>
> cc: Jakub Kicinski <[email protected]>
> cc: Paolo Abeni <[email protected]>
> cc: Jens Axboe <[email protected]>
> cc: Matthew Wilcox <[email protected]>
> cc: [email protected]
> cc: [email protected]
> ---
> crypto/algif_hash.c | 21 +++++----------------
> lib/scatterlist.c | 2 +-
> 2 files changed, 6 insertions(+), 17 deletions(-)
Acked-by: Herbert Xu <[email protected]>
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Can you have a look at:
https://lore.kernel.org/r/[email protected]/
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/[email protected]/
>
> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Herbert Xu <[email protected]> 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 <[email protected]> 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,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt