Re: [PATCH] dmaengine: pxa: fix double counting of the hw descriptors

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

New issues:
- [Critical] Removing the manual `sw_desc->nb_desc++` increment breaks the driver on compilers without `__builtin_counted_by_ref` support, permanently leaving `nb_desc` at 0 on success and causing out-of-bounds memory corruption.

Pre-existing issues:
- [High] In cyclic DMA mode, the updater descriptor is permanently leaked and its DMA address is mismatched, causing a `dma_pool_free` error and a potential double-free of the first descriptor's DMA address.
- [High] The driver's teardown sequence leaks DMA channels and creates a use-after-free window in the IRQ handler due to conflicting devm registration and manual cleanup.
--

--- Patch [1]: [PATCH] dmaengine: pxa: fix double counting of the hw descriptors ---
commit 31bd2b3ddeaa75fd6d698e4a2da9964bd5fc7adc
Author: Sascha Hauer <[email protected]>

dmaengine: pxa: fix double counting of the hw descriptors

This commit drops a manual increment of nb_desc in pxad_alloc_desc(),
relying on kzalloc_flex() to initialize the counted_by variable. It also
updates the error path to ensure nb_desc accurately reflects allocated
descriptors.

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..8252d27be8c3c 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -752,10 +752,11 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  			dev_err(&chan->vc.chan.dev->device,
>  				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
>  				__func__, i, sw_desc->desc_pool);
> +			/* Only the descriptors below i have been allocated */
> +			sw_desc->nb_desc = i;
>  			goto err;
>  		}
>  
> -		sw_desc->nb_desc++;

[Severity: Critical]
Does removing this manual increment break the driver on compilers without
__builtin_counted_by_ref() support (such as GCC < 15 or Clang < 19)?

It appears that without compiler support, the kzalloc_flex() macro acts as
a no-op for initializing the counter, which would leave sw_desc->nb_desc as 0
on a successful allocation.

If nb_desc remains 0, could this cause out-of-bounds memory corruption later
in set_updater_desc()?

drivers/dma/pxa_dma.c:set_updater_desc() {
	struct pxad_desc_hw *updater =
		sw_desc->hw_desc[sw_desc->nb_desc - 1];
...

If nb_desc is 0, this evaluates to hw_desc[-1], which would overlay and
corrupt the adjacent desc_pool pointer in the pxad_desc_sw struct.


[Severity: High]
This isn't a bug introduced by this patch, but does the current driver
permanently leak the updater descriptor in cyclic DMA mode?

During cyclic setup, set_updater_desc() overwrites the ddadr field of the
second-to-last descriptor:

drivers/dma/pxa_dma.c:set_updater_desc() {
	...
	if (sw_desc->cyclic)
		sw_desc->hw_desc[sw_desc->nb_desc - 2]->ddadr = sw_desc->first;
	...
}

Later, pxad_free_desc() relies on that overwritten ddadr to find the physical
address for freeing the updater:

drivers/dma/pxa_dma.c:pxad_free_desc() {
	...
	dma = sw_desc->hw_desc[i - 1]->ddadr;
	else
		dma = sw_desc->first;
	dma_pool_free(sw_desc->desc_pool,
		      sw_desc->hw_desc[i], dma);
	...
}

Could this pass the wrong physical address (sw_desc->first) to
dma_pool_free(), failing to free the updater descriptor and potentially
causing a double-free of the first descriptor's address?


[Severity: High]
This is a pre-existing issue, but does the driver's teardown sequence leak
DMA channels and create a use-after-free window in the IRQ handler?

In pxad_probe(), devres allocates the IRQ before the pxad_chan structures:

drivers/dma/pxa_dma.c:pxad_init_phys() {
	...
	ret = devm_request_irq(&op->dev, irq,
			       pxad_chan_handler,
			       IRQF_SHARED, "pxa-dma", phy);
	...
}

On teardown, pxad_remove() manually empties the channel list before the
devm-managed DMA engine unregistration occurs:

drivers/dma/pxa_dma.c:pxad_free_channels() {
	...
	list_del(&c->vc.chan.device_node);
	tasklet_kill(&c->vc.task);
	...
}

When devres later executes dma_async_device_unregister(), does the empty
list cause it to skip unregistering the individual channels?

If the channels are skipped, devres frees the pxad_chan memory while the
IRQ remains registered. Could an interrupt firing in this window cause
pxad_chan_handler() to dereference the freed chan pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1