Re: [PATCH v2 5/5] dmaengine: ti: omap-dma: use devm for dmaengine registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Deferring `dma_async_device_unregister()` to devres causes channel unregistration to be bypassed, leading to massive memory/IDA leaks and use-after-free vulnerabilities via sysfs.
--

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

dmaengine: ti: omap-dma: use devm for dmaengine registration

This commit switches to dmaenginem_async_device_register() to handle
unregistration automatically via devres. It removes the explicit
dma_async_device_unregister() calls from the probe error path and remove
function.

>     - The explicit dma_async_device_unregister() in the
>       of_dma_controller_register error path (devres unwind runs it
>       after omap_dma_free() has emptied the channels list, making the
>       iteration a safe no-op).

[Severity: High]
Is it actually safe for the channels list to be emptied before
dma_async_device_unregister() runs?

The commit message mentions this is a safe no-op, but looking at
omap_dma_free(), it empties the channel list and frees the structs:

drivers/dma/ti/omap-dma.c:omap_dma_free() {
    ...
    while (!list_empty(&od->ddev.channels)) {
        c = list_first_entry(&od->ddev.channels, ...);
        list_del(&c->vc.chan.device_node);
        ...
        kfree(c);
    }
}

Later, when the devres-managed dma_async_device_unregister() executes, it
iterates over the now-empty list:

drivers/dma/dmaengine.c:dma_async_device_unregister() {
    ...
    list_for_each_entry_safe(chan, n, &device->channels, device_node)
        __dma_async_device_channel_unregister(device, chan);
    ...
}

Since the core unregistration iteration is bypassed, won't this silently
skip channel teardown? It looks like this would leak IDAs and leave active
sysfs nodes with dangling pointers to the freed channel memory. If userspace
accesses these sysfs attributes later, could it lead to a use-after-free?

> diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
> index 8c32b7ab50f63..4bf34569d82bb 100644
> --- a/drivers/dma/ti/omap-dma.c
> +++ b/drivers/dma/ti/omap-dma.c

[ ... ]

> @@ -1878,8 +1876,6 @@ static void omap_dma_remove(struct platform_device *pdev)
>  	if (pdev->dev.of_node)
>  		of_dma_controller_free(pdev->dev.of_node);
>  
> -	dma_async_device_unregister(&od->ddev);
> -
>  	if (!omap_dma_legacy(od)) {
>  		spin_lock_irq(&od->irq_lock);
>  		od->irq_enable_mask = 0;

[ ... ]

> @@ -1888,10 +1884,6 @@ static void omap_dma_remove(struct platform_device *pdev)
>  		omap_dma_glbl_read(od, IRQENABLE_L1);
>  	}
>  
> -	irq = platform_get_irq(pdev, 1);
> -	if (irq > 0)
> -		devm_free_irq(&pdev->dev, irq, od);
> -
>  	omap_dma_free(od);
>  
>  	if (od->ll123_supported)

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