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

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

[Rosen Penev]

We need to tear down the irq before edac_mc_free() to avoid a use after
free on teardown as request_irq uses mci as dev_id.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/fsl_ddr_edac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index f4cc89b26954..f1ddd1f47f2a 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -601,7 +601,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);
@@ -639,6 +639,7 @@ void fsl_mc_err_remove(struct platform_device *op)
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
 		ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
+		free_irq(pdata->irq, mci);
 	}
 
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
-- 
2.55.0

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

[sashiko-bot]

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 <rosenp@gmail.com>

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/20260729012320.616174-1-rosenp@gmail.com?part=1