Skip to contentSkip to conversation

soc: renesas: rmobile-sysc: Fix some leaks in rmobile_init_pm_domains()

PATCH Superseded Dan Carpenter 2 participants 5 messages Sep 23, 2020 – Sep 24, 2020

Subsystems (2)
Kernel janitors Arm/RISC-V/Renesas architecture
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 soc: renesas: rmobile-sysc: Fix some leaks in rmobile_init_pm_domains()

This code needs to call iounmap() on the error paths.

Fixes 2ed29e15e4b2 ("ARM: shmobile: R-Mobile: Move pm-rmobile to drivers/soc/renesas/")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/soc/renesas/rmobile-sysc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/soc/renesas/rmobile-sysc.c +2 -0
index 54b616ad4a62..d8e6dc650939 100644
@@ -328,6 +328,7 @@ static int __init rmobile_init_pm_domains(void)
 		pmd = of_get_child_by_name(np, "pm-domains");
 		if (!pmd) {
 			pr_warn("%pOF lacks pm-domains node\n", np);
+			iounmap(base);
 			continue;
 		}
 

@@ -341,6 +342,7 @@ static int __init rmobile_init_pm_domains(void)
 		of_node_put(pmd);
 		if (ret) {
 			of_node_put(np);
+			iounmap(base);
 			break;
 		}
 	}
Signature · 1 line
2.28.0
#1 lore raw

Hi Dan,

Quoted 1 line · Dan Carpenter

This code needs to call iounmap() on the error paths.

Thanks for your patch!

Quoted 1 line
Fixes 2ed29e15e4b2 ("ARM: shmobile: R-Mobile: Move pm-rmobile to drivers/soc/renesas/")

This is not the commit that introduced the issue.

Fixes 2173fc7cb681c38b ("ARM: shmobile: R-Mobile: Add DT support for

PM domains")

drivers/soc/renesas/rmobile-sysc.c static int __init rmobile_init_pm_domains(void) · line 331

Quoted 9 lines
Signed-off-by Dan Carpenter <[email protected]>
drivers/soc/renesas/rmobile-sysc.c +1 -0
@@ -328,6 +328,7 @@ static int __init rmobile_init_pm_domains(void)
                pmd = of_get_child_by_name(np, "pm-domains");
                if (!pmd) {
                        pr_warn("%pOF lacks pm-domains node\n", np);
+                       iounmap(base);

This one I can agree with, although that case is a bug in the DTS.

drivers/soc/renesas/rmobile-sysc.c static int __init rmobile_init_pm_domains(void) · line 345

Quoted 8 lines
drivers/soc/renesas/rmobile-sysc.c +1 -0
                        continue;
                }
 

@@ -341,6 +342,7 @@ static int __init rmobile_init_pm_domains(void)
                of_node_put(pmd);
                if (ret) {
                        of_node_put(np);
+                       iounmap(base);

This one I cannot: in the (unlikely, only if OOM) case rmobile_add_pm_domains() returns an error, one or more PM subdomains may have been registered already. Hence if you call iounmap() here, the code will try to access unmapped registers later, leading to a crash.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
author
#2 lore raw
Quoted 10 lines · Geert Uytterhoeven

Hi Dan,

Quoted 1 line · Dan Carpenter

This code needs to call iounmap() on the error paths.

Thanks for your patch!

Quoted 1 line
Fixes 2ed29e15e4b2 ("ARM: shmobile: R-Mobile: Move pm-rmobile to drivers/soc/renesas/")

This is not the commit that introduced the issue.

Duh... I don't know what I was thinking there.

Quoted 19 lines
Fixes 2173fc7cb681c38b ("ARM: shmobile: R-Mobile: Add DT support for

PM domains")

drivers/soc/renesas/rmobile-sysc.c static int __init rmobile_init_pm_domains(void) · line 331

Quoted 9 lines
Signed-off-by Dan Carpenter <[email protected]>
drivers/soc/renesas/rmobile-sysc.c +1 -0
@@ -328,6 +328,7 @@ static int __init rmobile_init_pm_domains(void)
                pmd = of_get_child_by_name(np, "pm-domains");
                if (!pmd) {
                        pr_warn("%pOF lacks pm-domains node\n", np);
+                       iounmap(base);

This one I can agree with, although that case is a bug in the DTS.

Quoted 3 lines
drivers/soc/renesas/rmobile-sysc.c +0 -0
                        continue;
                }
Quoted 11 lines

drivers/soc/renesas/rmobile-sysc.c static int __init rmobile_init_pm_domains(void) · line 345

Quoted 5 lines
drivers/soc/renesas/rmobile-sysc.c +1 -0
@@ -341,6 +342,7 @@ static int __init rmobile_init_pm_domains(void)
                of_node_put(pmd);
                if (ret) {
                        of_node_put(np);
+                       iounmap(base);

This one I cannot: in the (unlikely, only if OOM) case rmobile_add_pm_domains() returns an error, one or more PM subdomains may have been registered already. Hence if you call iounmap() here, the code will try to access unmapped registers later, leading to a crash.

It's actually impossible for this rmobile_add_pm_domains() to fail on current kernels because small allocations never fail...

I'll send a v2. This is for a new static checker test that I added to Smatch so I'm just sending a few of these out every day to collect feedback for now. So thanks for reviewing this, it's very helpful.

regards,
dan carpenter

author
#3 lore raw

PATCH v2 1/1 soc: renesas: rmobile-sysc: Fix some leaks in rmobile_init_pm_domains()

In reply to Geert Uytterhoeven

This code needs to call iounmap() on one error path.

Fixes 2173fc7cb681 ("ARM: shmobile: R-Mobile: Add DT support for PM domains")
Signed-off-by Dan Carpenter <[email protected]>

---
v2: The v1 patch potentially led to a use after free.

 drivers/soc/renesas/rmobile-sysc.c | 1 +
 1 file changed, 1 insertion(+)
drivers/soc/renesas/rmobile-sysc.c +1 -0
index 54b616ad4a62..beb1c7211c3d 100644
@@ -327,6 +327,7 @@ static int __init rmobile_init_pm_domains(void)
 
 		pmd = of_get_child_by_name(np, "pm-domains");
 		if (!pmd) {
+			iounmap(base);
 			pr_warn("%pOF lacks pm-domains node\n", np);
 			continue;
 		}
Signature · 1 line
2.28.0
Reviewed-by on v2 1/1
#4 lore raw
Quoted 6 lines · Dan Carpenter

This code needs to call iounmap() on one error path.

Fixes 2173fc7cb681 ("ARM: shmobile: R-Mobile: Add DT support for PM domains")
Signed-off-by Dan Carpenter <[email protected]>

---
v2: The v1 patch potentially led to a use after free.

Reviewed-by Geert Uytterhoeven <[email protected]>

i.e. will queue in renesas-devel for v5.11.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds