Re: [PATCH v2] dmaengine: xilinx: dpdma: fix crash on zero-length cyclic transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dmaengine: xilinx: dpdma: misplaced and incomplete zero-length check
--

commit 9ce2097539c963154302835e8e5bdaba12caffe7
Author: Wang Haoran <haoranwangsec@gmail.com>

dmaengine: xilinx: dpdma: fix crash on zero-length cyclic transfer

This commit checks whether the descriptor list is empty after the loop in
xilinx_dpdma_chan_prep_cyclic() to avoid a crash when buf_len is zero.

> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c
> index d9a3542c4531c..bdfa85b80c6a8 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -730,6 +730,9 @@ xilinx_dpdma_chan_prep_cyclic(struct xilinx_dpdma_chan *chan,
>  		last = sw_desc;
>  	}
>  
> +	if (list_empty(&tx_desc->descriptors))
> +		goto error;

[Severity: Medium]
In xilinx_dpdma_chan_prep_cyclic(), every iteration of the loop either
jumps to error or appends a descriptor via list_add_tail():

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_chan_prep_cyclic() {
	unsigned int periods = buf_len / period_len;
	unsigned int i;

	tx_desc = xilinx_dpdma_chan_alloc_tx_desc(chan);
	if (!tx_desc)
		return NULL;

	for (i = 0; i < periods; i++) {
		...
		list_add_tail(&sw_desc->node, &tx_desc->descriptors);

		buf_addr += period_len;
		last = sw_desc;
	}
}

Because of this, list_empty(&tx_desc->descriptors) can only be true when
periods is 0, so checking after the loop allocates tx_desc with
xilinx_dpdma_chan_alloc_tx_desc() only to immediately free it under the
error label.

Also, in the caller xilinx_dpdma_prep_dma_cyclic(), buf_len % period_len
is evaluated without checking whether period_len is zero:

drivers/dma/xilinx/xilinx_dpdma.c:xilinx_dpdma_prep_dma_cyclic() {
	if (direction != DMA_MEM_TO_DEV)
		return NULL;

	if (buf_len % period_len)
		return NULL;

	return xilinx_dpdma_chan_prep_cyclic(chan, buf_addr, buf_len,
					     period_len, flags);
}

If period_len is 0 (for example, when both buf_len and period_len are 0),
won't this still trigger a divide-by-zero crash before reaching
list_empty()?

Would it be cleaner to validate if (!buf_len || !period_len) upfront in
xilinx_dpdma_prep_dma_cyclic(), matching how
xilinx_dpdma_prep_interleaved_dma() checks if (!xt->numf || !xt->sgl[0].size)
before allocating a descriptor?

Additionally, the commit message states:

  The following list_first_entry() on the empty list returns a bad pointer
  and causes a kernel oops.

When the list is empty, list_first_entry() computes an offset from
&tx_desc->descriptors and reads sw_desc->dma_addr relative to tx_desc,
whereas the NULL pointer dereference happens on the next statement
(last->hw.next_desc = ...) because last is initialized to NULL and never
updated when periods is 0. Could the commit message be updated to reflect
this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103083132.1122990-1-lixicaicai@gmail.com?part=1