[PATCH 0/3] fix leaked of_node references in drivers/firmware

[PATCH 0/3] fix leaked of_node references in drivers/firmware

[Wen Yang]

The call to of_get_cpu_node/of_find_compatible_node/of_parse_phandle...
returns a node pointer with refcount incremented thus it must be
explicitly decremented after the last usage.

We developed a coccinelle SmPL to detect drivers/firmware code and
found some issues.
This patch series fixes those issues.

Wen Yang (3):
  firmware: arm_sdei: fix leaked of_node references
  firmware: psci: fix leaked of_node references
  firmware: stratix10-svc: fix leaked of_node references

 drivers/firmware/arm_sdei.c      |  1 +
 drivers/firmware/psci.c          |  4 +++-
 drivers/firmware/stratix10-svc.c | 14 ++++++++++----
 3 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.9.5

[PATCH 1/3] firmware: arm_sdei: fix leaked of_node references

[Wen Yang]

In sdei_present_dt function, fw_np is obtained by calling
of_find_node_by_name(), np is obtained by calling
of_find_matching_node(), and the reference counts of those
two device_nodes, fw_np and np, are increased.
But when the function exits, only of_node_put is called on np,
and fw_np's reference count is leaked.

Detected by coccinelle with the following warnings:
./drivers/firmware/arm_sdei.c:1088:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function.
./drivers/firmware/arm_sdei.c:1091:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: James Morse <james.morse@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/arm_sdei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index e6376f9..2faa329 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1084,6 +1084,7 @@ static bool __init sdei_present_dt(void)
 		return false;
 
 	np = of_find_matching_node(fw_np, sdei_of_match);
+	of_node_put(fw_np);
 	if (!np)
 		return false;
 	of_node_put(np);
-- 
2.9.5

[PATCH 1/3] firmware: arm_sdei: fix leaked of_node references

[Wen Yang]

In sdei_present_dt function, fw_np is obtained by calling
of_find_node_by_name(), np is obtained by calling
of_find_matching_node(), and the reference counts of those
two device_nodes, fw_np and np, are increased.
But when the function exits, only of_node_put is called on np,
and fw_np's reference count is leaked.

Detected by coccinelle with the following warnings:
./drivers/firmware/arm_sdei.c:1088:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function.
./drivers/firmware/arm_sdei.c:1091:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1082, but without a corresponding object release within this function.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: James Morse <james.morse@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/arm_sdei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
index e6376f9..2faa329 100644
--- a/drivers/firmware/arm_sdei.c
+++ b/drivers/firmware/arm_sdei.c
@@ -1084,6 +1084,7 @@ static bool __init sdei_present_dt(void)
 		return false;
 
 	np = of_find_matching_node(fw_np, sdei_of_match);
+	of_node_put(fw_np);
 	if (!np)
 		return false;
 	of_node_put(np);
-- 
2.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] firmware: psci: fix leaked of_node references

[Wen Yang]

The call to of_find_matching_node_and_match returns a node pointer
with refcount incremented thus it must be explicitly decremented
after the last usage.

672 int __init psci_dt_init(void)
673 {
674         struct device_node *np;
    ...
678         np = of_find_matching_node_and_match(...);
679
680         if (!np || !of_device_is_available(np))
682                 return -ENODEV;  ---> leaked here
    ...
686         return init_fn(np);      ---> released here
687 }

Detected by using coccinelle.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/psci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index c80ec1d..e4143f8 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -677,8 +677,10 @@ int __init psci_dt_init(void)
 
 	np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
 
-	if (!np || !of_device_is_available(np))
+	if (!np || !of_device_is_available(np)) {
+		of_node_put(np);
 		return -ENODEV;
+	}
 
 	init_fn = (psci_initcall_t)matched_np->data;
 	return init_fn(np);
-- 
2.9.5

[PATCH 2/3] firmware: psci: fix leaked of_node references

[Wen Yang]

The call to of_find_matching_node_and_match returns a node pointer
with refcount incremented thus it must be explicitly decremented
after the last usage.

672 int __init psci_dt_init(void)
673 {
674         struct device_node *np;
    ...
678         np = of_find_matching_node_and_match(...);
679
680         if (!np || !of_device_is_available(np))
682                 return -ENODEV;  ---> leaked here
    ...
686         return init_fn(np);      ---> released here
687 }

Detected by using coccinelle.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/psci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index c80ec1d..e4143f8 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -677,8 +677,10 @@ int __init psci_dt_init(void)
 
 	np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
 
-	if (!np || !of_device_is_available(np))
+	if (!np || !of_device_is_available(np)) {
+		of_node_put(np);
 		return -ENODEV;
+	}
 
 	init_fn = (psci_initcall_t)matched_np->data;
 	return init_fn(np);
-- 
2.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] firmware: stratix10-svc: fix leaked of_node references

[Wen Yang]

In stratix10_svc_init function, fw_np is obtained by calling
of_find_node_by_name(), np is obtained by calling
of_find_matching_node(), and the reference counts of those
two device_nodes, fw_np and np, are increased.
But when the function exits, only of_node_put is called on np,
and fw_np's reference count is leaked.

Detected by coccinelle with the following warnings:
./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function.
./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function.
./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 1014, but without a corresponding object release within this function.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Tull <atull@kernel.org>
Cc: Richard Gong <richard.gong@intel.com>
Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/stratix10-svc.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index 6e65148..482a6bd 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void)
 		return -ENODEV;
 
 	np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
-	if (!np)
-		return -ENODEV;
+	if (!np) {
+		ret = -ENODEV;
+		goto out_put_fw_np;
+	}
 
 	of_node_put(np);
 	ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
 	if (ret)
-		return ret;
+		goto out_put_fw_np;
 
-	return platform_driver_register(&stratix10_svc_driver);
+	ret = platform_driver_register(&stratix10_svc_driver);
+
+out_put_fw_np:
+	of_node_put(fw_np);
+	return ret;
 }
 
 static void __exit stratix10_svc_exit(void)
-- 
2.9.5

Re: [PATCH 3/3] firmware: stratix10-svc: fix leaked of_node references

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

On Wed, 2019-04-17 at 10:44 +0800, Wen Yang wrote:
> In stratix10_svc_init function, fw_np is obtained by calling
> of_find_node_by_name(), np is obtained by calling
> of_find_matching_node(), and the reference counts of those
> two device_nodes, fw_np and np, are increased.
> But when the function exits, only of_node_put is called on np,
> and fw_np's reference count is leaked.
> 
> Detected by coccinelle with the following warnings:
> ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
> ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
> ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put;
> acquired a node pointer with refcount incremented on line 1014, but without a
> corresponding object release within this function.
> 
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Alan Tull <atull@kernel.org>
> Cc: Richard Gong <richard.gong@intel.com>
> Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/firmware/stratix10-svc.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-
> svc.c
> index 6e65148..482a6bd 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void)
>  		return -ENODEV;
>  
>  	np = of_find_matching_node(fw_np, stratix10_svc_drv_match);

Sorry but this patch isn't right, of_find_matching_node() will free the
reference to fw_np internally.

> -	if (!np)
> -		return -ENODEV;
> +	if (!np) {
> +		ret = -ENODEV;
> +		goto out_put_fw_np;
> +	}
>  
>  	of_node_put(np);
>  	ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
>  	if (ret)
> -		return ret;
> +		goto out_put_fw_np;

Consequently and assuming I'm not missing something, I think fw_np shouldn't be
used here as is.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] firmware: stratix10-svc: fix leaked of_nodereferences

[wen.yang99]

[-- Attachment #1.1: Type: text/plain, Size: 3694 bytes --]

Hi Nicolas, 

> > In stratix10_svc_init function, fw_np is obtained by calling
> > of_find_node_by_name(), np is obtained by calling
> > of_find_matching_node(), and the reference counts of those
> > two device_nodes, fw_np and np, are increased.
> > But when the function exits, only of_node_put is called on np,
> > and fw_np's reference count is leaked.
> >
> > Detected by coccinelle with the following warnings:
> > ./drivers/firmware/stratix10-svc.c:1020:2-8: ERROR: missing of_node_put;
> > acquired a node pointer with refcount incremented on line 1014, but without a
> > corresponding object release within this function.
> > ./drivers/firmware/stratix10-svc.c:1025:2-8: ERROR: missing of_node_put;
> > acquired a node pointer with refcount incremented on line 1014, but without a
> > corresponding object release within this function.
> > ./drivers/firmware/stratix10-svc.c:1027:1-7: ERROR: missing of_node_put;
> > acquired a node pointer with refcount incremented on line 1014, but without a
> > corresponding object release within this function.
> >
> > Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Alan Tull <atull@kernel.org>
> > Cc: Richard Gong <richard.gong@intel.com>
> > Cc: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/firmware/stratix10-svc.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-
> > svc.c
> > index 6e65148..482a6bd 100644
> > --- a/drivers/firmware/stratix10-svc.c
> > +++ b/drivers/firmware/stratix10-svc.c
> > @@ -1016,15 +1016,21 @@ static int __init stratix10_svc_init(void)
> >          return -ENODEV;
> >
> >      np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
> 
> Sorry but this patch isn't right, of_find_matching_node() will free the
> reference to fw_np internally.
> 
> > -    if (!np)
> > -        return -ENODEV;
> > +    if (!np) {
> > +        ret = -ENODEV;
> > +        goto out_put_fw_np;
> > +    }
> >
> >      of_node_put(np);
> >      ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
> >      if (ret)
> > -        return ret;
> > +        goto out_put_fw_np;
> 
> Consequently and assuming I'm not missing something, I think fw_np shouldn't be
> used here as is.

Ok, thank you very much for your comments.
of_find_matching_node() will indeed free the reference to fw_np internally.
We will continue to improve the SmPL rules based on your comments.

In addition, we also checked it further and found that this commit may have a problem:
32a66374487b ("fpga: stratix10-soc: fix wrong of_node_put() in init function")

drivers/fpga/stratix10-soc.c
500 static int __init s10_init(void)
501 {
502         struct device_node *fw_np;
503         struct device_node *np;
504         int ret;
505 
506         fw_np = of_find_node_by_name(NULL, "svc");
507         if (!fw_np)
508                 return -ENODEV;
509 
510         np = of_find_matching_node(fw_np, s10_of_match);  -->fw_np released here
511         if (!np)
512                 return -ENODEV;
513 
514         of_node_put(np);
515         ret = of_platform_populate(fw_np, s10_of_match, NULL, NULL);   -->fw_np shouldn't be used here
516         if (ret)
517                 return ret;
518 
519         return platform_driver_register(&s10_driver);
520 }

We plan to add such SmPL rules to further check the current kernel code:
after of_find_matching_node(node, ...), of_node_put(node) should not be called,
and "node" should not be used (unless there is an extra of_node_get).

--
Best wishes,
Wen