Re: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unloading the module while the device is runtime-suspended causes a clock enable count underflow and kernel WARN.
- [High] Spurious parent interrupts during runtime suspend cause a bus error/system crash due to unprotected hardware access.
- [High] Missing synchronize_irq() in remove() leads to a Use-After-Free race condition with in-flight chained handlers.
- [High] An unvalidated device tree property `fsl,num-irqs` leads to an out-of-bounds memory write.
--

--- Patch [5]: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module ---
commit 4fdbf892274cef735a839f80bcfd62708a887cbc
Author: Jindong Yue <jindong.yue@nxp.com>

irqchip/imx-irqsteer: Allow building as module

Make the driver buildable as a module by turning the Kconfig symbol into
a tristate and using module_platform_driver() instead of
builtin_platform_driver(). Let the driver core own the clock and
runtime PM lifetime using devres.

> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index a9909ecb6fef7..a60cc527e6191 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c

[ ... ]

> @@ -193,7 +194,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  		return PTR_ERR(data->regs);
>  	}
>  
> -	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	data->ipg_clk = devm_clk_get_enabled(&pdev->dev, "ipg");

[Severity: High]
This is a pre-existing issue, but since the patch modifies
imx_irqsteer_probe(), could the fsl,num-irqs property cause an out-of-bounds
write?

In imx_irqsteer_probe(), data->reg_num is calculated from fsl,num-irqs and
then used in a loop to clear CHANMASK:

for (i = 0; i < data->reg_num; i++)
        writel_relaxed(0, data->regs + CHANMASK(i, data->reg_num));

However, the bounds check to ensure data->irq_count does not exceed
CHAN_MAX_OUTPUT_INT happens after this loop. Could a large value in the
device tree lead to writing past the mapped IOMEM region?

>  	if (IS_ERR(data->ipg_clk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(data->ipg_clk),
>  				     "failed to get ipg clk\n");

[ ... ]

> @@ -293,8 +297,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  						 NULL, NULL);

[Severity: High]
This is a pre-existing issue, but when clearing the chained handlers here,
is there a risk of a use-after-free?

The chained handler is cleared via irq_set_chained_handler_and_data(), but
this does not wait for handlers executing on other CPUs to finish. When
devres subsequently unmaps the I/O memory, could an in-flight
imx_irqsteer_irq_handler() concurrently read data->regs and cause a bus
fault? Would calling synchronize_irq() be needed here?

>  		irq_dispose_mapping(irqsteer_data->irq[i]);
>  	}
> -
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
>  }

[Severity: High]
This is a pre-existing issue, but does removing manual clock management in
favor of devres introduce a clock enable count underflow during unbind?

If the module is unloaded while the device is auto-suspended, the clock is
disabled by imx_irqsteer_suspend(). Because the device is left in a
suspended state when imx_irqsteer_remove() returns, devm_clk_get_enabled()
teardown will unconditionally call clk_disable_unprepare().

Since pm_runtime_disable() does not automatically resume the device, will
this result in clk_disable() being called with an enable count of 0? Should
we call pm_runtime_get_sync() in imx_irqsteer_remove() to ensure the clock
is active before devres cleanup?

>  
>  #ifdef CONFIG_PM
> @@ -324,7 +326,7 @@ static int imx_irqsteer_suspend(struct device *dev)
>  	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
>  
>  	imx_irqsteer_save_regs(irqsteer_data);
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	clk_disable(irqsteer_data->ipg_clk);

[Severity: High]
This is a pre-existing issue, but since we are modifying the clock disable
logic in suspend, what happens if a spurious interrupt arrives while the
device is suspended?

imx_irqsteer_irq_handler() unconditionally reads the CHANSTATUS register
using readl_relaxed(). If imx_irqsteer_suspend() has disabled the hardware
clock, wouldn't reading from an unclocked peripheral trigger a bus error or
system crash?

Does the interrupt handler need to use pm_runtime_get_if_active() to verify
the power state before attempting hardware access?

>  
>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819090543.585131-1-Zhipeng.wang_1@oss.nxp.com?part=5