[PATCH 0/4] dmaengine: ti: omap-dma: various bug fixes

[PATCH 0/4] dmaengine: ti: omap-dma: various bug fixes

[Rosen Penev]

This series fixes four bugs in the TI OMAP DMA driver:

 - Patch 1: add missing return statement in the probe error path
 - Patch 2: fix a notifier leak in remove
 - Patch 3: fix dma_pool_destroy being called before omap_dma_free
 - Patch 4: fix interrupt handling in the remove path

Rosen Penev (4):
  dmaengine: ti: omap-dma: fix missing return in probe error path
  dmaengine: ti: omap-dma: fix notifier leak in remove
  dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in
    error paths
  dmaengine: ti: omap-dma: fix interrupt handling in remove

 drivers/dma/ti/omap-dma.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

-- 
2.54.0

[PATCH 1/4] dmaengine: ti: omap-dma: fix missing return in probe error path

[Rosen Penev]

If of_dma_controller_register() fails, the error path omits the return
statement, causing probe to continue (and eventually succeed) despite
the DMA controller not being registered. Add the missing return rc;.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 55ece7fd0d99..0f6dd6b0a301 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1828,6 +1828,7 @@ static int omap_dma_probe(struct platform_device *pdev)
 			if (od->ll123_supported)
 				dma_pool_destroy(od->desc_pool);
 			omap_dma_free(od);
+			return rc;
 		}
 	}
 
-- 
2.54.0

[PATCH 2/4] dmaengine: ti: omap-dma: fix notifier leak in remove

[Rosen Penev]

The notifier may be registered for needs_busy_check (omap2420) rather than
may_lose_context (omap3). The remove path only checks may_lose_context,
leaving the notifier registered during driver removal. Check both flags.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 0f6dd6b0a301..839e04f53fc2 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1853,7 +1853,7 @@ static void omap_dma_remove(struct platform_device *pdev)
 	struct omap_dmadev *od = platform_get_drvdata(pdev);
 	int irq;
 
-	if (od->cfg->may_lose_context)
+	if (od->cfg->needs_busy_check || od->cfg->may_lose_context)
 		cpu_pm_unregister_notifier(&od->nb);
 
 	if (pdev->dev.of_node)
-- 
2.54.0

[PATCH 3/4] dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

[Rosen Penev]

omap_dma_free() tears down channels and may free t2_desc entries from
the descriptor pool via tasklet cleanup. Destroying the pool before
omap_dma_free() is a use-after-free. Move omap_dma_free() ahead of
dma_pool_destroy() in both probe error paths and the remove path.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 839e04f53fc2..fd1ad3b4268c 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1808,9 +1808,9 @@ static int omap_dma_probe(struct platform_device *pdev)
 	if (rc) {
 		pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n",
 			rc);
+		omap_dma_free(od);
 		if (od->ll123_supported)
 			dma_pool_destroy(od->desc_pool);
-		omap_dma_free(od);
 		return rc;
 	}
 
@@ -1825,9 +1825,9 @@ static int omap_dma_probe(struct platform_device *pdev)
 		if (rc) {
 			pr_warn("OMAP-DMA: failed to register DMA controller\n");
 			dma_async_device_unregister(&od->ddev);
+			omap_dma_free(od);
 			if (od->ll123_supported)
 				dma_pool_destroy(od->desc_pool);
-			omap_dma_free(od);
 			return rc;
 		}
 	}
@@ -1869,10 +1869,10 @@ static void omap_dma_remove(struct platform_device *pdev)
 		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
 	}
 
+	omap_dma_free(od);
+
 	if (od->ll123_supported)
 		dma_pool_destroy(od->desc_pool);
-
-	omap_dma_free(od);
 }
 
 static const struct omap_dma_config omap2420_data = {
-- 
2.54.0

Re: [PATCH 3/4] dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential Use-After-Free in the IRQ handler during the `of_dma_controller_register` error path in probe.
- [High] Unsafe unbind and devm anti-pattern due to lack of `device_release` callback and failure to drain virt-dma descriptors.
--

commit 7feb24e91af080b6fba5d126723a162d39a35206
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

omap_dma_free() tears down channels and may free t2_desc entries from
the descriptor pool via tasklet cleanup. Destroying the pool before
omap_dma_free() is a use-after-free. Move omap_dma_free() ahead of
dma_pool_destroy() in both probe error paths and the remove path.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 839e04f53fc2d..fd1ad3b4268c9 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1825,9 +1825,9 @@ static int omap_dma_probe(struct platform_device *pdev)
>  		if (rc) {
>  			pr_warn("OMAP-DMA: failed to register DMA controller\n");
>  			dma_async_device_unregister(&od->ddev);
> +			omap_dma_free(od);

[Severity: High]
This is a pre-existing issue, but does this error path leave us vulnerable to
a use-after-free if an interrupt fires during probe failure?

If of_dma_controller_register() fails, the probe error path calls
omap_dma_free(), which frees the channel objects via kfree().

However, it does not appear to stop active DMA hardware, clear
od->irq_enable_mask, or unregister the IRQ. Because the DMA device was
already exposed by a successful dma_async_device_register() earlier in
probe, a concurrent client could have allocated a channel and started a
transfer, enabling hardware interrupts.

If a hardware interrupt fires before devres cleanup unwinds the IRQ:

omap_dma_irq() {
    ...
    c = od->lch_map[i];
    if (c) {
        spin_lock_irqsave(&c->vc.lock, flags);
        ...
        tasklet_schedule(&c->vc.task);
    ...
}

Would this read the freed channel structure from od->lch_map and schedule
a tasklet using a freed pointer?

>  			if (od->ll123_supported)
>  				dma_pool_destroy(od->desc_pool);
> -			omap_dma_free(od);
>  			return rc;
>  		}
>  	}

[ ... ]

> @@ -1869,10 +1869,10 @@ static void omap_dma_remove(struct platform_device *pdev)
>  		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
>  	}
>  
> +	omap_dma_free(od);

[Severity: High]
This is a pre-existing issue, but does forcefully freeing channel structures
here bypass dma_device refcounting and leak descriptor lists?

The driver does not seem to provide a .device_release callback for the
dma_device and forcefully frees the omap_chan structures using kfree() in
omap_dma_free():

omap_dma_free() {
    ...
    list_del(&c->vc.chan.device_node);
    tasklet_kill(&c->vc.task);
    kfree(c);
    ...
}

Because omap_dmadev is allocated with devm_kzalloc(), a forced unbind while a
client holds a channel reference will result in devm freeing omap_dmadev and
omap_dma_free() freeing omap_chan. This bypasses dma_device refcounting and
could cause a use-after-free when the client later attempts to call
dma_release_channel().

Additionally, omap_dma_free() does not drain the internal virt-dma lists
(desc_allocated, desc_submitted, etc.). Does this permanently leak any pending
virt_dma_desc items allocated by clients?

> +
>  	if (od->ll123_supported)
>  		dma_pool_destroy(od->desc_pool);
> -
> -	omap_dma_free(od);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531020535.594460-1-rosenp@gmail.com?part=3

[PATCH 4/4] dmaengine: ti: omap-dma: fix interrupt handling in remove

[Rosen Penev]

The remove path had three pre-existing bugs:

1. Interrupts are enabled via IRQENABLE_L1 in probe and alloc_chan_resources,
   but the remove path writes to IRQENABLE_L0, which has no effect on the L1
   interrupt line. The DMA engine can continue asserting its IRQ during
   removal. Write to IRQENABLE_L1 instead.

2. devm_free_irq() was called before disabling hardware interrupts. With
   IRQF_SHARED, the hardware may still assert the IRQ line after the handler
   is freed, causing unhandled interrupts that can lead to the kernel
   permanently disabling the shared IRQ line. Disable interrupts first.

3. platform_get_irq() return value was not checked before devm_free_irq().
   If it returns an error code (<= 0), passing it to devm_free_irq() is
   incorrect. Add a guard.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index fd1ad3b4268c..ad90ca226db3 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1859,16 +1859,17 @@ static void omap_dma_remove(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
 
-	irq = platform_get_irq(pdev, 1);
-	devm_free_irq(&pdev->dev, irq, od);
-
 	dma_async_device_unregister(&od->ddev);
 
 	if (!omap_dma_legacy(od)) {
-		/* Disable all interrupts */
-		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
+		od->irq_enable_mask = 0;
+		omap_dma_glbl_write(od, IRQENABLE_L1, 0);
 	}
 
+	irq = platform_get_irq(pdev, 1);
+	if (irq > 0)
+		devm_free_irq(&pdev->dev, irq, od);
+
 	omap_dma_free(od);
 
 	if (od->ll123_supported)
-- 
2.54.0

Re: [PATCH 4/4] dmaengine: ti: omap-dma: fix interrupt handling in remove

[sashiko-bot]

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

New issues:
- [High] Data race when modifying IRQ enable mask leaves interrupts vulnerable to storms.

Pre-existing issues:
- [Critical] Use-after-free of DMA channels during forced unbind.
- [High] Missing IO flush before freeing the shared IRQ handler allows unhandled interrupts.
--

commit cf02a74f5cf0d9275769507da876fed12689ebdf
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: ti: omap-dma: fix interrupt handling in remove

This commit aims to fix three interrupt handling bugs in the remove path.
It changes the interrupt disable logic to write to IRQENABLE_L1 instead
of IRQENABLE_L0, disables hardware interrupts before freeing the shared
IRQ handler, and adds a return value check for platform_get_irq().

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index fd1ad3b4268c9..ad90ca226db35 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c
> @@ -1859,16 +1859,17 @@ static void omap_dma_remove(struct platform_device *pdev)
>  	if (pdev->dev.of_node)
>  		of_dma_controller_free(pdev->dev.of_node);
>  
> -	irq = platform_get_irq(pdev, 1);
> -	devm_free_irq(&pdev->dev, irq, od);
> -
>  	dma_async_device_unregister(&od->ddev);
>  
>  	if (!omap_dma_legacy(od)) {
> -		/* Disable all interrupts */
> -		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
> +		od->irq_enable_mask = 0;
> +		omap_dma_glbl_write(od, IRQENABLE_L1, 0);

[Severity: High]
Does clearing od->irq_enable_mask and writing to IRQENABLE_L1 without
holding spin_lock_irq(&od->irq_lock) introduce a data race?

If an interrupt fires concurrently, the IRQ handler (omap_dma_irq) might
read the cleared mask, evaluate status &= 0, and return IRQ_NONE without
acknowledging the pending hardware interrupt.

Furthermore, if a client releases a channel concurrently:

omap_dma_free_chan_resources() {
    spin_lock_irqsave(&od->irq_lock, flags);
    ...
    omap_dma_glbl_write(od, IRQENABLE_L1, od->irq_enable_mask);
    spin_unlock_irqrestore(&od->irq_lock, flags);
}

This could write a non-zero mask to IRQENABLE_L1 immediately after this
remove path disabled it, leaving hardware interrupts active right before
devm_free_irq() removes the handler.

[Severity: High]
This isn't a bug introduced by this patch, but does the posted write to
IRQENABLE_L1 require a dummy read to flush the interconnect's write
buffers before freeing the IRQ?

Because there is no subsequent flush before devm_free_irq() is called,
the hardware disable command may not reach the DMA controller in time.

If the controller asserts an interrupt during this window, the CPU will
see an unhandled interrupt on a shared line, which can prompt the
generic IRQ subsystem to permanently disable the shared IRQ line.

>  	}
>  
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq > 0)
> +		devm_free_irq(&pdev->dev, irq, od);
> +
>  	omap_dma_free(od);

[Severity: Critical]
This is a pre-existing issue, but can the call to omap_dma_free() result
in a use-after-free if a manual sysfs unbind occurs?

omap_dma_free() unconditionally frees all channel structures:

omap_dma_free() {
    while (!list_empty(&od->ddev.channels)) {
        ...
        kfree(c);
    }
}

Since the DMA subsystem does not automatically create strict device links
for consumers, a sysfs unbind of the DMA controller will not synchronously
unbind its consumers.

Active clients will thus continue to access their held channels and
eventually call dma_release_channel() on the freed memory.

>  
>  	if (od->ll123_supported)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531020535.594460-1-rosenp@gmail.com?part=4