Re: [PATCH 3/4] EDAC/altera: Add support for Agilex5 ECC manager

[sashiko-bot@kernel.org]

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

New issues:
- [High] A flawed if/else chain causes Agilex5 to incorrectly fall through to legacy A10 logic if the optional `global_dbe` interrupt is missing, leading to probe failure or interrupt hijacking, and prevents registration of the intended panic notifier fallback.

Pre-existing issues:
- [High] Probe error paths fail to unregister the IRQ domain and chained handlers, leading to Resource Leaks and Use-After-Free.
- [High] The platform driver lacks a .remove callback, causing severe Use-After-Frees on module unbind.
--

--- Patch [3]: [PATCH 3/4] EDAC/altera: Add support for Agilex5 ECC manager ---
commit fe27bbb114924237edb349c2779f0fc2899da926
Author: Dinh Nguyen <dinguyen@kernel.org>

EDAC/altera: Add support for Agilex5 ECC manager

This commit adds support for the Agilex5 ECC manager to the Arria10 EDAC
driver. It introduces parsing for up to seven named interrupts, routing the
mandatory global_sbe and optional global_dbe through the chained interrupt
handler.

> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
> index e833869236914..13abce089ef1c 100644
> --- a/drivers/edac/altera_edac.c
> +++ b/drivers/edac/altera_edac.c
[ ... ]
> @@ -2145,14 +2197,31 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> -	edac->sb_irq = platform_get_irq(pdev, 0);
> -	if (edac->sb_irq < 0)
> -		return edac->sb_irq;
> +	if (edac->flag == SOCFPGA_AGILEX5) {
> +		rc = altr_agilex5_get_named_irqs(pdev, edac);
> +		if (rc)
> +			return rc;

[Severity: High]
This is a pre-existing issue, but if altr_agilex5_get_named_irqs() returns
an error like -EPROBE_DEFER, does this return path leak the IRQ domain
created earlier in altr_edac_a10_probe()?

Because edac is allocated with devres, the memory will be freed, but the
IRQ domain might remain registered and point to the freed edac structure.

Could this lead to a use-after-free if the IRQ subsystem attempts to access
the domain's host_data?

> +
> +		edac->sb_irq = edac->agilex5_irqs[ALTR_AGILEX5_IRQ_GLOBAL_SBE];
> +	} else {
> +		edac->sb_irq = platform_get_irq(pdev, 0);
> +		if (edac->sb_irq < 0)
> +			return edac->sb_irq;
> +	}
>  
>  	irq_set_chained_handler_and_data(edac->sb_irq,
>  					 altr_edac_a10_irq_handler,
>  					 edac);
>  
> +	/*
> +	 * Unlike Stratix10/Agilex7 (which deliver uncorrectable errors as
> +	 * Asynchronous SError), Agilex5 routes the global double-bit error
> +	 * through a dedicated SPI ("global_dbe"). Hook it into the same
> +	 * chained handler so the existing sb_irq/db_irq demux in
> +	 * altr_edac_a10_irq_handler() routes DBE events through the manager's
> +	 * IRQ domain. The panic notifier below is still registered as a
> +	 * fall-back for any DBEs that escape to SError.
> +	 */
>  	if (edac->flag == SOCFPGA_S10) {

[Severity: High]
The comment above states the panic notifier is registered as a fallback
for Agilex5. However, this check only matches SOCFPGA_S10. Does this mean
the panic notifier is inadvertently skipped for Agilex5 devices?

>  		int dberror, err_addr;
>  
[ ... ]
> @@ -2175,6 +2244,12 @@ static int altr_edac_a10_probe(struct platform_device *pdev)
>  			regmap_write(edac->ecc_mgr_map,
>  				     S10_SYSMGR_UE_ADDR_OFST, 0);
>  		}
> +	} else if (edac->flag == SOCFPGA_AGILEX5 &&
> +		   edac->agilex5_irqs[ALTR_AGILEX5_IRQ_GLOBAL_DBE] >= 0) {
> +		edac->db_irq = edac->agilex5_irqs[ALTR_AGILEX5_IRQ_GLOBAL_DBE];
> +		irq_set_chained_handler_and_data(edac->db_irq,
> +						 altr_edac_a10_irq_handler,
> +						 edac);
>  	} else {
>  		edac->db_irq = platform_get_irq(pdev, 1);
>  		if (edac->db_irq < 0)
>  			return edac->db_irq;

[Severity: High]
If an Agilex5 device omits the optional GLOBAL_DBE interrupt, will it
incorrectly fall through to this else branch?

This would cause it to call platform_get_irq(pdev, 1). If the device has
other optional interrupts, this might incorrectly hijack one of them (like
io96b0) as the chained DBE handler. If it doesn't have other interrupts,
it might fail and abort the probe with -ENXIO.

Also, this is a pre-existing issue, but if platform_get_irq(pdev, 1) fails,
does returning an error here leave the sb_irq chained handler dangling?

Since it was registered earlier in altr_edac_a10_probe(), it could cause a
use-after-free when the interrupt fires and dereferences the devres-freed
edac pointer.

[ ... ]
> @@ -2206,6 +2281,8 @@ static const struct of_device_id altr_edac_a10_of_match[] = {
>  	{ .compatible = "altr,socfpga-a10-ecc-manager" },
>  	{ .compatible = "altr,socfpga-s10-ecc-manager",
>  	  .data = (void *)SOCFPGA_S10 },
> +	{ .compatible = "altr,socfpga-agilex5-ecc-manager",
> +	  .data = (void *)SOCFPGA_AGILEX5 },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, altr_edac_a10_of_match);

[Severity: High]
This is a pre-existing issue, but does the platform driver lack a .remove
callback?

drivers/edac/altera_edac.c:
static struct platform_driver altr_edac_a10_driver = {
        .probe =  altr_edac_a10_probe,
        .driver = {
                .name = "socfpga_a10_ecc_manager",
                .of_match_table = altr_edac_a10_of_match,
        },
};

If the driver is unbound via sysfs, devres will automatically free the
edac struct and unmap MMIO regions. However, the chained IRQ handlers,
IRQ domain, and panic notifier remain registered.

Could this cause a use-after-free if a hardware interrupt or kernel panic
occurs after the module is unbound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720201425.1538771-1-dinguyen@kernel.org?part=3