Re: [PATCH 7/7] EDAC/aspeed: Add AST2700 support

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

New issues:
- [High] Unconditional `IRQ_HANDLED` in `ast2700_dramc_isr()` combined with selective interrupt clearing can cause an interrupt storm if a non-ECC interrupt is asserted.

Pre-existing issues:
- [High] The `mci` structure is freed in `aspeed_remove()` while the devm-managed interrupt handler is still registered and active, leading to a potential Use-After-Free.
--

--- Patch [7]: [PATCH 7/7] EDAC/aspeed: Add AST2700 support ---
commit c0a432a45b2a7c26475522dd80a9b976dbcdd2d3
Author: Ryan Chen <[email protected]>
Subject: EDAC/aspeed: Add AST2700 support

Add SDRAM ECC reporting for the Aspeed AST2700. Its DRAMC has a different
register layout, requiring new interrupt handling and per-chip ISR hooks.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 91df5d2df5f11..8b48044a00d14 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c

[ ... ]

> @@ -174,6 +199,52 @@ static irqreturn_t aspeed_mcr_isr(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t ast2700_dramc_isr(int irq, void *arg)
> +{
> +	u32 int_sts, ecc_sts, fail_addr;
> +	struct mem_ctl_info *mci = arg;
> +	struct aspeed_edac *priv;
> +	u8 rec_cnt, un_rec_cnt;
> +	phys_addr_t addr;
> +
> +	priv = mci->pvt_info;
> +
> +	scoped_guard(raw_spinlock, &priv->lock) {
> +		int_sts = readl(priv->regs + AST2700_INT_STS);
> +		ecc_sts = readl(priv->regs + AST2700_ECC_STS);
> +		fail_addr = readl(priv->regs + AST2700_ECC_FAIL_ADDR);
> +
> +		/* the interrupt registers are not key-protected; clear only ECC */
> +		writel(int_sts & (AST2700_INT_ECC_RECOVERABLE | AST2700_INT_ECC_UNRECOVERABLE),
> +		       priv->regs + AST2700_INT_CLR);
> +	}
> +
> +	rec_cnt = FIELD_GET(AST2700_ECC_REC_CNT, ecc_sts);
> +	un_rec_cnt = FIELD_GET(AST2700_ECC_UNREC_CNT, ecc_sts);
> +
> +	/* the register holds address bits [35:4], in units of 16 bytes */
> +	addr = (phys_addr_t)fail_addr << 4;
> +
> +	/*
> +	 * The controller records only the address of the latest failure,
> +	 * shared by both error types. When only one type occurred it owns
> +	 * that address; when both occurred attribute it to the uncorrectable
> +	 * error and report the corrected ones without an address.
> +	 */
> +	if (un_rec_cnt && !rec_cnt) {
> +		count_un_rec(mci, un_rec_cnt, addr, true);
> +	} else if (!un_rec_cnt && rec_cnt) {
> +		count_rec(mci, rec_cnt, addr, true);
> +	} else if (un_rec_cnt && rec_cnt) {
> +		count_un_rec(mci, un_rec_cnt, addr, true);
> +		count_rec(mci, rec_cnt, 0, false);
> +	} else {
> +		dev_dbg_ratelimited(mci->pdev, "received interrupt with no ECC counters set\n");
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Can this cause an interrupt storm if a non-ECC interrupt is asserted?

The handler intentionally clears only the ECC interrupt bits in AST2700_INT_CLR,
but it unconditionally returns IRQ_HANDLED even if other unmasked bits were
asserted and left uncleared.

Because IRQ_HANDLED is returned unconditionally, the kernel's spurious
interrupt detector will not disable the level-triggered interrupt line. This
could result in an endless interrupt loop if a non-ECC bit is raised.

Would it be safer to return IRQ_NONE when no ECC bits are found, or ensure
all asserted bits are cleared?

[ ... ]

> @@ -354,9 +439,10 @@ static int aspeed_probe(struct platform_device *pdev)
>  static void aspeed_remove(struct platform_device *pdev)
>  {
>  	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
> +	struct aspeed_edac *priv = mci->pvt_info;
>  
>  	/* disable interrupts */
> -	aspeed_set_irq(mci, false);
> +	priv->chip->set_irq(mci, false);
>  
>  	/* free resources */
>  	edac_mc_del_mc(&pdev->dev);
>  	edac_mc_free(mci);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this code leave an active
interrupt handler vulnerable to a use-after-free?

The interrupt is registered using devm_request_irq() in config_irq(), meaning
it will be automatically freed after aspeed_remove() returns. However,
aspeed_remove() frees the mci structure here via edac_mc_free(mci).

If an interrupt is executing concurrently on another CPU, or fires after this
point but before the devm cleanup completes, the ISR will access the freed
mci structure:

ast2700_dramc_isr()
    priv = mci->pvt_info;
    scoped_guard(raw_spinlock, &priv->lock) {
        ...
    }

Should the interrupt be explicitly synchronized or freed (e.g. using
devm_free_irq) before freeing the mci structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7