Re: [PATCH v2 51/64] dmaengine: bcm2835: kill vchan BH on teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing of_dma_controller_free() in the probe error path and remove function leaves a dangling pointer in the OF DMA list.
- [High] Force unbinding the driver while a client holds a DMA channel leaks the channel's IRQ and DMA pool, leading to a Use-After-Free when the IRQ fires.
--

--- Patch [51]: [PATCH v2 51/64] dmaengine: bcm2835: kill vchan BH on teardown ---
commit 922bf2cdbf2470f43f83cc2698c5817aa6b5f545
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: bcm2835: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup instead of tasklet_kill().

> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 06d830d368822..68e86c8d3d439 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -829,7 +829,7 @@ static void bcm2835_dma_free(struct bcm2835_dmadev *od)
>  	list_for_each_entry_safe(c, next, &od->ddev.channels,
>  				 vc.chan.device_node) {
>  		list_del(&c->vc.chan.device_node);
> -		tasklet_kill(&c->vc.task);
> +		dma_chan_kill_bh(&c->vc.chan);
>  	}
>  
>  	dma_unmap_page_attrs(od->ddev.dev, od->zero_page, PAGE_SIZE,

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak and use-after-free
if dma_async_device_register() fails in bcm2835_dma_probe() or if the driver
is forcefully unbound via sysfs.

In bcm2835_dma_probe(), if dma_async_device_register() fails, the error path
jumps to err_no_dma:

drivers/dma/bcm2835-dma.c:bcm2835_dma_probe() {
    ...
	rc = dma_async_device_register(&od->ddev);
	if (rc) {
		dev_err(&pdev->dev,
			"Failed to register slave DMA engine device: %d\n", rc);
		goto err_no_dma;
	}
    ...
err_no_dma:
	bcm2835_dma_free(od);
	return rc;
}

Since of_dma_controller_register() was called earlier in the probe function,
does this error path leave a dangling pointer in the OF DMA list?

If the devm-managed bcm2835_dmadev struct is freed automatically by devres,
any subsequent DMA channel request from another driver matching this device
tree node might dereference the freed pointer during the bcm2835_dma_xlate
callback.

Similarly, in bcm2835_dma_remove(), the OF DMA controller is not freed:

drivers/dma/bcm2835-dma.c:bcm2835_dma_remove() {
	struct bcm2835_dmadev *od = platform_get_drvdata(pdev);

	dma_async_device_unregister(&od->ddev);
	bcm2835_dma_free(od);
}

Should of_dma_controller_free() be called in both of these paths?

[Severity: High]
This is also a pre-existing issue, but I noticed another potential
use-after-free and memory leak if the driver is forcefully unbound via sysfs
while a client still holds a DMA channel.

When a client requests a channel, an IRQ and a DMA pool are dynamically
allocated:

drivers/dma/bcm2835-dma.c:bcm2835_dma_alloc_chan_resources() {
    ...
	return request_irq(c->irq_number, bcm2835_dma_callback,
			   c->irq_flags, "DMA IRQ", c);
}

If the driver is unbound via sysfs, bcm2835_dma_remove() executes and returns
without tearing down active channel resources, as the DMA engine core skips
forcefully releasing channel resources.

Since the IRQ was requested without devm_, it remains registered after the
driver memory (struct bcm2835_chan) is automatically freed by devres.

Could a subsequent hardware interrupt execute bcm2835_dma_callback() and
dereference the freed channel pointer?

drivers/dma/bcm2835-dma.c:bcm2835_dma_callback() {
    ...
	spin_lock_irqsave(&c->vc.lock, flags);
    ...
}

Should we ensure that free_irq() and dma_pool_destroy() are called when
cleaning up these resources during driver removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=51