Re: [PATCH] crypto: ixp4xx: Fix null-pointer dereference in chainup_buffers()

[Herbert Xu <herbert@gondor.apana.org.au>]

On Tue, Apr 21, 2026 at 05:39:17PM +0800, Ruoyu Wang wrote:
> chainup_buffers() builds a linked list of buffer descriptors for a
> scatterlist. If dma_pool_alloc() fails while constructing the list, the
> current code sets buf to NULL and later dereferences it unconditionally
> at the end of the function:
> 
>   buf->next = NULL;
>   buf->phys_next = 0;
> 
> This can lead to a null-pointer dereference on allocation failure.
> 
> If the failure happens after part of the descriptor chain has already
> been allocated and DMA-mapped, the partially constructed chain also
> needs to be released.
> 
> Fix this by unwinding the partially constructed chain, resetting the
> caller-provided hook descriptor, and returning NULL on allocation
> failure.
> 
> Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
> ---
>  drivers/crypto/intel/ixp4xx/ixp4xx_crypto.c | 24 +++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/crypto/intel/ixp4xx/ixp4xx_crypto.c b/drivers/crypto/intel/ixp4xx/ixp4xx_crypto.c
> index fcc0cf4df637..63ef28cd5766 100644
> --- a/drivers/crypto/intel/ixp4xx/ixp4xx_crypto.c
> +++ b/drivers/crypto/intel/ixp4xx/ixp4xx_crypto.c
> @@ -874,6 +874,11 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
>  		struct buffer_desc *buf, gfp_t flags,
>  		enum dma_data_direction dir)
>  {
> +	struct buffer_desc *first = buf;
> +
> +	first->next = NULL;
> +	first->phys_next = 0;
> +
>  	for (; nbytes > 0; sg = sg_next(sg)) {
>  		unsigned int len = min(nbytes, sg->length);
>  		struct buffer_desc *next_buf;
> @@ -883,10 +888,15 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
>  		nbytes -= len;
>  		ptr = sg_virt(sg);
>  		next_buf = dma_pool_alloc(buffer_pool, flags, &next_buf_phys);
> -		if (!next_buf) {
> -			buf = NULL;
> -			break;
> -		}
> +		if (!next_buf)
> +			goto err_unwind;
> +
> +		/*
> +		 * Keep the chain well-formed even on partial construction,
> +		 * so free_buf_chain() can safely unwind it on failure.
> +		 */
> +		next_buf->next = NULL;
> +		next_buf->phys_next = 0;
>  		sg_dma_address(sg) = dma_map_single(dev, ptr, len, dir);
>  		buf->next = next_buf;
>  		buf->phys_next = next_buf_phys;
> @@ -899,6 +909,12 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
>  	buf->next = NULL;
>  	buf->phys_next = 0;
>  	return buf;
> +
> +err_unwind:
> +	free_buf_chain(dev, first->next, first->phys_next);
> +	first->next = NULL;
> +	first->phys_next = 0;
> +	return NULL;

All callers of chainup_buffers try to unwind by calling free_buf_chain
too, although a couple of them might do so incorrectly.

It looks like the callers need the unwind path anyway, so perhaps
just fix up the callers so that their unwind paths actually work?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt