Re: [PATCH v3 02/11] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

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

New issues:
- [High] DMA use-after-free due to missing channel halt in the allocation error path.

Pre-existing issues:
- [High] Race condition in driver teardown leading to MMIO use-after-free and kernel oops.
--

--- Patch [2]: [PATCH v3 02/11] dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources ---
commit d5306db114dcdc27e3a80dfb2bb3a42a91ab9a3a
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix resource leak in alloc_chan_resources

This commit fixes a resource leak in switchtec_dma_alloc_chan_resources
by adding an error-unwind path. The new path disables the channel, clears
the active flags, and frees the descriptor rings if an allocation fails.

Link: https://lore.kernel.org/dmaengine/[email protected]
Fixes: 30eba9df76ad ("dmaengine: switchtec-dma: Implement hardware initialization and cleanup")

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index a4a7d66d042de..f77da31aeb65e 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1029,6 +1030,20 @@ static int switchtec_dma_alloc_chan_resources(struct dma_chan *chan)
>  		FIELD_GET(PERF_MRRS_MASK, perf_cfg));
>  
>  	return SWITCHTEC_DMA_SQ_SIZE;
> +
> +err_ring_inactive:
> +	spin_lock_bh(&swdma_chan->submit_lock);
> +	swdma_chan->ring_active = false;
> +	spin_unlock_bh(&swdma_chan->submit_lock);
> +
> +	spin_lock_bh(&swdma_chan->complete_lock);
> +	swdma_chan->comp_ring_active = false;
> +	spin_unlock_bh(&swdma_chan->complete_lock);
> +err_disable_channel:
> +	disable_channel(swdma_chan);
> +err_free_desc:
> +	switchtec_dma_free_desc(swdma_chan);

[Severity: High]
If an error occurs in switchtec_dma_alloc_chan_resources() after
unhalt_channel() succeeds (for example, if the RCU pointer check fails),
the unwind path jumps to err_ring_inactive and falls through to
err_disable_channel.

Since disable_channel() only clears a software configuration bit and does
not explicitly issue a hardware halt command or wait for the hardware to
stop processing, could the hardware channel continue performing DMA accesses?

If so, does calling switchtec_dma_free_desc() immediately after
disable_channel() create a window for a DMA use-after-free, where the device
can still read or write into the newly freed coherent memory pages?

> +	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but looking at the driver teardown sequence
in switchtec_dma_remove():

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
    ...
    switchtec_dma_chans_release(pdev, swdma_dev);
    rcu_assign_pointer(swdma_dev->pdev, NULL);
    synchronize_rcu();
    pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
    pci_free_irq_vectors(pdev);
    dma_async_device_unregister(&swdma_dev->dma_dev);
    iounmap(swdma_dev->bar);
    ...
}

Because the resources and MMIO regions are dismantled before the device is
unregistered from the dmaengine core, is it possible for a concurrent
client to call dma_request_chan() and allocate a channel during this window?

If a concurrent allocation finishes right after the channel was freed,
could the client attempt to submit a DMA operation and trigger a kernel
oops when accessing the unmapped MMIO registers?

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