Re: [PATCH v4 07/12] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

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

Pre-existing issues:
- [High] Use-After-Free of `swdma_dev` in `switchtec_dma_remove()` due to teardown ordering.
- [High] Missing validation of `ioremap()` return value leads to NULL pointer dereference.
- [High] IRQ handler registered before data structures are initialized leading to a NULL pointer dereference.
- [High] Unvalidated hardware register `chan_cnt` leads to out-of-bounds MMIO accesses.
--

--- Patch [7]: [PATCH v4 07/12] dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error ---
commit 52eb1f3ef1524cf69a5b20f4b681533c108b4f8f
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix chan_status_irq cleanup on create() error

This commit fixes the cleanup of chan_status_irq in the create() error
path. It properly initializes the irq index to -1 and uses pci_free_irq()
instead of free_irq() to ensure the correct Linux IRQ is freed.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 63e60de9f86dc..6ab2c62b52fd8 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1262,6 +1262,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	if (!swdma_dev)
>  		return -ENOMEM;
>  
> +	swdma_dev->chan_status_irq = -1;
> +
>  	swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
>  				 pci_resource_len(pdev, 0));
>  

[Severity: High]
This is a pre-existing issue, but is it possible for ioremap() to fail here?

If the mapping fails due to exhausted vmalloc space, it returns a NULL pointer.
Shortly after this, the code calls readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC),
which would result in an unconditional NULL pointer dereference.

Should there be a check for swdma_dev->bar being NULL before proceeding?

> @@ -1336,8 +1338,8 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	switchtec_dma_chans_free(swdma_dev);
>  
>  err_exit:
> -	if (swdma_dev->chan_status_irq)
> -		free_irq(swdma_dev->chan_status_irq, swdma_dev);
> +	if (swdma_dev->chan_status_irq >= 0)
> +		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
>  
>  	iounmap(swdma_dev->bar);
>  	kfree(swdma_dev);

While reviewing this patch, I noticed a few other pre-existing issues in the
surrounding code:

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an
uninitialized list dereference if an interrupt fires early?

In switchtec_dma_create(), pci_request_irq() is called and enables
switchtec_dma_chan_status_isr before the dma->channels list is initialized
by INIT_LIST_HEAD().

If an interrupt fires during this window, switchtec_dma_chan_status_isr() will
iterate over the zero-initialized dma_dev->channels list:

switchtec_dma_chan_status_isr()
    list_for_each_entry(chan, &dma_dev->channels, device_node) {

Since swdma_dev was allocated with kzalloc_obj, the next pointer is NULL,
which would cause an invalid pointer dereference when evaluating
chan->dev->device.

Should the IRQ be requested after the channel list is fully initialized?

[Severity: High]
This isn't a bug introduced by this patch, but does this code trust the
hardware register chan_cnt too much?

The driver reads chan_cnt directly from SWITCHTEC_REG_CHAN_CNT:

switchtec_dma_create()
    chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);

This count is then used in switchtec_dma_chans_enumerate() to calculate MMIO
offsets via pointer arithmetic:

switchtec_dma_chan_init()
    swdma_chan->mmio_chan_fw = swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
                               i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;

If a faulty or maliciously crafted PCI endpoint provides a dangerously large
chan_cnt, could this cause the driver to read and write far outside the safely
allocated ioremap region?

Should chan_cnt be validated against the actual size of the mapped PCI BAR?

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
during device teardown?

During module unload or device removal, switchtec_dma_remove() calls
dma_async_device_unregister(&swdma_dev->dma_dev).

This synchronously drops the reference count of the DMA device to 0 if there
are no active clients, which triggers the switchtec_dma_release() callback.

switchtec_dma_release()
    kfree(swdma_dev);

Immediately following this, switchtec_dma_remove() continues teardown:

switchtec_dma_remove()
    dma_async_device_unregister(&swdma_dev->dma_dev);
    iounmap(swdma_dev->bar);

Because swdma_dev has already been freed, does accessing the bar member for
unmapping result in a use-after-free?

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