[PATCH] ARM: imx: avic: fix device_node refcount leaks in mxc_init_irq()

[PATCH] ARM: imx: avic: fix device_node refcount leaks in mxc_init_irq()

[Weigang He]

mxc_init_irq() obtains two device_node references via
of_find_compatible_node() and never releases either one:

  - The "fsl,imx25-ccm" node (looked up to map the CCM low-power
    interrupt mask registers on i.MX25) is stored in np, used by
    of_iomap(), and then the same np variable is overwritten by the
    second of_find_compatible_node() call without an of_node_put().
    On i.MX25 this leaks the node reference on every boot.

  - The "fsl,avic" node is passed via of_fwnode_handle(np) to
    irq_domain_create_legacy(), which takes its own reference on the
    fwnode through fwnode_handle_get(), so the caller's reference is
    not transferred. np is then leaked at function return.

Both lookups predate the switch to irq_domain_create_*(); the missing
puts have been there since the code was introduced.

Drop each reference once the value derived from it is no longer needed:
after of_iomap() has mapped the CCM registers, and after
irq_domain_create_legacy() has taken its own fwnode reference.
of_node_put() is NULL-safe, so platforms without these nodes are
unaffected.

Found by static analysis tool CodeQL.

Fixes: 544496ab5cbd ("ARM: imx: move irq_domain_add_legacy call into avic driver")
Fixes: 9b454d16e57d ("ARM: imx: avic: set low-power interrupt mask for imx25")
Signed-off-by: Weigang He <geoffreyhe2@gmail.com>
---
 arch/arm/mach-imx/avic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-imx/avic.c b/arch/arm/mach-imx/avic.c
index 3067c06b4b8eb..6873a50bbe2c0 100644
--- a/arch/arm/mach-imx/avic.c
+++ b/arch/arm/mach-imx/avic.c
@@ -173,6 +173,7 @@ static void __init mxc_init_irq(void __iomem *irqbase)
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx25-ccm");
 	mx25_ccm_base = of_iomap(np, 0);
+	of_node_put(np);
 
 	if (mx25_ccm_base) {
 		/*
@@ -203,6 +204,7 @@ static void __init mxc_init_irq(void __iomem *irqbase)
 	np = of_find_compatible_node(NULL, NULL, "fsl,avic");
 	domain = irq_domain_create_legacy(of_fwnode_handle(np), AVIC_NUM_IRQS, irq_base, 0,
 					  &irq_domain_simple_ops, NULL);
+	of_node_put(np);
 	WARN_ON(!domain);
 
 	for (i = 0; i < AVIC_NUM_IRQS / 32; i++, irq_base += 32)

base-commit: 0f61b1860cc3f52aef9036d7235ed1f017632193
-- 
2.43.0

Re: [PATCH] ARM: imx: avic: fix device_node refcount leaks in mxc_init_irq()

[Martin Kaiser]

Hi,

Thus wrote Weigang He (geoffreyhe2@gmail.com):

> mxc_init_irq() obtains two device_node references via
> of_find_compatible_node() and never releases either one:

>   - The "fsl,imx25-ccm" node (looked up to map the CCM low-power
>     interrupt mask registers on i.MX25) is stored in np, used by
>     of_iomap(), and then the same np variable is overwritten by the
>     second of_find_compatible_node() call without an of_node_put().
>     On i.MX25 this leaks the node reference on every boot.

>   - The "fsl,avic" node is passed via of_fwnode_handle(np) to
>     irq_domain_create_legacy(), which takes its own reference on the
>     fwnode through fwnode_handle_get(), so the caller's reference is
>     not transferred. np is then leaked at function return.

> Both lookups predate the switch to irq_domain_create_*(); the missing
> puts have been there since the code was introduced.

> Drop each reference once the value derived from it is no longer needed:
> after of_iomap() has mapped the CCM registers, and after
> irq_domain_create_legacy() has taken its own fwnode reference.
> of_node_put() is NULL-safe, so platforms without these nodes are
> unaffected.

> Found by static analysis tool CodeQL.

> Fixes: 544496ab5cbd ("ARM: imx: move irq_domain_add_legacy call into avic driver")
> Fixes: 9b454d16e57d ("ARM: imx: avic: set low-power interrupt mask for imx25")
> Signed-off-by: Weigang He <geoffreyhe2@gmail.com>
> ---
>  arch/arm/mach-imx/avic.c | 2 ++
>  1 file changed, 2 insertions(+)

> diff --git a/arch/arm/mach-imx/avic.c b/arch/arm/mach-imx/avic.c
> index 3067c06b4b8eb..6873a50bbe2c0 100644
> --- a/arch/arm/mach-imx/avic.c
> +++ b/arch/arm/mach-imx/avic.c
> @@ -173,6 +173,7 @@ static void __init mxc_init_irq(void __iomem *irqbase)

>  	np = of_find_compatible_node(NULL, NULL, "fsl,imx25-ccm");
>  	mx25_ccm_base = of_iomap(np, 0);
> +	of_node_put(np);

>  	if (mx25_ccm_base) {
>  		/*
> @@ -203,6 +204,7 @@ static void __init mxc_init_irq(void __iomem *irqbase)
>  	np = of_find_compatible_node(NULL, NULL, "fsl,avic");
>  	domain = irq_domain_create_legacy(of_fwnode_handle(np), AVIC_NUM_IRQS, irq_base, 0,
>  					  &irq_domain_simple_ops, NULL);
> +	of_node_put(np);
>  	WARN_ON(!domain);

>  	for (i = 0; i < AVIC_NUM_IRQS / 32; i++, irq_base += 32)

> base-commit: 0f61b1860cc3f52aef9036d7235ed1f017632193
> -- 
> 2.43.0

Reviewed-by: Martin Kaiser <martin@kaiser.cx>

Thanks,
Martin