[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: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

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

[Rosen Penev]

On Fri, Jul 31, 2026 at 8:12 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> 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.
That's probably appropriate.
>
> Frank
>
> > >
> > > --
> > > 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()

[Rosen Penev]

On Fri, Jul 31, 2026 at 7:53 AM Frank Li <Frank.li@oss.nxp.com> wrote:
>
> 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.
Needing to explicitly call devm_free_irq kind of defeats the purpose of devm.

Explicit ordering in this case makes more sense to me, especially with
the ddr_out32 calls.

There's actually a second benefit to the non devm version. The devm
version prints an error message on failure. The normal one does not.
The significance is that the driver prints its own message.
>
> Frank
>
> > >
> > > Frank
> > >
> > > >       }
> > > >
> > > >       ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> > > > --
> > > > 2.55.0
> > > >
> > > >