Re: [PATCH] dmaengine: ppc4xx: use devm_platform_ioremap_resource()

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

Pre-existing issues:
- [High] Calling `dma_free_coherent()` with a NULL `adev->dev` pointer in the error path if `devm_platform_ioremap_resource()` fails.
- [High] Missing tasklet_kill on the probe error path leads to Use-After-Free of the chan structure.
- [High] Missing free_irq() for the error IRQ if subsequent allocations fail during setup.
- [High] Missing check for dma_map_page() failure.
--

commit ed3114dad8f9fbd2d3d5f92d786754a5f161c4f0
Author: Rosen Penev [off-list ref]
Subject: dmaengine: ppc4xx: use devm_platform_ioremap_resource()

This patch replaces the open-coded sequence of of_address_to_resource(),
request_mem_region(), and ioremap() with devm_platform_ioremap_resource().
It also removes unused error codes and simplifies error paths since the
devm framework handles automatic release.

[ ... ]

[Severity: High]
This is a pre-existing issue, but does jumping to err_regs_alloc here cause a
NULL pointer dereference?

adev is allocated earlier with kzalloc_obj(), which zero-initializes the memory
and leaves adev->dev as NULL. adev->dev is only assigned after the register
mapping succeeds.

If devm_platform_ioremap_resource() fails, execution jumps to the
err_regs_alloc label:

err_regs_alloc:
	dma_free_coherent(adev->dev, adev->pool_size,
			  adev->dma_desc_pool_virt,
			  adev->dma_desc_pool);

Could passing this NULL adev->dev pointer to dma_free_coherent() cause a kernel
panic during device probe failure?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a tasklet
to run after the chan structure is freed here?

During probe, the driver initializes a tasklet and successfully registers the
hardware IRQ handler via ppc440spe_adma_setup_irqs(). From this point, an
interrupt can fire and schedule the tasklet asynchronously.

If the subsequent dma_async_device_register() fails, the code jumps to the
err_dev_reg error path, which calls ppc440spe_adma_release_irqs(). This frees
the IRQ, but it doesn't appear to call tasklet_kill(&chan->irq_tasklet) to
synchronize with or cancel any pending or running tasklets.

When execution falls through to err_page_alloc, chan is freed via kfree(chan).
Will a running or pending tasklet then access the freed chan memory, leading to
a use-after-free?

-err_chan_alloc:
-	if (adev->id == PPC440SPE_XOR_ID)
-		iounmap(adev->xor_reg);
-	else
-		iounmap(adev->dma_reg);
 err_regs_alloc:
 	dma_free_coherent(adev->dev, adev->pool_size,
 			  adev->dma_desc_pool_virt,
 			  adev->dma_desc_pool);

[ ... ]

In addition to the changes in the diff, a few pre-existing issues were noticed
in this file during review:

[Severity: High]
This is a pre-existing issue, but should the return value of dma_map_page() be
checked for errors?

In ppc440spe_adma_probe():

	chan->pdest = dma_map_page(&ofdev->dev, chan->pdest_page, 0,
				   PAGE_SIZE, DMA_BIDIRECTIONAL);
	chan->qdest = dma_map_page(&ofdev->dev, chan->qdest_page, 0,
				   PAGE_SIZE, DMA_BIDIRECTIONAL);

If the system is under heavy memory or IOMMU pressure and dma_map_page() fails,
it will return an invalid DMA address. Since this address is never verified
with dma_mapping_error(), could the hardware be programmed with an invalid
address, resulting in memory corruption or IOMMU faults?

[Severity: High]
This is a pre-existing issue, but are we leaking the error IRQ in the
ppc440spe_adma_setup_irqs() error path?

If adev->err_irq > 0, the error IRQ is successfully requested using
request_irq(). Later in the function, if of_find_compatible_node() or
of_iomap() fails, the code jumps to err_req2:

err_req2:
	free_irq(adev->irq, chan);
err_req1:
	irq_dispose_mapping(adev->irq);
err_irq_map:
	if (adev->err_irq > 0) {

The err_req2 block calls free_irq(adev->irq, chan) for the primary IRQ, but
it appears to miss calling free_irq(adev->err_irq, chan) for the error IRQ.

If the probe function subsequently fails and frees the chan structure, could a
firing error interrupt execute the handler with the freed chan pointer, leading
to a use-after-free?

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