[PATCH 0/2] EDAC: fsl_ddr: probe failure and remove lifetime fixes

[PATCH 0/2] EDAC: fsl_ddr: probe failure and remove lifetime fixes

[Rosen Penev]

This series fixes two lifetime/error-path issues in the FSL DDR EDAC
driver (drivers/edac/fsl_ddr_edac.c):

* Patch 1: On probe failure, fsl_mc_err_probe() leaves the memory
  controller hardware configured to generate interrupts (DISABLE cleared,
  INT_EN set) while no handler is registered, which can trigger an
  unhandled interrupt storm on the shared IRQ line. Restore the MC error
  registers in both error paths.

* Patch 2: The devm-requested IRQ passes mci as context, but
  fsl_mc_err_remove() frees mci via edac_mc_free() before devres tears
  down the IRQ, leaving a use-after-free window for the shared interrupt
  handler. Manage mci lifetime through devres so it is freed only after
  the IRQ is released.

Both patches are build-tested for arm64 (defconfig + CONFIG_EDAC_FSL_DDR)
with LLVM=1.

Rosen Penev (2):
  EDAC: fsl_ddr: restore MC error registers on probe failure
  EDAC: fsl_ddr: manage mci lifetime via devres to fix remove UAF

 drivers/edac/fsl_ddr_edac.c | 42 +++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 16 deletions(-)

-- 
2.55.0

[PATCH 1/2] EDAC: fsl_ddr: restore MC error registers on probe failure

[Rosen Penev]

fsl_mc_err_probe() clears FSL_MC_ERR_DISABLE and, in EDAC_OPSTATE_INT
mode, sets FSL_MC_ERR_INT_EN to enable error reporting. If a later
initialization step fails and jumps to the err/err2 labels, the cleanup
only frees driver memory and does not restore these hardware registers.

This leaves the memory controller configured to generate interrupts while
no handler is successfully registered, which can cause an unhandled
interrupt storm on the shared IRQ line. Restore FSL_MC_ERR_DISABLE,
FSL_MC_ERR_INT_EN and the SBE threshold in both error paths.

Initialize the orig_ddr_err_disable/orig_ddr_err_sbe save fields up front
so the error paths are safe even when they are reached before the INT
block populates them.

Built for arm64 (defconfig + CONFIG_EDAC_FSL_DDR) with LLVM=1;
drivers/edac/fsl_ddr_edac.o compiles cleanly.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/fsl_ddr_edac.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index b27dff96aeb6..b1e6e177b088 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -518,6 +518,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 
 	pdata = mci->pvt_info;
 	pdata->name = "fsl_mc_err";
+	pdata->orig_ddr_err_disable = 0;
+	pdata->orig_ddr_err_sbe = 0;
 	mci->pdev = &op->dev;
 	pdata->edac_idx = edac_mc_idx++;
 	dev_set_drvdata(mci->pdev, mci);
@@ -645,7 +647,11 @@ int fsl_mc_err_probe(struct platform_device *op)
 
 err2:
 	edac_mc_del_mc(&op->dev);
+	ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
 err:
+	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
+		  pdata->orig_ddr_err_disable);
+	ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);
 	devres_release_group(&op->dev, fsl_mc_err_probe);
 	edac_mc_free(mci);
 	return res;
-- 
2.55.0

Re: [PATCH 1/2] EDAC: fsl_ddr: restore MC error registers on probe failure

[sashiko-bot]

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

New issues:
- [High] Calling `ddr_out32()` in the `err` cleanup path unconditionally dereferences unmapped I/O memory pointers if probe fails early, leading to a kernel crash.
- [High] Pre-initializing `orig_ddr_err_disable` and `orig_ddr_err_sbe` to 0 causes the error path to blindly overwrite hardware registers with 0 instead of their true original states on intermediate probe failures.

Pre-existing issues:
- [High] The EDAC MC object (`mci`) is freed before the shared interrupt handler is unregistered during module removal, creating a use-after-free vulnerability.
--

--- Patch [1]: [PATCH 1/2] EDAC: fsl_ddr: restore MC error registers on probe failure ---
commit 987a0eb5943a39fdf0201244bbdd7e9cdf6c562e
Author: Rosen Penev <rosenp@gmail.com>

EDAC: fsl_ddr: restore MC error registers on probe failure

fsl_mc_err_probe() clears FSL_MC_ERR_DISABLE and, in EDAC_OPSTATE_INT
mode, sets FSL_MC_ERR_INT_EN to enable error reporting. If a later
initialization step fails and jumps to the err/err2 labels, the cleanup
only frees driver memory and does not restore these hardware registers.

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index e4eaec0aa81d5..7fb789e97074b 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -519,6 +519,8 @@ int fsl_mc_err_probe(struct platform_device *op)
>  
>  	pdata = mci->pvt_info;
>  	pdata->name = "fsl_mc_err";
> +	pdata->orig_ddr_err_disable = 0;
> +	pdata->orig_ddr_err_sbe = 0;

[Severity: High]
Does pre-initializing these to 0 cause the error path to overwrite
hardware registers incorrectly?

If probe fails after I/O mapping but before the original hardware states are
read (for example, if the ECC verification check fails), the code jumps to the
err: block. 

Because orig_ddr_err_disable and orig_ddr_err_sbe are pre-initialized to 0,
the driver will write 0 back to the hardware registers rather than restoring
the bootloader's true configuration, which clears all disable bits and
forcibly enables all memory controller errors.

>  	mci->pdev = &op->dev;
>  	pdata->edac_idx = edac_mc_idx++;
>  	dev_set_drvdata(mci->pdev, mci);

[ ... ]

> @@ -646,7 +648,11 @@ int fsl_mc_err_probe(struct platform_device *op)
>  
>  err2:
>  	edac_mc_del_mc(&op->dev);
> +	ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
>  err:
> +	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
> +		  pdata->orig_ddr_err_disable);
> +	ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);

[Severity: High]
Can this unconditionally dereference unmapped I/O memory pointers if the
probe fails early?

If early initialization steps such as of_address_to_resource(),
devm_request_mem_region(), or devm_ioremap() fail in fsl_mc_err_probe(), the
code jumps to the err: label. At this point, pdata->mc_vbase is still NULL.

The cleanup path added here unconditionally calls ddr_out32(), which executes
ddr_reg_addr() to calculate the register address. This would result in an
invalid low memory address (NULL + off) being passed to iowrite32(),
triggering a fatal Data Abort.

>  	devres_release_group(&op->dev, fsl_mc_err_probe);
>  	edac_mc_free(mci);
>  	return res;


[Severity: High]
This is a pre-existing issue, but does fsl_mc_err_remove() have a
use-after-free vulnerability during module removal?

In fsl_mc_err_probe(), a shared interrupt is requested via devm_request_irq()
using mci as the dev_id. However, during module removal, edac_mc_free(mci) is
called synchronously before devres cleans up the shared IRQ:

drivers/edac/fsl_ddr_edac.c:fsl_mc_err_remove() {
    ...
	edac_mc_del_mc(&op->dev);
	edac_mc_free(mci);
}

Because the IRQ remains registered until devres cleanup runs after
fsl_mc_err_remove() returns, an interrupt on the shared IRQ line during this
window would cause fsl_mc_isr() to execute and dereference the freed mci
pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719194009.117532-1-rosenp@gmail.com?part=1

[PATCH 2/2] EDAC: fsl_ddr: manage mci lifetime via devres to fix remove UAF

[Rosen Penev]

fsl_mc_err_probe() requests the shared interrupt with devm_request_irq(),
passing mci as the handler context. At removal fsl_mc_err_remove() calls
edac_mc_free(mci) explicitly, but the devm-requested IRQ is only torn
down by devres afterwards. Between edac_mc_free() and the IRQ release,
another device sharing the line can fire and fsl_mc_isr() will
dereference the freed mci and its pdata.

Register an edac_mc_free() callback via devm_add_action_or_reset() right
after edac_mc_alloc(), so mci is owned by devres and freed only after the
devm IRQ is released (devres runs actions in LIFO order). Drop the
explicit edac_mc_free() from both the probe error paths and
fsl_mc_err_remove(); edac_mc_del_mc() is still called explicitly at
remove time so the edac device is unregistered before the deferred free.

This also lets the now-redundant devres group open/release/remove calls
be removed.

Built for arm64 (defconfig + CONFIG_EDAC_FSL_DDR) with LLVM=1;
drivers/edac/fsl_ddr_edac.o compiles cleanly and passes checkpatch --strict.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/edac/fsl_ddr_edac.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index b1e6e177b088..cdd129bc42c7 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -490,6 +490,13 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 	}
 }
 
+static void fsl_mc_edac_free(void *data)
+{
+	struct mem_ctl_info *mci = data;
+
+	edac_mc_free(mci);
+}
+
 int fsl_mc_err_probe(struct platform_device *op)
 {
 	struct mem_ctl_info *mci;
@@ -500,9 +507,6 @@ int fsl_mc_err_probe(struct platform_device *op)
 	u32 sdram_ctl;
 	int res;
 
-	if (!devres_open_group(&op->dev, fsl_mc_err_probe, GFP_KERNEL))
-		return -ENOMEM;
-
 	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
 	layers[0].size = 4;
 	layers[0].is_virt_csrow = true;
@@ -511,10 +515,17 @@ int fsl_mc_err_probe(struct platform_device *op)
 	layers[1].is_virt_csrow = false;
 	mci = edac_mc_alloc(edac_mc_idx, ARRAY_SIZE(layers), layers,
 			    sizeof(*pdata));
-	if (!mci) {
-		devres_release_group(&op->dev, fsl_mc_err_probe);
+	if (!mci)
 		return -ENOMEM;
-	}
+
+	/*
+	 * Manage mci lifetime via devres so it is freed only after the
+	 * devm-requested IRQ is released, avoiding a use-after-free of mci
+	 * (and its pdata) in the shared interrupt handler during removal.
+	 */
+	res = devm_add_action_or_reset(&op->dev, fsl_mc_edac_free, mci);
+	if (res)
+		return res;
 
 	pdata = mci->pvt_info;
 	pdata->name = "fsl_mc_err";
@@ -558,10 +569,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 
 	if (pdata->flag == TYPE_IMX9) {
 		pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
-		if (IS_ERR(pdata->inject_vbase)) {
-			res = -ENOMEM;
-			goto err;
-		}
+		if (IS_ERR(pdata->inject_vbase))
+			return -ENOMEM;
 	}
 
 	if (pdata->flag == TYPE_IMX9) {
@@ -575,8 +584,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 	if ((sdram_ctl & ecc_en_mask) != ecc_en_mask) {
 		/* no ECC */
 		pr_warn("%s: No ECC DIMMs discovered\n", __func__);
-		res = -ENODEV;
-		goto err;
+		return -ENODEV;
 	}
 
 	edac_dbg(3, "init mci\n");
@@ -639,7 +647,6 @@ int fsl_mc_err_probe(struct platform_device *op)
 		       pdata->irq);
 	}
 
-	devres_remove_group(&op->dev, fsl_mc_err_probe);
 	edac_dbg(3, "success\n");
 	pr_info(EDAC_MOD_STR " MC err registered\n");
 
@@ -652,8 +659,6 @@ int fsl_mc_err_probe(struct platform_device *op)
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
 		  pdata->orig_ddr_err_disable);
 	ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);
-	devres_release_group(&op->dev, fsl_mc_err_probe);
-	edac_mc_free(mci);
 	return res;
 }
 
@@ -674,5 +679,4 @@ void fsl_mc_err_remove(struct platform_device *op)
 
 
 	edac_mc_del_mc(&op->dev);
-	edac_mc_free(mci);
 }
-- 
2.55.0

Re: [PATCH 2/2] EDAC: fsl_ddr: manage mci lifetime via devres to fix remove UAF

[sashiko-bot]

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

New issues:
- [High] NULL Pointer Dereference / Kernel Panic in probe error path.

Pre-existing issues:
- [High] Use-After-Free during driver removal due to shared IRQ line.
--

--- Patch [2]: [PATCH 2/2] EDAC: fsl_ddr: manage mci lifetime via devres to fix remove UAF ---
commit 15d5e252fe89541dc42e690006d7c3334debe7af
Author: Rosen Penev <rosenp@gmail.com>

EDAC: fsl_ddr: manage mci lifetime via devres to fix remove UAF

fsl_mc_err_probe() requests the shared interrupt with devm_request_irq(),
passing mci as the handler context. At removal fsl_mc_err_remove() calls
edac_mc_free(mci) explicitly, but the devm-requested IRQ is only torn
down by devres afterwards. Between edac_mc_free() and the IRQ release,
another device sharing the line can fire and fsl_mc_isr() will
dereference the freed mci and its pdata.

Register an edac_mc_free() callback via devm_add_action_or_reset() right
after edac_mc_alloc(), so mci is owned by devres and freed only after the
devm IRQ is released (devres runs actions in LIFO order).

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 7fb789e97074b..5eec7fe17adda 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c

[ ... ]

> @@ -653,8 +660,6 @@ int fsl_mc_err_probe(struct platform_device *op)
>  	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
>  		  pdata->orig_ddr_err_disable);

[Severity: High]
Could this error path trigger a kernel panic if initialization fails early?

If early initialization fails (like of_address_to_resource,
devm_request_mem_region, or devm_ioremap), the code jumps to the err label
while pdata->mc_vbase is still NULL.

ddr_out32() then calls ddr_reg_addr():

drivers/edac/fsl_ddr_edac.c:ddr_reg_addr() {
    ...
    return pdata->mc_vbase + off;
}

This will attempt to write to an unmapped low address. This regression was
introduced by the immediately preceding patch in this series (EDAC: fsl_ddr:
restore MC error registers on probe failure) and appears to remain unfixed
here.

>  	ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);
> -	devres_release_group(&op->dev, fsl_mc_err_probe);
> -	edac_mc_free(mci);
>  	return res;
>  }
>  
> @@ -675,5 +680,4 @@ void fsl_mc_err_remove(struct platform_device *op)
>  
>  
>  	edac_mc_del_mc(&op->dev);
> -	edac_mc_free(mci);
>  }

[Severity: High]
This is a pre-existing issue, but does mpc85xx_edac.c have the identical
shared IRQ use-after-free during driver removal?

This patch fixes the issue in fsl_ddr_edac.c, but looking at
mpc85xx_l2_err_probe() and mpc85xx_pci_err_probe(), they also request a
shared IRQ handler via devm_request_irq() with IRQF_SHARED.

During remove(), the EDAC device structures are freed explicitly:

drivers/edac/mpc85xx_edac.c:mpc85xx_l2_err_remove() {
    ...
    edac_device_free_ctl_info(edac_dev);
}

drivers/edac/mpc85xx_edac.c:mpc85xx_pci_err_remove() {
    ...
    edac_pci_free_ctl_info(pci);
}

However, the devres-managed IRQ is not released until after remove() returns.
If an interrupt fires on the shared line in this window, the ISR executes
and dereferences the already freed EDAC structure.

Could a similar devres-managed lifetime fix be applied to these sister
drivers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719194009.117532-1-rosenp@gmail.com?part=2