Re: [PATCH v2 04/13] dmaengine: ste_dma40: Fix DMA registration unwind

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

Pre-existing issues:
- [High] The DMA_CYCLIC capability is incorrectly applied to dma_slave instead of dma_both during dma_both setup.
- [High] The channel tasklets are not explicitly killed on the probe error path, which can result in a Use-After-Free if a tasklet is scheduled before the probe fails.
--

--- Patch [4]: [PATCH v2 04/13] dmaengine: ste_dma40: Fix DMA registration unwind ---
commit b477f2730e0cab6c69890c774c76310e0edd988b
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix DMA registration unwind

d40_dmaengine_init() registers DMAengine devices using devres-managed
unregister actions. This commit wraps the registration in a temporary
devres group, releasing it if probe fails to prevent DMAengine devices
from staying visible until devres unwinds.

Closes: https://lore.kernel.org/dmaengine/[email protected]/

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 145f7105218ce..384c2f3805b02 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[ ... ]
> @@ -3638,20 +3639,29 @@ static int __init d40_probe(struct platform_device *pdev)
>  
>  	d40_hw_init(base);
>  
> +	dmaenginem_reg_group = devres_open_group(dev, NULL, GFP_KERNEL);
> +	if (!dmaenginem_reg_group) {
> +		ret = -ENOMEM;
> +		goto destroy_cache;
> +	}
> +
>  	ret = d40_dmaengine_init(base, num_reserved_chans);

[Severity: High]
This isn't a bug introduced by this patch, but does the configuration of
base->dma_both in d40_dmaengine_init() incorrectly set the DMA_CYCLIC
capability on base->dma_slave instead of base->dma_both?

Looking at d40_dmaengine_init():

drivers/dma/ste_dma40.c:d40_dmaengine_init() {
    ...
    dma_cap_zero(base->dma_both.cap_mask);
    dma_cap_set(DMA_SLAVE, base->dma_both.cap_mask);
    dma_cap_set(DMA_MEMCPY, base->dma_both.cap_mask);
    dma_cap_set(DMA_CYCLIC, base->dma_slave.cap_mask);
    ...
}

Should the last line be setting DMA_CYCLIC on base->dma_both.cap_mask so that
physical channels managed by dma_both advertise cyclic support?

>  	if (ret)
> -		goto destroy_cache;
> +		goto release_dmaenginem;
>  
>  	ret = of_dma_controller_register(np, d40_xlate, NULL);
>  	if (ret) {
>  		dev_err(dev,
>  			"could not register of_dma_controller\n");
> -		goto destroy_cache;
> +		goto release_dmaenginem;
>  	}
> +	devres_remove_group(dev, dmaenginem_reg_group);
>  
>  	dev_info(base->dev, "initialized\n");
>  	return 0;
>  
> + release_dmaenginem:
> +	devres_release_group(dev, dmaenginem_reg_group);

[Severity: High]
This is a pre-existing issue, but does this error path need to explicitly
kill the channel tasklets initialized during probe?

During d40_probe(), tasklets are initialized for all channels in
d40_chan_init(). Then d40_dmaengine_init() exposes the DMA devices via
dmaenginem_async_device_register().

At this point, a client could request a channel and submit a transfer,
causing a hardware interrupt to schedule the tasklet. If probe subsequently
fails here at of_dma_controller_register(), the driver unwinds and returns
an error, causing devm to free the base structure containing the tasklets.

Since tasklet_kill() is never called in the error path, could a scheduled
tasklet execute after the memory is freed, leading to a Use-After-Free crash?

>   destroy_cache:
>  	if (base->lcla_pool.dma_addr)
>  		dma_unmap_single(base->dev, base->lcla_pool.dma_addr,

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