[PATCHv3] dmaengine: ti: omap-dma: turn lch_map into a flexible array

[PATCHv3] dmaengine: ti: omap-dma: turn lch_map into a flexible array

[Rosen Penev]

Convert the separately-allocated lch_map pointer array to a C99
flexible array member at the end of struct omap_dmadev and annotate it
with __counted_by(lch_count). The probe is reordered so platform_data
lookup and the lch_count determination happen before the parent
allocation, letting struct_size() size the FAM and the dedicated
devm_kcalloc() for lch_map go away.

Two allocations collapse into one and the runtime bounds checks from
__counted_by now apply to every lch_map[] access.

Add some fixes reported by Sashiko. Missing return and missing check for
needs_busy_check. Also a free_irq ordering issue.

Assisted-by: Claude:Opus-4.7
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: fix sashiko warnings again
 v2: fix sashiko warnings
 drivers/dma/ti/omap-dma.c | 82 ++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 55ece7fd0d99..7c46be2755be 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -48,7 +48,7 @@ struct omap_dmadev {
 	const struct omap_dma_config *cfg;
 	struct notifier_block nb;
 	struct omap_dma_context context;
-	int lch_count;
+	u32 lch_count;
 	DECLARE_BITMAP(lch_bitmap, OMAP_SDMA_CHANNELS);
 	struct mutex lch_lock;		/* for assigning logical channels */
 	bool legacy;
@@ -57,7 +57,7 @@ struct omap_dmadev {
 	unsigned dma_requests;
 	spinlock_t irq_lock;
 	uint32_t irq_enable_mask;
-	struct omap_chan **lch_map;
+	struct omap_chan *lch_map[] __counted_by(lch_count);
 };

 struct omap_chan {
@@ -1656,36 +1656,55 @@ static const struct omap_dma_config default_cfg;
 static int omap_dma_probe(struct platform_device *pdev)
 {
 	const struct omap_dma_config *conf;
+	struct omap_system_dma_plat_info *plat;
 	struct omap_dmadev *od;
+	u32 lch_count;
 	int rc, i, irq;
 	u32 val;

-	od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
-	if (!od)
-		return -ENOMEM;
-
-	od->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(od->base))
-		return PTR_ERR(od->base);
-
 	conf = of_device_get_match_data(&pdev->dev);
 	if (conf) {
-		od->cfg = conf;
-		od->plat = dev_get_platdata(&pdev->dev);
-		if (!od->plat) {
+		plat = dev_get_platdata(&pdev->dev);
+		if (!plat) {
 			dev_err(&pdev->dev, "omap_system_dma_plat_info is missing");
 			return -ENODEV;
 		}
 	} else if (IS_ENABLED(CONFIG_ARCH_OMAP1)) {
-		od->cfg = &default_cfg;
-
-		od->plat = omap_get_plat_info();
-		if (!od->plat)
+		plat = omap_get_plat_info();
+		if (!plat)
 			return -EPROBE_DEFER;
 	} else {
 		return -ENODEV;
 	}

+	/* Number of available logical channels */
+	if (!pdev->dev.of_node) {
+		lch_count = plat->dma_attr->lch_count;
+		if (unlikely(!lch_count))
+			lch_count = OMAP_SDMA_CHANNELS;
+	} else if (of_property_read_u32(pdev->dev.of_node, "dma-channels", &lch_count)) {
+		dev_info(&pdev->dev, "Missing dma-channels property, using %u.\n",
+			 OMAP_SDMA_CHANNELS);
+		lch_count = OMAP_SDMA_CHANNELS;
+	}
+
+	if (lch_count > OMAP_SDMA_CHANNELS) {
+		dev_err(&pdev->dev, "invalid dma-channels value %u\n", lch_count);
+		return -EINVAL;
+	}
+
+	od = devm_kzalloc(&pdev->dev, struct_size(od, lch_map, lch_count), GFP_KERNEL);
+	if (!od)
+		return -ENOMEM;
+
+	od->lch_count = lch_count;
+	od->plat = plat;
+	od->cfg = conf ? conf : &default_cfg;
+
+	od->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(od->base))
+		return PTR_ERR(od->base);
+
 	od->reg_map = od->plat->reg_map;

 	dma_cap_set(DMA_SLAVE, od->ddev.cap_mask);
@@ -1730,19 +1749,6 @@ static int omap_dma_probe(struct platform_device *pdev)
 			 OMAP_SDMA_REQUESTS);
 	}

-	/* Number of available logical channels */
-	if (!pdev->dev.of_node) {
-		od->lch_count = od->plat->dma_attr->lch_count;
-		if (unlikely(!od->lch_count))
-			od->lch_count = OMAP_SDMA_CHANNELS;
-	} else if (of_property_read_u32(pdev->dev.of_node, "dma-channels",
-					&od->lch_count)) {
-		dev_info(&pdev->dev,
-			 "Missing dma-channels property, using %u.\n",
-			 OMAP_SDMA_CHANNELS);
-		od->lch_count = OMAP_SDMA_CHANNELS;
-	}
-
 	/* Mask of allowed logical channels */
 	if (pdev->dev.of_node && !of_property_read_u32(pdev->dev.of_node,
 						       "dma-channel-mask",
@@ -1754,12 +1760,6 @@ static int omap_dma_probe(struct platform_device *pdev)
 	if (od->plat->dma_attr->dev_caps & HS_CHANNELS_RESERVED)
 		bitmap_set(od->lch_bitmap, 0, 2);

-	od->lch_map = devm_kcalloc(&pdev->dev, od->lch_count,
-				   sizeof(*od->lch_map),
-				   GFP_KERNEL);
-	if (!od->lch_map)
-		return -ENOMEM;
-
 	for (i = 0; i < od->dma_requests; i++) {
 		rc = omap_dma_chan_init(od);
 		if (rc) {
@@ -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;
 		}
 	}

@@ -1852,17 +1853,18 @@ 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)
 		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);

+	irq = platform_get_irq(pdev, 1);
+	if (irq > 0)
+		devm_free_irq(&pdev->dev, irq, od);
+
 	if (!omap_dma_legacy(od)) {
 		/* Disable all interrupts */
 		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
--
2.54.0

Re: [PATCHv3] dmaengine: ti: omap-dma: turn lch_map into a flexible array

[sashiko-bot]

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

Pre-existing issues:
- [High] Freeing the shared IRQ handler before properly disabling hardware interrupts can cause an interrupt storm.
- [High] The late error path in probe frees channels while the shared IRQ handler is active, leading to UAF.
- [High] Destroying the descriptor pool before killing virt-dma tasklets causes a UAF when tasklets free memory.
--

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

dmaengine: ti: omap-dma: turn lch_map into a flexible array

Convert the separately-allocated lch_map pointer array to a C99
flexible array member at the end of struct omap_dmadev and annotate it
with __counted_by(lch_count). This patch also adds fixes for a missing
return, a needs_busy_check, and a free_irq ordering issue.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..7c46be2755be7 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;
>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does destroying the descriptor
pool before freeing the channels lead to a use-after-free?

omap_dma_free() kills the virt-dma tasklets:

drivers/dma/ti/omap-dma.c:omap_dma_free() {
	while (!list_empty(&od->ddev.channels)) {
        ...
		tasklet_kill(&c->vc.task);
		kfree(c);
	}
}

Because tasklets can run concurrently until they are explicitly killed, could a
pending tasklet invoke omap_dma_desc_free() and attempt to return memory to the
already destroyed od->desc_pool?

[Severity: High]
This is also a pre-existing issue, but is it safe to free the channels while
the shared IRQ handler is still active?

The IRQ handler was registered with devm_request_irq() earlier in probe(), so
it remains active during this error path since devres hasn't cleaned it up yet.

If a shared interrupt fires, could omap_dma_irq() dereference the freed channel
pointers in od->lch_map?

[ ... ]
> @@ -1852,17 +1853,18 @@ 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)
>  		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);
>  
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq > 0)
> +		devm_free_irq(&pdev->dev, irq, od);
> +
>  	if (!omap_dma_legacy(od)) {
>  		/* Disable all interrupts */
>  		omap_dma_glbl_write(od, IRQENABLE_L0, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does freeing the shared IRQ
handler before disabling hardware interrupts create a window for an interrupt
storm?

Since the IRQ is requested with IRQF_SHARED, if the handler is freed before
the hardware stops asserting the interrupt, the unhandled interrupt might cause
the kernel to permanently disable the shared IRQ line.

Additionally, probe() configures IRQENABLE_L1 to enable interrupts, but this
code attempts to disable them by writing to IRQENABLE_L0. Does this mean the
hardware interrupts may not actually be disabled here?

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