Re: [PATCH] irqchip/imx-intmux: fix runtime PM teardown on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error paths in `imx_intmux_probe()` fail to unwind Runtime PM initialization and leak IRQ domains and chained handlers, leading to permanent PM usage count leaks and critical use-after-free vulnerabilities.
- [High] The chained IRQ handler accesses hardware registers without verifying that the device is in an active Runtime PM state, risking a bus fault.
--

commit 3fdfa141ddbe4114d8632a64386f9d6fcbce0d98
Author: Guangshuo Li <lgs201920130244@gmail.com>

irqchip/imx-intmux: fix runtime PM teardown on remove

This commit fixes runtime PM teardown in imx_intmux_remove() by properly
resuming the device before accessing registers. It also balances the clock
enable configuration for both CONFIG_PM states.

> diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
> index 47c2681d138a0..de2a8f98f6255 100644
> --- a/drivers/irqchip/irq-imx-intmux.c
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -288,12 +288,16 @@ static int imx_intmux_probe(struct platform_device *pdev)
>  static void imx_intmux_remove(struct platform_device *pdev)
>  {
>  	struct intmux_data *data = platform_get_drvdata(pdev);
> -	int i;
> +	int i, ret;
> +
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0)
> +		dev_warn(&pdev->dev, "failed to resume device: %d\n", ret);

[ ... ]

>  	pm_runtime_disable(&pdev->dev);
> +	if (ret >= 0) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		clk_disable_unprepare(data->ipg_clk);
> +		pm_runtime_set_suspended(&pdev->dev);
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed imx_intmux_probe()
has some error paths that fail to unwind Runtime PM initialization and leak
IRQ domains and chained handlers.

If clk_prepare_enable() fails, the function returns an error without unwinding
the pm_runtime_enable() and pm_runtime_get_noresume() calls:

drivers/irqchip/irq-imx-intmux.c:imx_intmux_probe() {
    ...
	pm_runtime_get_noresume(&pdev->dev);
	pm_runtime_set_active(&pdev->dev);
	pm_runtime_enable(&pdev->dev);

	ret = clk_prepare_enable(data->ipg_clk);
	if (ret) {
		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
		return ret;
	}
    ...
}

Also, if initialization fails for a channel, the function jumps to the out
label and returns without tearing down the chained handlers and IRQ domains
created in previous successful iterations:

drivers/irqchip/irq-imx-intmux.c:imx_intmux_probe() {
    ...
	for (i = 0; i < channum; i++) {
        ...
		domain = irq_domain_create_linear(of_fwnode_handle(np), 32,
						  &imx_intmux_domain_ops,
						  &data->irqchip_data[i]);
		if (!domain) {
			ret = -ENOMEM;
			dev_err(&pdev->dev, "failed to create IRQ domain\n");
			goto out;
		}
        ...
	}
    ...
out:
	clk_disable_unprepare(data->ipg_clk);
	return ret;
}

Can this lead to a permanent PM usage count leak, or a use-after-free if
a hardware event triggers the parent IRQ line whose chained handler was left
registered?


[Severity: High]
This is a pre-existing issue, but the chained IRQ handler accesses hardware
registers without verifying that the device is in an active Runtime PM state:

drivers/irqchip/irq-imx-intmux.c:imx_intmux_irq_handler() {
    ...
	chained_irq_enter(irq_desc_get_chip(desc), desc);

	/* read the interrupt source pending status of this channel */
	irqstat = readl_relaxed(data->regs + CHANIPR(idx));

	for_each_set_bit(pos, &irqstat, 32)
		generic_handle_domain_irq(irqchip_data->domain, pos);
    ...
}

If a spurious interrupt arrives on the parent IRQ line while the intmux
device is runtime-suspended and its clock is disabled, wouldn't this
unclocked hardware access cause a bus fault? Should the handler use
pm_runtime_get_if_active() to confirm the device is active before reading
the registers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914134211.1734163-1-lgs201920130244@gmail.com?part=1