Re: [PATCH] dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free during descriptor list cleanup on error paths.
- [High] Tasklets are not killed during device removal, leading to a potential Use-After-Free.
- [High] Stale state and missing initialization for `chan->dir` and `chan->dcmd` breaking memory-to-memory DMA.
- [High] Unprotected overwrite of channel-wide `byte_align` flag by subsequent prep calls.
--

commit 67c707d8b7f67369582be4fc75b9289a8abcab3a
Author: Baineng Shou <shoubaineng@gmail.com>

dmaengine: mmp_pdma: fix wrong sg length in mmp_pdma_prep_slave_sg

In mmp_pdma_prep_slave_sg(), for_each_sg() iterates the scatterlist
putting each entry into 'sg', but the entry length is read from 'sgl'
(the list head) instead of 'sg' (the current entry):
[ ... ]
Single-sg or uniformly-sized lists happen to mask the issue.

> diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
> index 386e85cd4882a..e90fd2023af71 100644
> --- a/drivers/dma/mmp_pdma.c
> +++ b/drivers/dma/mmp_pdma.c
> @@ -713,7 +713,7 @@ mmp_pdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
>  
>  	for_each_sg(sgl, sg, sg_len, i) {
>  		addr = sg_dma_address(sg);
> -		avail = sg_dma_len(sgl);
> +		avail = sg_dma_len(sg);
>  
>  		do {
>  			len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);

[Severity: High]
This isn't a regression introduced by this patch, but this lockless
modification of chan->byte_align appears to silently overwrite the hardware
alignment requirement for any earlier descriptors that are still pending or
running:

drivers/dma/mmp_pdma.c:mmp_pdma_prep_slave_sg() {
    ...
		do {
			len = min_t(size_t, avail, PDMA_MAX_DESC_BYTES);
			if (addr & 0x7)
				chan->byte_align = true;
    ...
}

If a client prepares multiple scatterlists on the same channel before
submitting them, does the last prepared scatterlist overwrite
chan->byte_align for the entire channel?

When earlier transactions are eventually started via enable_chan(), they
are submitted to hardware with the overwritten alignment configuration:

drivers/dma/mmp_pdma.c:enable_chan() {
    ...
	if (phy->vchan->byte_align)
		dalgn |= 1 << phy->idx;
    ...
}

Could this lead to hardware misbehavior or data corruption due to incorrect
alignment configuration?


[Severity: High]
This is a pre-existing regression, but I have a question about the error
handling path further down in mmp_pdma_prep_slave_sg():

drivers/dma/mmp_pdma.c:mmp_pdma_prep_slave_sg() {
    ...
fail:
	if (first)
		mmp_pdma_free_desc_list(chan, &first->tx_list);
	return NULL;
}

The cleanup sequence explicitly passes &first->tx_list, which is embedded
in the descriptor to be freed.

drivers/dma/mmp_pdma.c:mmp_pdma_free_desc_list() {
    ...
	list_for_each_entry_safe(desc, _desc, list, node) {
		list_del(&desc->node);
		dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
	}
    ...
}

The list iteration macro safely caches the next element, but it reads from
and writes to the list head during the update and list_del() steps.

If the list head (&first->tx_list) is embedded in a freed element (because
the 'first' descriptor is freed during the first iteration), wouldn't this
result in a use-after-free when subsequent list manipulations or the loop's
next-entry calculation dereference the freed memory?


[Severity: High]
This is a pre-existing regression, but there might be an initialization
problem in mmp_pdma_prep_memcpy() when reusing channels:

drivers/dma/mmp_pdma.c:mmp_pdma_prep_memcpy() {
    ...
	if (!chan->dir) {
		chan->dir = DMA_MEM_TO_MEM;
		chan->dcmd = DCMD_INCTRGADDR | DCMD_INCSRCADDR;
		chan->dcmd |= DCMD_BURST32;
	}
    ...
}

Does the !chan->dir condition fail to clear stale state from a previous slave
DMA session?

When a channel allocated from the shared pool is used for slave DMA (e.g.,
DMA_MEM_TO_DEV), it modifies chan->dir and chan->dcmd. When the channel is
freed via mmp_pdma_free_chan_resources(), these fields are not cleared.

If the channel is later requested for dma_memcpy, mmp_pdma_prep_memcpy() will
use the stale state. Could this incorrectly omit DCMD_INCTRGADDR (leaving the
target address static) and apply the wrong address increment logic, resulting
in corrupted memory writes?


[Severity: High]
This is a pre-existing regression, but I noticed a potential teardown
problem in the device removal path:

drivers/dma/mmp_pdma.c:mmp_pdma_remove() {
    ...
	dma_async_device_unregister(&pdev->device);
}

The device removal teardown is missing necessary tasklet_kill() calls for
all initialized channels before their memory backing is freed by devres.

Since chan is managed by devm_kzalloc(), it is automatically freed after
mmp_pdma_remove() completes.

If a tasklet was scheduled right before free_irq() but hasn't executed,
will it run after removal completes and access the freed chan structure,
causing a use-after-free?

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