Re: [PATCHv3 1/8] dmaengine: ti: omap-dma: fix missing return in probe error path

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message factually misstates the behavior of the bug, claiming the probe function 'eventually succeeds' without the return statement.

Pre-existing issues:
- [High] The CPU PM notifier is registered during probe if `needs_busy_check` is true, but `omap_dma_remove` only unregisters it if `may_lose_context` is true, causing a resource leak and potential use-after-free.
- [High] `platform_get_irq` returning an error (e.g., `-EPROBE_DEFER`) on non-OMAP1 platforms incorrectly allows the driver to probe without an interrupt handler.
- [Medium] Unconditional call to `devm_free_irq` in `omap_dma_remove` can use an invalid or unrequested IRQ if `platform_get_irq` returned an error during probe.
--

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

dmaengine: ti: omap-dma: fix missing return in probe error path

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>

> 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;.

[Severity: Medium]
Is this description accurate?

The variable rc retains the non-zero error code returned by
of_dma_controller_register(). Since there are no further assignments to rc in
the remainder of omap_dma_probe(), the function ultimately reaches return rc;
and fails. 

The actual impact of the missing return was that the driver erroneously executed
subsequent initialization (like registering the CPU PM notifier) and then
failed.

Since the failure path after that point does not unregister the notifier, this
left a dangling pointer and caused an immediate use-after-free when the device
structure was freed by devres.

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 55ece7fd0d99a..0f6dd6b0a301f 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 is there a missing unregister
for the CPU PM notifier in omap_dma_remove()?

In omap_dma_probe(), the notifier is registered if either needs_busy_check or
may_lose_context is true:

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
    ...
	if (od->cfg->needs_busy_check) {
		od->nb.notifier_call = omap_dma_busy_notifier;
		cpu_pm_register_notifier(&od->nb);
	} else if (od->cfg->may_lose_context) {
		od->nb.notifier_call = omap_dma_context_notifier;
		cpu_pm_register_notifier(&od->nb);
	}
    ...
}

However, omap_dma_remove() strictly guards the unregistration with
may_lose_context:

drivers/dma/ti/omap-dma.c:omap_dma_remove() {
    ...
	if (od->cfg->may_lose_context)
		cpu_pm_unregister_notifier(&od->nb);
    ...
}

For SoC configurations where needs_busy_check is true but may_lose_context is
false, the notifier is never unregistered upon module removal, leading to a
use-after-free panic upon the next PM transition when the CPU PM subsystem
calls the freed notifier.

[Severity: High]
This is a pre-existing issue, but does omap_dma_probe() incorrectly ignore
errors from platform_get_irq() on non-OMAP1 platforms?

drivers/dma/ti/omap-dma.c:omap_dma_probe() {
    ...
	irq = platform_get_irq(pdev, 1);
	if (irq <= 0) {
		dev_info(&pdev->dev, "failed to get L1 IRQ: %d\n", irq);
		od->legacy = true;
	} else {
    ...
}

If platform_get_irq() returns an error like -EPROBE_DEFER, the driver sets
legacy mode and continues without an IRQ.

On non-OMAP1 platforms, the legacy macro evaluates to false, causing the driver
to operate as a modern DMA controller configuring interrupts, but without an
IRQ handler registered.

This results in silent probe success followed by permanently hung DMA transfers.

[Severity: Medium]
This is a pre-existing issue, but can omap_dma_remove() pass an invalid IRQ
number to devm_free_irq()?

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

If platform_get_irq() returned an error during probe, or returns an error here
in remove, devm_free_irq() is called with an invalid IRQ.

This causes a warning in devres_release() since the IRQ was never requested via
devres, followed by calling free_irq() with an invalid IRQ number.

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