Re: [PATCH v2 1/4] dmaengine: dw-axi-dmac: Fix AXI burst length encoding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing checks for zero-length inputs in descriptor prep functions lead to out-of-bounds array accesses against ZERO_SIZE_PTR and divide-by-zero panics.
- [High] axi_desc_put() unconditionally calls dma_pool_free() with potentially NULL pointers, causing a kernel panic when tearing down partially allocated descriptor lists.
- [High] axi_desc_put() unconditionally subtracts the maximum expected descriptor count from chan->descs_allocated, causing an atomic counter underflow on error paths.
--

commit 645dfa80d0619fd42c37ace18ec6558aa67250f3
Author: Jia Wang <wangjia@ultrarisc.com>

    dmaengine: dw-axi-dmac: Fix AXI burst length encoding
    
    The snps,axi-max-burst-len property describes the number of beats in an
    AXI burst, while the ARLEN and AWLEN fields encode that value minus one.
    
    The driver keeps axi_rw_burst_len as the actual burst length so that
    dma_device.max_burst reports the correct value. However, it also programs
    that unencoded value directly into the hardware fields. A value of 256
    therefore overflows the 8-bit fields and can cause AXI decode errors.
    
    Subtract one only when constructing hardware descriptors, while keeping
    the actual value for dma_device.max_burst.
    
    Fixes: c454d16a7d5a ("dmaengine: dw-axi-dmac: Burst length settings")
    Signed-off-by: Jia Wang <wangjia@ultrarisc.com>
    Reviewed-by: Frank Li <Frank.Li@nxp.com>

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> index eebed2474210d..742e08cfab435 100644
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -706,7 +706,7 @@ static int dw_axi_dma_set_hw_desc(struct axi_dma_chan *chan,
>  	ctlhi = CH_CTL_H_LLI_VALID;
>  
>  	if (chan->chip->dw->hdata->restrict_axi_burst_len) {
> -		burst_len = chan->chip->dw->hdata->axi_rw_burst_len;
> +		burst_len = chan->chip->dw->hdata->axi_rw_burst_len - 1;
>  		ctlhi |= CH_CTL_H_ARLEN_EN | CH_CTL_H_AWLEN_EN |
>  			 burst_len << CH_CTL_H_ARLEN_POS |
>  			 burst_len << CH_CTL_H_AWLEN_POS;

[ ... ]

I have a few questions about how zero-length inputs and error handling
are managed in this file.

In drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_prep_dma_memcpy():

	while (len) {
		...
		num++;
	}

	/* Set end-of-link to the last link descriptor of list */
	set_desc_last(&desc->hw_desc[num - 1]);

[Severity: High]
This isn't a bug introduced by this patch, but does this cause an
out-of-bounds array access if the DMA client requests a zero-length transfer?

If len is 0, the while loop never executes, num remains 0, and we access
desc->hw_desc[-1], leading to an immediate out-of-bounds dereference.


Also in drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_cyclic():

	num_periods = buf_len / period_len;
	...
	total_segments = num_periods * num_segments;
	...
	llp = desc->hw_desc[0].llp;

	/* Managed transfer list */
	do {
		hw_desc = &desc->hw_desc[--total_segments];
		write_desc_llp(hw_desc, llp | lms);
		llp = hw_desc->llp;
	} while (total_segments);

[Severity: High]
This is a pre-existing issue, but does this cause a divide-by-zero panic if
period_len is 0?

[Severity: High]
This is a pre-existing issue, but does this code also go out of bounds if
buf_len is 0?

If buf_len is 0, total_segments is 0. Accessing desc->hw_desc[0].llp goes
out of bounds, and --total_segments underflows, causing another out-of-bounds
access against the descriptor array.


I also have a question about the error handling path.

In drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_desc_put():

	for (descs_put = 0; descs_put < count; descs_put++) {
		hw_desc = &desc->hw_desc[descs_put];
		dma_pool_free(chan->desc_pool, hw_desc->lli, hw_desc->llp);
	}

	kfree(desc->hw_desc);
	kfree(desc);
	atomic_sub(descs_put, &chan->descs_allocated);

[Severity: High]
This isn't a bug introduced by this patch, but can dma_pool_free() panic if
axi_desc_get() fails during partial allocation?

If axi_desc_get() fails under memory pressure, hw_desc->lli could be NULL
for the descriptors that failed allocation. dma_pool_free() appears to
unconditionally dereference this pointer.

[Severity: High]
This is a pre-existing issue, but does this code also underflow
chan->descs_allocated?

Because descs_put iterates up to the total requested descriptors (count),
atomic_sub() subtracts the total requested count, not just the successfully
allocated ones. If chan->descs_allocated underflows, could this break the
cyclic DMA interrupt handling logic by failing the loop bound check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-dma-fix-v2-0-d7f0459ebb14@ultrarisc.com?part=1