Re: [PATCH 4/7] EDAC/aspeed: Free the mem_ctl_info unconditionally on remove

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

Pre-existing issues:
- [High] Potential use-after-free of `mci` in the interrupt handler during driver removal due to devm-managed IRQ and manual resource freeing.
--

--- Patch [4]: [PATCH 4/7] EDAC/aspeed: Free the mem_ctl_info unconditionally on remove ---
commit c5d400d5b360781ecdef9cf321a50a2fde1b94dc
Author: Ryan Chen <[email protected]>

EDAC/aspeed: Free the mem_ctl_info unconditionally on remove

This commit removes a redundant NULL check on the return value of
edac_mc_del_mc() in aspeed_remove(), instead fetching the mem_ctl_info
from driver data to free it unconditionally.

> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 71535e0b6bad8..8bfeb21d3204a 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c
> @@ -343,16 +343,15 @@ static int aspeed_probe(struct platform_device *pdev)
>  
>  static void aspeed_remove(struct platform_device *pdev)
>  {
> -	struct mem_ctl_info *mci;
> +	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
>  
>  	/* disable interrupts */
>  	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
>  			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
>  
>  	/* free resources */
> -	mci = edac_mc_del_mc(&pdev->dev);
> -	if (mci)
> -		edac_mc_free(mci);
> +	edac_mc_del_mc(&pdev->dev);
> +	edac_mc_free(mci);

[Severity: High]
This is a pre-existing issue, but could this edac_mc_free() cause a
use-after-free if the interrupt handler is running concurrently during
driver removal?

Because config_irq() registers the interrupt using devm_request_irq(), the
IRQ handler won't be unregistered and synchronized until devres cleanup
runs, which happens after aspeed_remove() returns.

While the hardware interrupt is disabled via regmap_update_bits() just
above, this does not wait for already-executing handlers on other CPUs
to finish. If mcr_isr() is running on another CPU, it could access the
just-freed mci:

mcr_isr() {
	struct mem_ctl_info *mci = arg;
	...
	dev_dbg(mci->pdev, "received edac interrupt...");
}

Would it be safer to explicitly free and synchronize the interrupt before
freeing mci, or use a non-devm request_irq()/free_irq() instead?

>  }
>  
>  static const struct of_device_id aspeed_of_match[] = {

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