Re: [PATCH v4 2/3] EDAC/cadence: Add Cadence DDR EDAC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] edac: use-after-free of mci structure in interrupt handler
- [Medium] edac: incomplete memory size calculation misses discontiguous banks
--

commit a92ae5cd8100995af0607d832e0079266536f567
Author: Paul Louvel <paul.louvel@bootlin.com>

EDAC/cadence: Add Cadence DDR EDAC driver

Add the Cadence EDAC driver found on Renesas RZ/N1x SoC.
The memory controller supports ECC, software scrubbing, and SECDED.

> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c
> new file mode 100644
> index 0000000000000..3f27bbb94c53c
> --- /dev/null
> +++ b/drivers/edac/cadence_edac.c

[ ... ]

> +static int cdns_get_mem_sz(resource_size_t *mem_sz)
> +{
> +	struct device_node *np;
> +	struct resource res;
> +	int ret;
> +
> +	np = of_find_node_by_name(NULL, "memory");
> +	if (!np)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(np, 0, &res);

[Severity: Medium]
By hardcoding index 0 in of_address_to_resource(), does this calculation
only fetch the first contiguous memory region? On systems with multiple
memory nodes or discontiguous memory banks, it appears this will
under-report the total system memory capacity to the EDAC core. Could we
iterate over all available memory regions instead?

> +
> +	of_node_put(np);

[ ... ]

> +static int cdns_setup_irq(struct platform_device *pdev, struct mem_ctl_info *mci)
> +{
> +	int irq, ret;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "unable to get irq\n");
> +
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_mc_err_handler, 0, dev_name(&pdev->dev), mci);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "unable to request irq %d\n", irq);
> +
> +	return 0;
> +}

[ ... ]

> +static int cdns_setup_mc(struct platform_device *pdev, void __iomem *io_base)
> +{

[ ... ]

> +	ret = cdns_setup_irq(pdev, mci);
> +	if (ret)
> +		goto free_edac_mc;
> +
> +	ret = edac_mc_add_mc(mci);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "failed to add mc\n");
> +		goto free_edac_mc;
> +	}
> +
> +#ifdef CONFIG_EDAC_DEBUG
> +	cdns_setup_debugfs(mci);
> +#endif
> +
> +	edac_mc_chipset_printk(mci, KERN_DEBUG, DRV_NAME, "ECC %s\n",
> +			       (mci->edac_cap == EDAC_FLAG_NONE) ? "disabled" : "enabled");
> +
> +	return 0;
> +
> +free_edac_mc:
> +	edac_mc_free(mci);

[Severity: High]
In the probe error path, mci is explicitly freed while the devres-managed
interrupt handler is still registered. Since hardware interrupts are triggered
asynchronously, could a spurious interrupt fire before the devres cleanup
runs?

If it does, the interrupt handler cdns_mc_err_handler() will dereference the
freed mci and priv pointers:

static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id)
{
        struct mem_ctl_info *mci = dev_id;
        struct cdns_mc_priv *priv = mci->pvt_info;
        ...

Would it be safer to explicitly free the irq or use the non-devres variant
of request_irq() to ensure it is freed before the mci structure?

> +	return ret;
> +}

[ ... ]

> +static void cdns_mc_remove(struct platform_device *pdev)
> +{
> +	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> +	struct cdns_mc_priv *priv = mci->pvt_info;
> +
> +	writel(CDNS_DDR_ECC_INT_CTRL_MASK_ALL, priv->io_base + CDNS_DDR_ECC_INT_CTRL);
> +#ifdef CONFIG_EDAC_DEBUG
> +	edac_debugfs_remove_recursive(priv->debugfs);
> +#endif
> +	edac_mc_del_mc(&pdev->dev);
> +	edac_mc_free(mci);

[Severity: High]
Similarly, since the interrupt handler was registered using devm_request_irq(),
its cleanup is deferred until after this remove function completes. Does
explicitly calling edac_mc_free() here create a window where a concurrent
interrupt could fire and access the freed mci structure?

Even though interrupts are masked at the device level earlier in this
function, an interrupt handler could already be executing concurrently on
another CPU when edac_mc_free() is called.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-paul-v7-3-rc1-edac-v4-0-4646a5b7cac9@bootlin.com?part=2