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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbounded `dma-channels` device tree property causes out-of-bounds stack read and heap buffer overflow.
- [High] Missing return on error path and improper unregistration leaves a dangling CPU PM notifier, leading to Use-After-Free.
--

commit 91ef33462c6154050830fe926f0bdab84e94c15b
Author: Rosen Penev <rosenp@gmail.com>
Subject: 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). The probe is reordered so platform_data lookup and
the lch_count determination happen before the parent allocation. This lets
struct_size() size the FAM, collapsing two allocations into one.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..901e38b089624 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1656,36 +1656,53 @@ 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;
> +	int 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)) {

[Severity: High]
This is a pre-existing issue, but does this code properly bound the value
read from the dma-channels device tree property?

If a malformed device tree provides a value greater than OMAP_SDMA_CHANNELS
(which is 32), the subsequent call to bitmap_from_arr32(od->lch_bitmap,
&val, od->lch_count) will attempt to read more than 32 bits from the single
32-bit stack variable 'val'.

Since od->lch_bitmap is strictly sized to 32 bits via:
    DECLARE_BITMAP(lch_bitmap, OMAP_SDMA_CHANNELS);

Could this out-of-bounds stack read silently corrupt subsequent members of
the omap_dmadev structure on the heap (such as the lch_lock mutex)?

> +		dev_info(&pdev->dev,
> +			 "Missing dma-channels property, using %u.\n",
> +			 OMAP_SDMA_CHANNELS);
> +		lch_count = OMAP_SDMA_CHANNELS;
> +	}

[ ... ]

>  	if (pdev->dev.of_node) {
>  		omap_dma_info.dma_cap = od->ddev.cap_mask;
>  
>  		/* Device-tree DMA controller registration */
>  		rc = of_dma_controller_register(pdev->dev.of_node,
>  				of_dma_simple_xlate, &omap_dma_info);
>  		if (rc) {
>  			pr_warn("OMAP-DMA: failed to register DMA controller\n");
>  			dma_async_device_unregister(&od->ddev);
>  			if (od->ll123_supported)
>  				dma_pool_destroy(od->desc_pool);
>  			omap_dma_free(od);
>  		}

[Severity: High]
This is a pre-existing issue, but is there a missing return statement in
the of_dma_controller_register() error path above?

If of_dma_controller_register() fails, the code handles the error by logging
a warning and calling omap_dma_free(od) but fails to immediately return 'rc'.

Execution falls through, eventually calling cpu_pm_register_notifier(&od->nb)
and then returning the error code 'rc'.

Because the probe function returns an error, the driver core will free 'od'
via devres. However, the CPU PM notifier remains globally registered. When a
CPU PM event occurs, could the kernel invoke the notifier callback with the
freed 'od' pointer, resulting in a use-after-free?

Additionally, does the unregistration logic in omap_dma_remove() properly
clean this up? It appears to only unregister if 'may_lose_context' is true,
but probe registers it if 'needs_busy_check' is true as well.

>  	}
>  
>  	omap_dma_init_gcr(od, DMA_DEFAULT_ARB_RATE, DMA_DEFAULT_FIFO_DEPTH, 0);
>  
>  	if (od->cfg->needs_busy_check) {
>  		od->nb.notifier_call = omap_dma_busy_notifier;
>  		cpu_pm_register_notifier(&od->nb);

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