Re: [PATCH v3 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

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

New issues:
- [High] Incomplete fix / DMA Use-After-Free due to early return in switchtec_dma_chan_stop() on halt timeout.

Pre-existing issues:
- [High] Soft lockup and severe latency spikes due to 100ms atomic busy-waiting under a spinlock.
- [High] Extreme allocation brittleness due to requesting 32,768 descriptors with GFP_NOWAIT.
- [High] DMA Use-After-Free in switchtec_dma_free_chan_resources() due to incorrect teardown ordering.
- [High] Massive Use-After-Free of swdma_dev during driver removal.
--

--- Patch [3]: [PATCH v3 03/11] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error ---
commit 647e1927ea58080fcbb88ef03dd8a74aa7555178
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

The commit adds a call to switchtec_dma_chan_stop() on the error-unwind
path of switchtec_dma_alloc_chan_resources(). This aims to synchronously 
halt the channel and clear DMA base registers before freeing the
descriptors, preventing a potential use-after-free if the hardware 
continues accessing the rings.

Link: https://lore.kernel.org/dmaengine/[email protected]

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..107769cca772f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1040,6 +1040,7 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  	swdma_chan->comp_ring_active = false;
>  	spin_unlock_bh(&swdma_chan->complete_lock);
>  err_disable_channel:
> +	switchtec_dma_chan_stop(swdma_chan);
>  	disable_channel(swdma_chan);
>  err_free_desc:
>  	switchtec_dma_free_desc(swdma_chan);

[Severity: High]
Does this completely resolve the use-after-free if the hardware is 
unresponsive?

Looking at switchtec_dma_chan_stop(), if halt_channel() times out, the 
function returns early without clearing the DMA base registers:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_stop() {
        rc = halt_channel(swdma_chan);
        if (rc)
                return;
        ...
        writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
        ...
}

If that happens, execution continues to disable_channel() which does an
unflushed write, and then switchtec_dma_free_desc() unconditionally frees
the descriptors while the hardware might still be accessing them.


[Severity: High]
This is a pre-existing issue, but can this lead to soft lockups or severe 
latency spikes?

In halt_channel(), wait_for_chan_status() is called while holding a spinlock:

drivers/dma/switchtec_dma.c:halt_channel() {
        spin_lock(&swdma_chan->hw_ctrl_lock);
        writeb(SWITCHTEC_CHAN_CTRL_HALT, &chan_hw->ctrl);
        ret = wait_for_chan_status(chan_hw, SWITCHTEC_CHAN_STS_HALTED, true);
        spin_unlock(&swdma_chan->hw_ctrl_lock);
        ...
}

wait_for_chan_status() uses readl_poll_timeout_atomic() with a 100ms timeout.
Is it safe to block preemption for up to 100 milliseconds if the hardware 
is slow to respond?


[Severity: High]
This isn't a bug introduced by this patch, but does this allocation strategy 
cause issues under memory pressure?

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
        for (i = 0; i < SWITCHTEC_DMA_RING_SIZE; i++) {
                desc = kzalloc_obj(*desc, GFP_NOWAIT);
                if (!desc) {
        ...
}

SWITCHTEC_DMA_RING_SIZE is 32,768. Iteratively requesting 32,768 allocations 
with GFP_NOWAIT forbids sleeping and memory reclaim. Could this rapidly 
drain atomic memory pools and fail on slightly fragmented systems?


[Severity: High]
This is a pre-existing issue, but could this teardown ordering result in 
a use-after-free?

drivers/dma/switchtec_dma.c:switchtec_dma_free_chan_resources() {
        switchtec_dma_chan_stop(swdma_chan);
        switchtec_dma_abort_desc(swdma_chan, 0);
        switchtec_dma_free_desc(swdma_chan);
        
        disable_channel(swdma_chan);
        ...
}

If switchtec_dma_chan_stop() times out and fails to halt the channel, 
the descriptors are freed by switchtec_dma_free_desc() before the 
channel enable bit is cleared by disable_channel(). Could the active 
hardware continue executing and corrupt the freed descriptor memory?


[Severity: High]
This is a pre-existing issue, but does this sequence cause a use-after-free 
of swdma_dev?

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
        ...
        dma_async_device_unregister(&swdma_dev->dma_dev);
        
        iounmap(swdma_dev->bar);
        ...
}

If dma_async_device_unregister() drops the last reference to the DMA device,
it synchronously invokes the release callback, switchtec_dma_release(), 
which frees swdma_dev. Does this mean iounmap(swdma_dev->bar) will 
dereference the freed swdma_dev pointer?

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