Re: [PATCHv2 5/9] dmaengine: mv_xor: use devm_clk_get_optional_enabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing tasklet_kill during channel teardown leads to Use-After-Free.
- [High] Missing .remove callback without suppress_bind_attrs leads to sysfs unbind Use-After-Free.
--

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

dmaengine: mv_xor: use devm_clk_get_optional_enabled

Replace clk_get() + clk_prepare_enable() + clk_put() with
devm_clk_get_optional_enabled(). This eliminates the need for manual clock
cleanup in the probe error path.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..a97fa0038652d 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1359,12 +1359,9 @@ static int mv_xor_probe(struct platform_device *pdev)
>  			mv_xor_conf_mbus_windows(xordev, dram);
>  	}
>  
> -	/* Not all platforms can gate the clock, so it is not
> -	 * an error if the clock does not exists.
> -	 */
> -	xordev->clk = clk_get(&pdev->dev, NULL);
> -	if (!IS_ERR(xordev->clk))
> -		clk_prepare_enable(xordev->clk);
> +	xordev->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);

[Severity: High]
This is a pre-existing issue, but does moving to a devres-managed clock
further expose a missing .remove callback?

The platform driver mv_xor_driver lacks a .remove callback and does not set
suppress_bind_attrs = true. This allows the driver to be unbound via sysfs.

Because there is no .remove callback, the driver core performs no teardown
on unbind, leaving DMA channels registered and the interrupt active since
request_irq() is used rather than devm_request_irq().

Devres will then automatically free the mv_chan memory, unmap IO memory,
and (with this patch) disable the clock. Any subsequent interrupt or DMA
request will access the freed mv_chan memory and unmapped registers,
causing a use-after-free and system panic.

> +	if (IS_ERR(xordev->clk))
> +		return PTR_ERR(xordev->clk);
>  
>  	/*
>  	 * We don't want to have more than one channel per CPU in

[ ... ]

> @@ -1452,11 +1449,6 @@ static int mv_xor_probe(struct platform_device *pdev)
>  				irq_dispose_mapping(xordev->channels[i]->irq);
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but could a pending tasklet run
after devres frees the channel memory during probe failure?

In mv_xor_channel_add() in drivers/dma/mv_xor.c, an IRQ tasklet is
initialized. If probe fails (for instance, if a self-test times out while
an interrupt fires concurrently), the error path here calls
mv_xor_channel_remove(), which uses free_irq() to disable the interrupt.

However, neither mv_xor_channel_remove() nor the err_free_irq path in
mv_xor_channel_add() calls tasklet_kill(). Because mv_chan is allocated
via devm_kzalloc(), devres will free the memory shortly after probe returns.

If the scheduled tasklet executes after the memory is freed, it will
dereference mv_chan, leading to a use-after-free.

> -	if (!IS_ERR(xordev->clk)) {
> -		clk_disable_unprepare(xordev->clk);
> -		clk_put(xordev->clk);
> -	}
> -
>  	return ret;
>  }

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