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

[sashiko-bot@kernel.org]

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