From: Kuniyuki Iwashima <kuniyu@amazon.com>
To: <herbert@gondor.apana.org.au>
CC: <davem@davemloft.net>, <kuni1840@gmail.com>, <kuniyu@amazon.com>,
	<linux-crypto@vger.kernel.org>, <syzkaller@googlegroups.com>
Subject: Re: [v2 PATCH] crypto: scomp - Fix wild memory accesses in scomp_free_streams
Date: Thu, 10 Apr 2025 19:43:18 -0700
Message-ID: <20250411024346.64403-1-kuniyu@amazon.com>
In-Reply-To: <Z_hv117exy6sjUI7@gondor.apana.org.au>
References: <Z_hv117exy6sjUI7@gondor.apana.org.au>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain

From: Herbert Xu <herbert@gondor.apana.org.au>
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@googlegroups.com>
> Co-developed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Co-developed-by: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> 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;
>  }
>  
> --