From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Date: Fri, 11 Apr 2025 09:26:47 +0800
> On Thu, Apr 10, 2025 at 11:33:45AM -0700, Kuniyuki Iwashima wrote:
> > syzkaller reported the splat below [0].
> >
> > When alg->alloc_ctx() fails, alg is passed to scomp_free_streams(),
> > but alg->stream is still NULL there.
> >
> > Also, ps->ctx has ERR_PTR(), which would bypass the NULL check and
> > could be passed to alg->free_ctx(ps->ctx).
>
> Thanks for the report. I think we should instead move the assignment
> up and test for IS_ERR_OR_NULL in scomp_free_streams.
I didn't move the assignment just because I was not sure if the
immature alg->stream could be accessed by another thread.
But if it's okay, v2 looks better to me.
Thanks!
>
> ---8<---
> In order to use scomp_free_streams to free the partially allocted
> streams in the allocation error path, move the alg->stream assignment
> to the beginning. Also check for error pointers in scomp_free_streams
> before freeing the ctx.
>
> Finally set alg->stream to NULL to not break subsequent attempts
> to allocate the streams.
>
> Fixes: 3d72ad46a23a ("crypto: acomp - Move stream management into scomp layer")
> Reported-by: syzkaller <syzkaller@xxxxxxxxxxxxxxxx>
> Co-developed-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> Co-developed-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> diff --git a/crypto/scompress.c b/crypto/scompress.c
> index f67ce38d203d..5762fcc63b51 100644
> --- a/crypto/scompress.c
> +++ b/crypto/scompress.c
> @@ -111,13 +111,14 @@ static void scomp_free_streams(struct scomp_alg *alg)
> struct crypto_acomp_stream __percpu *stream = alg->stream;
> int i;
>
> + alg->stream = NULL;
> if (!stream)
> return;
>
> for_each_possible_cpu(i) {
> struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);
>
> - if (!ps->ctx)
> + if (IS_ERR_OR_NULL(ps->ctx))
> break;
>
> alg->free_ctx(ps->ctx);
> @@ -135,6 +136,8 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
> if (!stream)
> return -ENOMEM;
>
> + alg->stream = stream;
> +
> for_each_possible_cpu(i) {
> struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);
>
> @@ -146,8 +149,6 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
>
> spin_lock_init(&ps->lock);
> }
> -
> - alg->stream = stream;
> return 0;
> }
>
> --
