Re: [PATCH] dmaengine: dw-axi-dmac: fix __le32 on set of CH_CTL_H_LLI_VALID

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `status_lo` field of `struct axi_dma_lli` is a little-endian `__le32` value, but it is passed directly without byte-swapping to `axi_chan_irq_clear()`. Furthermore, `status_lo` contains block transfer status, not an interrupt clear mask.
- [High] `axi_chan_block_xfer_complete` uses the global channel-wide allocation counter `chan->descs_allocated` to iterate over the local `desc->hw_desc` array, leading to a slab out-of-bounds access.
- [High] `dw_axi_dma_chan_prep_cyclic` produces incorrect segment lengths if `period_len` is not evenly divisible by `num_segments`, causing cyclic buffer overflows and missed callbacks.
--

commit 7955a4e962eae80371df48e83bbcd1b7445eaf76
Author: Ben Dooks <ben.dooks@codethink.co.uk>

dmaengine: dw-axi-dmac: fix __le32 on set of CH_CTL_H_LLI_VALID

This patch fixes an endianness issue in the dw-axi-dmac driver by using
cpu_to_le32() when setting the CH_CTL_H_LLI_VALID bit in the LLI control
high register.

> diff --git a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> --- a/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> +++ b/drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c
> @@ -1123,7 +1123,7 @@ static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
>  				hw_desc = &desc->hw_desc[i];

[Severity: High]
This is a pre-existing issue, but since we are indexing desc->hw_desc
here, can this loop read out of bounds?

The loop limit count is derived from the global channel-wide allocation:

dw-axi-dmac-platform.c:axi_chan_block_xfer_complete()
    int count = atomic_read(&chan->descs_allocated);

However, the array desc->hw_desc is sized specifically for the current
transaction (desc->nr_hw_descs). If multiple descriptors are prepared on
the same channel, won't descs_allocated be larger than desc->nr_hw_descs
and cause this loop to access memory past the end of the array?

>  				if (hw_desc->llp == llp) {
>  					axi_chan_irq_clear(chan, hw_desc->lli->status_lo);

[Severity: High]
This isn't a bug introduced by this patch, but addressing the question in
the commit message about whether passing lli->status_lo is a bug: yes,
is this writing garbage bits to the interrupt clear register?

The status_lo field is an __le32 value being passed directly to
axi_chan_irq_clear() which expects a CPU-endian u32. More importantly,
status_lo contains the hardware status writeback of the LLI, not an IRQ
bitmask. 

Writing this to CH_INTCLEAR could clear unrelated interrupts, such as
critical error interrupts like DWAXIDMAC_IRQ_ALL_ERR, before they can
be handled.

> -					hw_desc->lli->ctl_hi |= CH_CTL_H_LLI_VALID;
> +					hw_desc->lli->ctl_hi |= cpu_to_le32(CH_CTL_H_LLI_VALID);
>  					desc->completed_blocks = i;
>  
>  					if (((hw_desc->len * (i + 1)) % desc->period_len) == 0)

[Severity: High]
This is a pre-existing issue, but does this modulo condition fail to trigger
callbacks if the period length isn't evenly divisible?

In dw_axi_dma_chan_prep_cyclic(), the segment length is calculated with
DIV_ROUND_UP():

dw-axi-dmac-platform.c:dw_axi_dma_chan_prep_cyclic()
    segment_len = DIV_ROUND_UP(period_len, num_segments);

If period_len is not a clean multiple of num_segments, the unconditionally
padded segment lengths mean the mathematical condition here will never
evaluate to 0, permanently breaking vchan_cyclic_callback() execution.

Additionally, would the padded segment lengths cause an out-of-bounds DMA
memory transfer exceeding the actual buffer length?

>  						vchan_cyclic_callback(vd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617084944.705266-1-ben.dooks@codethink.co.uk?part=1