[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

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

[Frank Li]

On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> 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);

here already disable hardware irq. Just call
synchronize_irq() to make sure pending irq handled.

Frank

>  	}
>
>  	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> --
> 2.55.0
>
>

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

[Rosen Penev]

On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > 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);
>
> here already disable hardware irq. Just call
> synchronize_irq() to make sure pending irq handled.
AI disagrees with this suggestion:

The reviewer's suggestion to just call synchronize_irq() instead of
free_irq() is problematic for two reasons:
1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
this device from asserting the IRQ line, but other devices sharing the
same IRQ line can still trigger it. Without free_irq, the handler
remains in the shared action list and can be called with a dangling
mci pointer after edac_mc_free() — the very UAF this patch aims to
fix.
2. free_irq already synchronizes: free_irq() internally calls
synchronize_irq() after removing the action from the descriptor. An
explicit synchronize_irq() before free_irq() would be redundant.
>
> Frank
>
> >       }
> >
> >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > --
> > 2.55.0
> >
> >

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

[Borislav Petkov]

On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > here already disable hardware irq. Just call
> > synchronize_irq() to make sure pending irq handled.
> AI disagrees with this suggestion:

How about you try to parse what AI says and verify it yourself?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Rosen Penev]

On Thu, Jul 30, 2026 at 5:37 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > here already disable hardware irq. Just call
> > > synchronize_irq() to make sure pending irq handled.
> > AI disagrees with this suggestion:
>
> How about you try to parse what AI says and verify it yourself?
Makes sense to me.

The alternative is to use devm_add_action_or_reset for edac_mc_free.
I'd rather not do that.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

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

[Frank Li]

On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> On Thu, Jul 30, 2026 at 11:49 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> >
> > On Tue, Jul 28, 2026 at 06:23:20PM -0700, Rosen Penev wrote:
> > > 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);
> >
> > here already disable hardware irq. Just call
> > synchronize_irq() to make sure pending irq handled.
> AI disagrees with this suggestion:
>
> The reviewer's suggestion to just call synchronize_irq() instead of
> free_irq() is problematic for two reasons:
> 1. Shared IRQ: The handler is registered with IRQF_SHARED. Disabling
> the DDR controller's local IRQ output (ddr_out32(..., 0)) prevents
> this device from asserting the IRQ line, but other devices sharing the
> same IRQ line can still trigger it. Without free_irq, the handler
> remains in the shared action list and can be called with a dangling
> mci pointer after edac_mc_free() — the very UAF this patch aims to
> fix.
> 2. free_irq already synchronizes: free_irq() internally calls
> synchronize_irq() after removing the action from the descriptor. An
> explicit synchronize_irq() before free_irq() would be redundant.

You post too quick, I have not seen this mail yet.

devm_free_irq() here. and add comment why need explicit call it.

Frank

> >
> > Frank
> >
> > >       }
> > >
> > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > --
> > > 2.55.0
> > >
> > >

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

[Frank Li]

On Thu, Jul 30, 2026 at 05:44:34PM -0700, Rosen Penev wrote:
> On Thu, Jul 30, 2026 at 5:37 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Thu, Jul 30, 2026 at 05:16:11PM -0700, Rosen Penev wrote:
> > > > here already disable hardware irq. Just call
> > > > synchronize_irq() to make sure pending irq handled.
> > > AI disagrees with this suggestion:
> >
> > How about you try to parse what AI says and verify it yourself?
> Makes sense to me.
>
> The alternative is to use devm_add_action_or_reset for edac_mc_free.
> I'd rather not do that.

This one should work, of add devm_edac_add*() version to simplufy probe and
beanfit other EDAC driver.

Frank

> >
> > --
> > Regards/Gruss,
> >     Boris.
> >
> > https://people.kernel.org/tglx/notes-about-netiquette