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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Zero-length transfers passed to `dma_chan_prep_dma_memcpy` result in negative array indexing and an underflowing infinite loop, leading to severe out-of-bounds access and arbitrary kernel memory corruption.
- [High] Lack of zero-length validation in dw_axi_dma_chan_prep_slave_sg and dw_axi_dma_chan_prep_cyclic can cause divide-by-zero panics.
- [High] The error path for hardware descriptor allocation blindly frees the entire array of requested descriptors without checking for NULL, causing a guaranteed NULL pointer dereference.
- [High] Silent data truncation and subsequent NULL pointer dereference in scatterlist transfers if lengths are not exact multiples of the computed segment size.
- [High] The driver fails to start the next queued descriptor when a non-cyclic transfer completes, leading to stalled DMA channels.
- [Medium] dma_chan_issue_pending unconditionally attempts to start the channel even if it is already running, resulting in spurious 'non-idle' errors.
--

commit 212e0a527db7f8f42167aa10e801996bc3ac1256
Author: Jia Wang <wangjia@ultrarisc.com>
Subject: dmaengine: dw-axi-dmac: Fix AXI burst length encoding

This commit fixes AXI decode errors caused by unencoded burst lengths
overflowing hardware fields. It subtracts one from the burst length value
when configuring the hardware descriptors while preserving the actual value
for reporting.

> 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

[ ... ]

> @@ -975,7 +975,7 @@ dma_chan_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst_adr,
>  
>  		reg = CH_CTL_H_LLI_VALID;
>  		if (chan->chip->dw->hdata->restrict_axi_burst_len) {
> -			u32 burst_len = chan->chip->dw->hdata->axi_rw_burst_len;
> +			u32 burst_len = chan->chip->dw->hdata->axi_rw_burst_len - 1;
>  
>  			reg |= (CH_CTL_H_ARLEN_EN |
>  				burst_len << CH_CTL_H_ARLEN_POS |

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem 
in this function for zero-length transfers.

If dma_chan_prep_dma_memcpy() is called with len == 0, num starts at 0 and
the while (len) loop is skipped. Does this lead to an out-of-bounds
access here:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_prep_dma_memcpy() {
...
    set_desc_last(&desc->hw_desc[num - 1]);
...
}

And subsequently, could this cause an underflowing loop that writes to
arbitrary memory?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_prep_dma_memcpy() {
...
    do {
        hw_desc = &desc->hw_desc[--num];
        write_desc_llp(hw_desc, llp | lms);
        llp = hw_desc->llp;
    } while (num);
...
}

[Severity: High]
This is a pre-existing issue, but the error path in dma_chan_prep_dma_memcpy()
seems like it might dereference a NULL pointer.

If axi_desc_get() fails and the code jumps to err_desc_get, we call
axi_desc_put(desc):

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_prep_dma_memcpy() {
...
err_desc_get:
    if (desc)
        axi_desc_put(desc);
...
}

Since the uninitialized elements of desc->hw_desc will have a NULL lli
pointer, does axi_desc_put() crash when it unconditionally calls
dma_pool_free() on the entire array?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_desc_put() {
...
    dma_pool_free(chan->desc_pool, hw_desc->lli, hw_desc->llp);
...
}

[Severity: High]
This is a pre-existing issue, but I noticed a potential division by zero
elsewhere in this file.

If a caller passes a period_len of 0 to dw_axi_dma_chan_prep_cyclic(),
could this cause a panic?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_cyclic() {
...
    num_periods = buf_len / period_len;
...
}

Similarly, in dw_axi_dma_chan_prep_slave_sg(), if any scatterlist element
has a length of 0, num_segments evaluates to 0. Does this macro then cause
a divide-by-zero panic?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_slave_sg() {
...
    segment_len = DIV_ROUND_UP(sg_dma_len(sg), num_segments);
...
}

[Severity: High]
This is a pre-existing issue, but does dw_axi_dma_chan_prep_slave_sg()
truncate data if the transfer length is not an exact multiple of the
computed segment size?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_slave_sg() {
...
        segment_len = DIV_ROUND_UP(sg_dma_len(sg), num_segments);
...
    } while (len >= segment_len);
...
}

If the loop exits early, the remaining bytes seem to be discarded. Further
down in the function, it loops over all allocated segments:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_slave_sg() {
...
    do {
        hw_desc = &desc->hw_desc[--num_sgs];
        write_desc_llp(hw_desc, llp | lms);
...
}

Since the earlier loop exited prematurely, the trailing elements in hw_desc
have a NULL lli pointer. Does calling write_desc_llp() on them cause a panic?

[Severity: High]
This is a pre-existing issue, but I have a question about how hardware
queueing is managed. In axi_chan_block_xfer_complete(), after completing a
transfer:

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_block_xfer_complete() {
...
    list_del(&vd->node);
    vchan_cookie_complete(vd);
}
out:
...
}

Should this function call axi_chan_start_first_queued() or check
vchan_next_desc to start the next pending transfer? Without it, does a
queued subsequent transfer sit indefinitely without being processed?

[Severity: Medium]
This is a pre-existing issue, but does dma_chan_issue_pending()
unconditionally attempt to start the channel even if it is already actively
processing a transfer?

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:dma_chan_issue_pending() {
...
    if (vchan_issue_pending(&chan->vc))
        axi_chan_start_first_queued(chan);
...
}

If the hardware is already active, this leads to a spurious error printed
in axi_chan_block_xfer_start():

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:axi_chan_block_xfer_start() {
...
    if (unlikely(axi_chan_is_hw_enable(chan))) {
        dev_err(chan2dev(chan), "%s is non-idle!\n",
...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-dma-fix-v3-0-414c6449fbcc@ultrarisc.com?part=1