Re: [PATCH v2 54/64] dmaengine: st_fdma: use dma_chan_kill_bh

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

Pre-existing issues:
- [High] Calling `list_del()` in `st_fdma_free()` prematurely empties the channel list, breaking devres-based DMA engine unregistration and silently leaking resources on probe failure.
- [High] The per-channel completion BH work is never cancelled during driver removal, leading to a Use-After-Free if scheduled before devres frees channel memory.
- [High] The driver lacks a `device_synchronize` callback and destroys the DMA pool before pending BH work is cancelled, leading to a UAF or NULL pointer dereference on channel release.
- [High] Virtual DMA descriptors are permanently leaked because `st_fdma_free_chan_res()` omits a call to `vchan_free_chan_resources()`.
- [High] Inverted teardown sequence in `st_fdma_remove` vs devres allows DMA transactions on partially destroyed hardware.
--

--- Patch [54]: [PATCH v2 54/64] dmaengine: st_fdma: use dma_chan_kill_bh ---
commit 4cab1e1fac46ef4cc457cee5eab4226a73880829
Author: Allen Pais <[email protected]>
Subject: dmaengine: st_fdma: use dma_chan_kill_bh

virt-dma now dispatches completion callbacks through per-channel BH
work instead of its tasklet. Cancel that work during teardown before
channel storage is released.

> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
> index d9547017f3bd6..d4cff851e1d52 100644
> --- a/drivers/dma/st_fdma.c
> +++ b/drivers/dma/st_fdma.c
> @@ -733,7 +733,7 @@ static void st_fdma_free(struct st_fdma_dev *fdev)
>  	for (i = 0; i < fdev->nr_channels; i++) {
>  		fchan = &fdev->chans[i];
>  		list_del(&fchan->vchan.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does calling list_del() here prematurely
empty the channel list and break devres-based DMA engine unregistration?

If probe fails at of_dma_controller_register(), st_fdma_free() is called and
unlinks all channels from fdev->dma_device.channels.

When devres subsequently invokes dma_async_device_unregister(), it iterates
over the now-empty list and skips calling
__dma_async_device_channel_unregister(). This could permanently leak DMA
engine channel structures, IDA allocations, and sysfs nodes.

> -		tasklet_kill(&fchan->vchan.task);
> +		dma_chan_kill_bh(&fchan->vchan.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but since this patch updates teardown paths to
use dma_chan_kill_bh(), should st_fdma_remove() be updated as well?

drivers/dma/st_fdma.c:st_fdma_remove() {
    ...
	devm_free_irq(&pdev->dev, fdev->irq, fdev);
	st_slim_rproc_put(fdev->slim_rproc);
	of_dma_controller_free(pdev->dev.of_node);
}

The per-channel completion BH work is never cancelled during driver removal.
If the driver is unbound and an interrupt schedules the BH work just before
devm_free_irq() disables it, devres will free fdev->chans. 

The delayed BH (vchan_complete) would then execute on the freed memory,
causing a use-after-free.

[Severity: High]
This is a pre-existing issue, but could destroying the DMA pool without
synchronizing against pending BH work lead to a use-after-free or null
pointer dereference?

drivers/dma/st_fdma.c:st_fdma_free_chan_res() {
    ...
	dma_pool_destroy(fchan->node_pool);
	fchan->node_pool = NULL;
    ...
}

The driver lacks a device_synchronize callback (e.g., pointing to
vchan_synchronize). Because of this, dmaengine_synchronize() returns
immediately without waiting for pending virt-dma BH work.

If vchan_complete() executes concurrently or shortly after, it invokes
st_fdma_free_desc():

drivers/dma/st_fdma.c:st_fdma_free_desc() {
    ...
		dma_pool_free(fdesc->fchan->node_pool, fdesc->node[i].desc,
    ...
}

Passing the null node_pool to dma_pool_free() could crash the kernel.

[Severity: High]
This is a pre-existing issue, but are virtual DMA descriptors permanently
leaked here because st_fdma_free_chan_res() omits a call to
vchan_free_chan_resources()?

drivers/dma/st_fdma.c:st_fdma_free_chan_res() {
    ...
    // missing call to vchan_free_chan_resources(&fchan->vchan)
    ...
}

If a client prepares a DMA transaction (allocating virtual descriptors onto
vchan->desc_allocated) but releases the channel without submitting them,
st_fdma_free_chan_res() destroys the hardware node pool.

Because vchan_free_chan_resources() isn't called, the virtual descriptors are
orphaned on internal lists, leaking their memory.

[Severity: High]
This is a pre-existing issue, but does the teardown sequence in
st_fdma_remove() allow DMA transactions on partially destroyed hardware?

drivers/dma/st_fdma.c:st_fdma_remove() {
    ...
	st_slim_rproc_put(fdev->slim_rproc);
    ...
}

Manual hardware power-down occurs here before the devres-managed
dma_async_device_unregister() runs. 

During this window, clients can still issue DMA requests via
dmaengine_submit(), which would write to unclocked or powered-down hardware
registers.

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