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

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

New issues:
- [High] Unloading the irqchip module leaves dangling pointers to `imx_irqsteer_irq_chip` in active `irq_desc` mappings, causing a Use-After-Free kernel panic.

Pre-existing issues:
- [High] The driver calls `pm_runtime_enable()` in `probe()` but fails to call `pm_runtime_disable()` in `remove()`, breaking runtime PM on module reload.
--

--- Patch [1]: [PATCH] irqchip/imx-irqsteer: Allow building as module ---
commit 04000917a2f34d79136a34fa78a99d5b4adbc6e7
Author: Jindong Yue <[email protected]>

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().

> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 42f2278a702d0..07db3b678f94a 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -560,7 +560,7 @@ config CSKY_APB_INTC
>  	  the controller's register.
>  
>  config IMX_IRQSTEER
> -	bool "i.MX IRQSTEER support"
> +	tristate "i.MX IRQSTEER support"

[Severity: High]
Does allowing this driver to be unloaded as a module introduce a
use-after-free?

When the module is unloaded, imx_irqsteer_remove() calls irq_domain_remove(),
but this does not unmap existing child interrupts.

If an interrupt remains mapped, the irq_desc->irq_data.chip pointer will
continue to point to the statically allocated imx_irqsteer_irq_chip struct,
which resides in module memory. Once the module is unloaded, this memory is
freed.

Could a subsequent read of /proc/interrupts cause a kernel panic when
show_interrupts() attempts to access the freed imx_irqsteer_irq_chip->name?

[ ... ]
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index 87b07f517be35..2365fc04596ef 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
[ ... ]
> @@ -359,4 +361,7 @@ static struct platform_driver imx_irqsteer_driver = {
>  	.probe		= imx_irqsteer_probe,
>  	.remove		= imx_irqsteer_remove,

[Severity: High]
This is a pre-existing issue, but does imx_irqsteer_remove() need to call
pm_runtime_disable() to balance the pm_runtime_enable() called in probe()?

If the driver is unloaded and then reloaded, probe() will call
pm_runtime_set_active(). Since pm_runtime_disable() was never called during
removal, the disable_depth remains 0, causing pm_runtime_set_active() to
return -EAGAIN.

The subsequent call to pm_runtime_enable() in probe() will then trigger an
"Unbalanced pm_runtime_enable!" warning and leave the device's PM state
corrupted.

While this imbalance was present before via sysfs unbind, converting the
driver to a module now exposes it directly to standard rmmod and modprobe
operations.

>  };
> -builtin_platform_driver(imx_irqsteer_driver);
> +module_platform_driver(imx_irqsteer_driver);

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