Re: [PATCH v5 08/12] dmaengine: switchtec-dma: disable channels before freeing on registration failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing check for ioremap failure in switchtec_dma_create() leads to NULL pointer dereference.
- [High] The channel status IRQ handler dereferences uninitialized structures if the IRQ fires before dma_async_device_register() completes.
- [High] Missing bounds check on untrusted `chan_cnt` read from the PCI device allows out-of-bounds MMIO pointer arithmetic and memory writes.
--

commit 9d90eebe0f109c6aab9cf75114365b10451e1028
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: disable channels before freeing on registration failure

When switchtec_dma_create() fails after channels have been added to
dma_dev->channels, the channel status IRQ is still live while the channels
are released and freed. Add switchtec_dma_chans_disable() to free the
channel status IRQ before releasing and freeing channels in both error paths.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Link: https://lore.kernel.org/dmaengine/20260721162822.05CDD1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));
>  
>  	RCU_INIT_POINTER(swdma_dev->pdev, pdev);
>  
>  	nr_vecs = pci_msix_vec_count(pdev);
>  	rc = pci_alloc_irq_vectors(pdev, nr_vecs, nr_vecs, PCI_IRQ_MSIX);
>  	if (rc < 0)
>  		goto err_exit;
>  
>  	irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);

[Severity: High]
This is a pre-existing issue, but does this code cause a NULL pointer
dereference if ioremap() fails? The swdma_dev->bar pointer does not appear
to be checked before being unconditionally dereferenced by readw() during
device probe.

>  	pci_dbg(pdev, "Channel pause irq vector %d\n", irq);
>  
>  	rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
>  			     swdma_dev, KBUILD_MODNAME);
>  	if (rc)
>  		goto err_exit;

[Severity: High]
This is a pre-existing issue, but does this request the IRQ too early? If
the hardware asserts the IRQ immediately, the ISR will run before the
channels are fully initialized.

Looking at switchtec_dma_chan_status_isr():

	list_for_each_entry(chan, &dma_dev->channels, device_node) {
		swdma_chan = container_of(chan, struct switchtec_dma_chan,
					  dma_chan);
		chan_dev = &swdma_chan->dma_chan.dev->device;
		chan_hw = swdma_chan->mmio_chan_hw;

Could this dereference an invalid pointer if the channels list is empty or
if chan->dev is not yet initialized by dma_async_device_register()?

>  	swdma_dev->chan_status_irq = irq;
>  
>  	chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);
>  	if (!chan_cnt) {
>  		pci_err(pdev, "No channel configured.\n");
>  		rc = -ENXIO;
>  		goto err_exit;
>  	}
>  
>  	chan_cnt = switchtec_dma_chans_enumerate(swdma_dev, pdev, chan_cnt);

[Severity: High]
This is a pre-existing issue, but is there a missing upper bounds check on
chan_cnt? Since chan_cnt is read directly from hardware, an untrusted
device could provide a very large count.

In switchtec_dma_chan_init(), this unvalidated count is used for pointer
arithmetic:

	swdma_chan->mmio_chan_fw =
		swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
		i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;
	swdma_chan->mmio_chan_hw =
		swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CTRL_OFFSET +
		i * SWITCHTEC_DMA_CHAN_HW_REGS_SIZE;

Could this allow out-of-bounds MMIO pointer arithmetic and memory writes
outside the valid bar resource window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831191403.207631-1-logang@deltatee.com?part=8