Re: [PATCHv2] dmaengine: at_hdmac: use more devm in _probe

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

Pre-existing issues:
- [High] Missing tasklet_kill() for vchan tasklets during device removal leads to a potential use-after-free.
- [High] The driver does not implement the device_synchronize callback required for vchan.
--

--- Patch [1]: [PATCHv2] dmaengine: at_hdmac: use more devm in _probe ---
commit 4a21134962b742ca7876c61c580dce0a0982f3d7
Author: Rosen Penev <[email protected]>

dmaengine: at_hdmac: use more devm in _probe

As devm is already used in _probe, complete the conversion so that everything
unwinds in proper order.

> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
> index e5b30a57c477a..f16cf29ca5487 100644
> --- a/drivers/dma/at_hdmac.c
> +++ b/drivers/dma/at_hdmac.c
[ ... ]
> @@ -2080,10 +2074,10 @@ static int __init at_dma_probe(struct platform_device *pdev)
>  	  dma_has_cap(DMA_SLAVE, atdma->dma_device.cap_mask)  ? "slave " : "",
>  	  plat_dat->nr_channels);
>  
> -	err = dma_async_device_register(&atdma->dma_device);
> +	err = dmaenginem_async_device_register(&atdma->dma_device);

[Severity: High]
This is a pre-existing issue, but I noticed that atdma->dma_device does not
implement the device_synchronize callback.

Client drivers call dmaengine_synchronize() to ensure all transfers and their
callbacks have completed before freeing buffers. Because at_hdmac defers
descriptor completion to a tasklet via vchan but omits the device_synchronize
callback, dmaengine_synchronize() becomes a no-op.

Could this lead to client drivers freeing memory while the tasklet is still
actively running or queued, causing a use-after-free or data corruption?
Should device_synchronize be mapped to vchan_synchronize?

>  	if (err) {
>  		dev_err(&pdev->dev, "Unable to register: %d.\n", err);
> -		goto err_dma_async_device_register;
> +		return err;
>  	}
[ ... ]
> @@ -2121,22 +2103,12 @@ static void at_dma_remove(struct platform_device *pdev)
>  	struct dma_chan		*chan, *_chan;
>  
>  	at_dma_off(atdma);
> -	if (pdev->dev.of_node)
> -		of_dma_controller_free(pdev->dev.of_node);
> -	dma_async_device_unregister(&atdma->dma_device);
> -
> -	dma_pool_destroy(atdma->memset_pool);
> -	dma_pool_destroy(atdma->lli_pool);
> -	free_irq(platform_get_irq(pdev, 0), atdma);
>  
>  	list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
>  			device_node) {
>  		/* Disable interrupts */
>  		atc_disable_chan_irq(atdma, chan->chan_id);
> -		list_del(&chan->device_node);

[Severity: High]
This is a pre-existing issue, but it seems there is no tasklet_kill() called
for the vchan tasklets during device removal in at_dma_remove().

If a DMA channel tasklet is scheduled by an interrupt just before device
removal (for example, via sysfs unbind), at_dma_remove() might execute and
return without killing the vchan tasklets. Devres would then subsequently
destroy the lli_pool and free the at_dma structure.

If the pending softirq tasklet executes after this, could it access the freed
pool and cause a use-after-free?

>  	}
> -
> -	clk_disable_unprepare(atdma->clk);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1