Re: [PATCH] EDAC/fsl_ddr: use non devm for request_irq()

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

Pre-existing issues:
- [High] Hardware state leak on error path in `fsl_mc_err_probe` can lead to an interrupt storm.
--

--- Patch [1]: [PATCH] EDAC/fsl_ddr: use non devm for request_irq() ---
commit 8909bee5f3be8d2142784ef41571cbe3330c5c9a
Author: Rosen Penev <[email protected]>

EDAC/fsl_ddr: use non devm for request_irq()

This commit changes the driver to use request_irq() instead of
devm_request_irq() so that the interrupt can be manually torn down before
edac_mc_free(), avoiding a use-after-free on teardown.

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index e4eaec0aa81d5..878d4a86a3c9c 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
[ ... ]
> @@ -623,7 +623,7 @@ int fsl_mc_err_probe(struct platform_device *op)
>  
>  		/* register interrupts */
>  		pdata->irq = platform_get_irq(op, 0);
> -		res = devm_request_irq(&op->dev, pdata->irq,
> +		res = request_irq(pdata->irq,
>  				       fsl_mc_isr,
>  				       IRQF_SHARED,
>  				       "[EDAC] MC err", mci);
>  		if (res < 0) {
>  			pr_err("%s: Unable to request irq %d for FSL DDR DRAM ERR\n",
>  			       __func__, pdata->irq);
>  			res = -ENODEV;
>  			goto err2;

[Severity: High]
This is a pre-existing issue, but is it possible for this error path to leave
interrupts enabled in the hardware and cause an interrupt storm?

Earlier in fsl_mc_err_probe(), the hardware state is modified by clearing
FSL_MC_ERR_DISABLE and enabling interrupts in FSL_MC_ERR_INT_EN. If
request_irq() fails here, the goto err2 path cleans up the software state with
edac_mc_del_mc() and edac_mc_free(), but it does not appear to revert these
hardware registers.

Since the interrupt is requested with IRQF_SHARED, an unhandled interrupt
storm from this device could cause the kernel IRQ core to permanently
disable the shared IRQ line, potentially creating a denial of service for
other devices sharing it. 

This path could easily be hit if platform_get_irq() returns -EPROBE_DEFER,
since its return value is not explicitly checked for errors before being
passed directly to request_irq().

Should the err2 path restore FSL_MC_ERR_DISABLE and disable FSL_MC_ERR_INT_EN
before returning?

[ ... ]

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