Re: [PATCH v2] dmaengine: fsl-edma: Add error handling for devm_kasprintf

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper handling of IRQ API error codes breaks probe deferral.
- [High] Double disable of `muxclk` clocks due to explicit disable of devres-managed clocks upon probe failure.
- [High] Double disable of channel clocks on probe error and device removal.
- [High] Use-After-Free of power domain device pointer during probe failure or device removal.
--

--- Patch [1]: [PATCH v2] dmaengine: fsl-edma: Add error handling for devm_kasprintf ---
commit a6d0ef2d595b7ee7a27a92eb52a981ce05946b5f
Author: Griffin Kroah-Hartman <griffin@kroah.com>

dmaengine: fsl-edma: Add error handling for devm_kasprintf

Add error handling statement to fsl_edma3_irq_init() for the
devm_kasprintf() call.

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index 36155ab1602a9..d9fb717b5b53c 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
> @@ -414,6 +414,8 @@ static int fsl_edma3_irq_init(struct platform_device *pdev, struct fsl_edma_engi
>  
>  		errirq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-err",
>  					     dev_name(&pdev->dev));
> +		if (!errirq_name)
> +			return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but returning -ENOMEM here makes a
pre-existing use-after-free in fsl_edma3_detach_pd() more likely to be
triggered during probe failure.

When detaching a power domain, dev_pm_domain_detach() unregisters and frees
the virtual device. Is it safe to pass the freed pd_dev pointer to
pm_runtime_dont_use_autosuspend() and pm_runtime_set_suspended() immediately
after?

drivers/dma/fsl-edma-main.c:fsl_edma3_detach_pd() {
    ...
	if (fsl_chan->pd_dev) {
		dev_pm_domain_detach(fsl_chan->pd_dev, false);
		pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev);
		pm_runtime_set_suspended(fsl_chan->pd_dev);
	}
    ...
}

[Severity: High]
This is a pre-existing issue, but returning -ENOMEM here will also trigger a
pre-existing double disable of channel clocks during probe error handling.

Channel clocks are acquired via devm_clk_get_enabled(), but fsl_edma_probe()
explicitly disables them. Could devres execute clk_disable_unprepare()
again on these already disabled clocks when handling the error?

drivers/dma/fsl-edma-main.c:fsl_edma_probe() {
    ...
	if (fsl_chan->edma->drvdata->flags & FSL_EDMA_DRV_HAS_CHCLK)
		clk_disable_unprepare(fsl_chan->clk);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but looking earlier in
fsl_edma3_irq_init() and fsl_edma2_irq_init(), the error code from
platform_get_irq() is discarded.

If platform_get_irq() returns -EPROBE_DEFER, returning -EINVAL or -ENXIO
instead of propagating the error code will break deferred probing. Would it
be better to return the actual error code instead?

drivers/dma/fsl-edma-main.c:fsl_edma3_irq_init() {
    ...
		fsl_chan->txirq = platform_get_irq(pdev, i);
		if (fsl_chan->txirq < 0)
			return  -EINVAL;
    ...
}

drivers/dma/fsl-edma-main.c:fsl_edma2_irq_init() {
    ...
		irq = platform_get_irq(pdev, i);
		ret = 0;
		if (irq < 0)
			return -ENXIO;
    ...
}

[Severity: High]
This is a pre-existing issue, but while looking at fsl_edma_probe(), there
appears to be another double disable when devm_platform_ioremap_resource()
fails.

If resource allocation fails, fsl_disable_clocks() is called manually, which
disables muxclk. Since muxclk was acquired with devm_clk_get_enabled(),
won't devres double disable it?

drivers/dma/fsl-edma-main.c:fsl_edma_probe() {
    ...
		if (IS_ERR(fsl_edma->muxbase[i])) {
			/* on error: disable all previously enabled clks */
			fsl_disable_clocks(fsl_edma, i);
			return PTR_ERR(fsl_edma->muxbase[i]);
		}
    ...
}

>  
>  		ret = devm_request_irq(&pdev->dev, fsl_edma->errirq, fsl_edma3_err_handler_shared,
>  				       0, errirq_name, fsl_edma);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708075736.47822-1-griffin@kroah.com?part=1