Re: [PATCH v4 04/12] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

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

New issues:
- [High] Intentionally leaking descriptors to prevent hardware UAF leaves dangling pointers in the channel structure, causing a UAF if a subsequent allocation fails, or a permanent leak if it succeeds.

Pre-existing issues:
- [Critical] Zeroing DMA base registers while the channel fails to halt redirects active DMA transactions to physical address 0.
- [High] Holding a spinlock while polling for up to 100ms violates core kernel locking rules and can trigger softlockups.
--

--- Patch [4]: [PATCH v4 04/12] dmaengine: switchtec-dma: halt channel on alloc_chan_resources error ---
commit 886fa8092e9d55e67d82b2e1ee9d1125fcadac6d
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: halt channel on alloc_chan_resources error

This patch modifies the error unwind path in channel allocation to call
switchtec_dma_chan_stop() before disabling the channel. This prevents a
hardware use-after-free by ensuring the channel is successfully halted
before freeing the descriptor rings. If the halt fails, it skips freeing
the rings.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index cbbc9ad082471..25b988e9b4afb 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1047,6 +1047,10 @@ 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:
> +	if (switchtec_dma_chan_stop(swdma_chan)) {

[Severity: Critical]
This is a pre-existing issue, but does writing 0 to the DMA base registers
redirect active DMA transactions to physical address 0x0?

Looking at switchtec_dma_chan_stop(), if the halt times out, the registers
are still cleared:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_stop() {
        rc = halt_channel(swdma_chan);
        ...
        if (rc)
                pci_err(pdev, "Channel %d halt timed out...");

        writel(0, &swdma_chan->mmio_chan_fw->sq_base_lo);
        ...
}

Since the channel is still unhalted and active, modifying these live
registers might cause ongoing or pending hardware DMA descriptor fetches to
target physical address 0x0.


[Severity: High]
This is a pre-existing issue, but can this call chain trigger softlockups
by holding a spinlock for up to 100ms?

switchtec_dma_chan_stop() calls halt_channel(), which acquires a spinlock
before polling:

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);
}

And wait_for_chan_status() performs a 100ms atomic polling loop:

drivers/dma/switchtec_dma.c:wait_for_chan_status() {
        return readl_poll_timeout_atomic(&chan_hw->status, status,
                                         ..., 10, 100 * USEC_PER_MSEC);
}

Holding the spinlock disables preemption for the duration of the timeout,
which might cause severe scheduling latency spikes.

> +		disable_channel(swdma_chan);
> +		return rc;

[Severity: High]
When returning here, the descriptors are intentionally leaked to prevent a
hardware use-after-free, but do the pointers in swdma_chan remain populated?

If a subsequent allocation fails in switchtec_dma_alloc_desc(), it will call
switchtec_dma_free_desc() which frees these exact pointers while the hardware
might still be accessing them. 

If the allocation succeeds, the pointers will be overwritten:

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_desc() {
        swdma_chan->hw_sq = dma_alloc_coherent(..., GFP_NOWAIT);
}

Could this lead to a use-after-free on a subsequent allocation error, or a
permanent memory leak if the pointers are overwritten?

> +	}
>  	disable_channel(swdma_chan);
>  err_free_desc:
>  	switchtec_dma_free_desc(swdma_chan);

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