Re: [PATCH v2 53/64] dmaengine: fsl-qdma: use dma_chan_kill_bh

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

Pre-existing issues:
- [High] Prematurely removing channels from the `dmadev->channels` list in `fsl_qdma_cleanup_vchan()` bypasses core DMA engine unregistration, leading to resource leaks and Use-After-Free vulnerabilities.
- [High] The driver fails to halt the DMA engine during device removal and tears down IRQs before unregistering the DMA controller.
- [High] Unconditionally destroying shared DMA pools in `fsl_qdma_free_chan_resources()` causes a Use-After-Free for other active channels.
--

--- Patch [53]: [PATCH v2 53/64] dmaengine: fsl-qdma: use dma_chan_kill_bh ---
commit 705da09605a777fa7dd32343007b9e054bcb5942
Author: Allen Pais <[email protected]>

dmaengine: fsl-qdma: 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.

Signed-off-by: Allen Pais <[email protected]>

> diff --git a/drivers/dma/fsl-qdma.c b/drivers/dma/fsl-qdma.c
> index df843fad0ece5..e0fb39fe821c9 100644
> --- a/drivers/dma/fsl-qdma.c
> +++ b/drivers/dma/fsl-qdma.c
> @@ -1255,7 +1255,7 @@ static void fsl_qdma_cleanup_vchan(struct dma_device *dmadev)
>  	list_for_each_entry_safe(chan, _chan,
>  				 &dmadev->channels, vchan.chan.device_node) {
>  		list_del(&chan->vchan.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does removing this channel from the
dmadev->channels list here bypass the core DMA engine unregistration?

If the driver empties the channel list before the core DMA framework
unregisters it in fsl_qdma_remove():

drivers/dma/fsl-qdma.c:fsl_qdma_remove() {
        ...
        fsl_qdma_cleanup_vchan(&fsl_qdma->dma_dev);
        ...
        dma_async_device_unregister(&fsl_qdma->dma_dev);
}

The framework's cleanup loop will be silently skipped:

drivers/dma/dmaengine.c:dma_async_device_unregister() {
        ...
        list_for_each_entry_safe(chan, n, &device->channels, device_node)
                __dma_async_device_channel_unregister(device, chan);
        ...
}

Could this leave sysfs entries exposed and cause a use-after-free when
the underlying memory is freed?

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

[Severity: High]
This is also a pre-existing issue, but does the driver fail to halt the
DMA engine during device removal?

Looking at fsl_qdma_remove(), IRQs are destroyed while the controller
might still be active:

drivers/dma/fsl-qdma.c:fsl_qdma_remove() {
        ...
        fsl_qdma_irq_exit(pdev, fsl_qdma);
        fsl_qdma_cleanup_vchan(&fsl_qdma->dma_dev);
        ...
}

Since fsl_qdma_halt() is not called here, could the hardware continue
performing reads or writes against freed memory?

[Severity: High]
This is another pre-existing issue, but does destroying the shared DMA
pools in fsl_qdma_free_chan_resources() cause a use-after-free for
other active channels?

During initialization, multiple channels can share the same physical
queue structure and memory pools:

drivers/dma/fsl-qdma.c:fsl_qdma_probe() {
        ...
        fsl_chan->queue = fsl_qdma->queue + i % (fsl_qdma->n_queues *
                                        fsl_qdma->block_number);
        ...
}

However, when a channel is freed, the shared pools are unconditionally
destroyed:

drivers/dma/fsl-qdma.c:fsl_qdma_free_chan_resources() {
        ...
        dma_pool_destroy(fsl_queue->comp_pool);
        dma_pool_destroy(fsl_queue->desc_pool);
        ...
}

If another active channel shares this same queue, will it crash or
corrupt memory upon subsequent operations?

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