diff mbox series

[v6,07/10] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

Message ID 20260902062153.31048-8-logang@deltatee.com
State New
Headers show
Series dmaengine: miscellaneous fixes for ioat, switchtec_dma and plx_dma | expand

Commit Message

Logan Gunthorpe Sept. 2, 2026, 6:21 a.m. UTC 
dma_async_device_unregister() can drop the last reference on dma_dev
and free swdma_dev synchronously via switchtec_dma_release(), but
switchtec_dma_remove() then uses swdma_dev->bar for iounmap().

Cache bar in a local variable before the unregister call.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/switchtec_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 6:37 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unchecked ioremap() return value leading to potential NULL pointer dereference
- [High] Spinlock held across DMA engine API boundaries leading to deadlock on sequential prep
- [High] The driver invokes the client's completion callback with `complete_lock` held, leading to a self-deadlock if the client interacts with the channel.
--

commit f727848a55e557b0732eca07f63de472baae9774
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

Fixes a use-after-free bug in switchtec_dma_remove() where swdma_dev->bar
was used after dma_async_device_unregister() could have freed swdma_dev.
The patch caches bar in a local variable before unregistering the DMA device.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 63f80e2a5b6f4..d390949b843cd 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1380,6 +1380,7 @@ static int switchtec_dma_probe(struct pci_dev *pdev,
>  static void switchtec_dma_remove(struct pci_dev *pdev)
>  {
>  	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> +	void __iomem *bar = swdma_dev->bar;
>  
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  
> @@ -1392,7 +1393,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
>  
>  	dma_async_device_unregister(&swdma_dev->dma_dev);
>  
> -	iounmap(swdma_dev->bar);
> +	iounmap(bar);
>  	pci_release_mem_regions(pdev);
>  	pci_disable_device(pdev);
>  }

While reviewing this patch, I noticed some pre-existing issues in the same
file.

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
...
	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 is the return value of ioremap() checked
here?

If memory mapping fails and returns NULL, calling readw() on
swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC will result in a NULL pointer
dereference and kernel panic during device probe.


drivers/dma/switchtec_dma.c:switchtec_dma_prep_desc() {
...
	spin_lock_bh(&swdma_chan->submit_lock);

	if (!swdma_chan->ring_active)
		goto err_unlock;
...
	/* return with the lock held, it will be released in tx_submit */

	return &desc->txd;
...
}

drivers/dma/switchtec_dma.c:switchtec_dma_tx_submit() {
...
	cookie = dma_cookie_assign(desc);

	spin_unlock_bh(&swdma_chan->submit_lock);

	return cookie;
}

[Severity: High]
This isn't a bug introduced by this patch, but could holding the spinlock
across the DMA engine API boundaries lead to a deadlock on sequential prep
calls?

If a generic client calls prep_memcpy() twice in a row before submitting,
wouldn't the second call self-deadlock the CPU attempting to acquire the
already-held swdma_chan->submit_lock?

Also, does this permanently leak the lock if a client prepares a descriptor
but frees it instead of submitting?


drivers/dma/switchtec_dma.c:switchtec_dma_cleanup_completed() {
...
	do {
		spin_lock_bh(&swdma_chan->complete_lock);
...
		do {
			dma_cookie_complete(&desc->txd);
			dma_descriptor_unmap(&desc->txd);
			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
...
}

[Severity: High]
While this is a pre-existing issue, does invoking the client's completion
callback with complete_lock held expose the system to a self-deadlock?

It is legal for DMA clients to interact with the channel from their
completion callback, such as checking the status via
dmaengine_tx_status() or calling dmaengine_terminate_all().

If a client does this, the call routes to switchtec_dma_tx_status() or
switchtec_dma_terminate_all(), both of which attempt to acquire the same
complete_lock, resulting in a permanent self-deadlock on the same CPU.
diff mbox series

Patch

diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
index 63f80e2a5b6f..d390949b843c 100644
--- a/drivers/dma/switchtec_dma.c
+++ b/drivers/dma/switchtec_dma.c
@@ -1380,6 +1380,7 @@  static int switchtec_dma_probe(struct pci_dev *pdev,
 static void switchtec_dma_remove(struct pci_dev *pdev)
 {
 	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
+	void __iomem *bar = swdma_dev->bar;
 
 	switchtec_dma_chans_release(pdev, swdma_dev);
 
@@ -1392,7 +1393,7 @@  static void switchtec_dma_remove(struct pci_dev *pdev)
 
 	dma_async_device_unregister(&swdma_dev->dma_dev);
 
-	iounmap(swdma_dev->bar);
+	iounmap(bar);
 	pci_release_mem_regions(pdev);
 	pci_disable_device(pdev);
 }