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

[sashiko-bot@kernel.org]

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