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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Type mismatch and bypassed bounds check for `lch_count`.
- [Medium] Undocumented behavioral changes and bug fixes silently included in the patch.

Pre-existing issues:
- [High] Teardown race condition and unconditional devres WARN_ON in remove().
--

commit 274cec0b6701807c53603cef31b387fd271e1ddd
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). The probe is reordered so platform_data lookup
and the lch_count determination happen before the parent allocation.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..0cccd66636284 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c
[ ... ]
> @@ -1656,36 +1656,55 @@ 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;
[ ... ]
> +	} 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);

[Severity: Medium]
Is it possible for a malformed Device Tree to bypass this bounds check?

The variable lch_count is declared as a signed int but its address is passed
to of_property_read_u32().

If a Device Tree provides a dma-channels value of 0x80000000 or greater,
of_property_read_u32() writes it as a negative number. This would bypass the
bounds check above since it performs a signed comparison.

Consequently, struct_size() would implicitly cast the negative value to a
massive size_t value, resulting in an integer overflow and an unintended
-ENOMEM allocation failure rather than gracefully rejecting the invalid DT
property with -EINVAL.

Could lch_count be declared as a u32 to avoid this and the -Wpointer-sign
warning?

[ ... ]
> @@ -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: Medium]
This isn't a bug, but should this fix be documented in the commit message or
separated into a standalone commit?

This silently fixes a missing return statement on the error path, which is
unrelated to the flexible array conversion described in the commit message.

[ ... ]
> @@ -1852,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);

[Severity: Medium]
This isn't a bug, but should this fix also be split out or documented?

Checking od->cfg->needs_busy_check here patches a PM notifier resource leak,
which seems unrelated to the flexible array conversion.

[Severity: High]
This is a pre-existing issue, but does omap_dma_remove() have a teardown race
condition?

Looking at the rest of omap_dma_remove():

drivers/dma/ti/omap-dma.c:omap_dma_remove() {
        ...
        irq = platform_get_irq(pdev, 1);
        devm_free_irq(&pdev->dev, irq, od);

        dma_async_device_unregister(&od->ddev);
        ...
}

The function explicitly frees the IRQ handler via devm_free_irq() before
calling dma_async_device_unregister().

If active DMA transfers complete after the IRQ is freed, they will trigger an
unhandled interrupt. Since the IRQ is shared, repeatedly raising unhandled
interrupts will cause the core IRQ subsystem to permanently disable the
shared IRQ line.

Also, for legacy devices where irq <= 0, the driver skips requesting the IRQ
during probe but still unconditionally passes the invalid/negative IRQ to
devm_free_irq() during remove. Does this cause devres to fail its resource
lookup and trigger a warning stack trace?

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