[PATCH 0/2] fix use-after-free in mpc831x_usb_cfg() and do some cleanups

[PATCH 0/2] fix use-after-free in mpc831x_usb_cfg() and do some cleanups

[Wen Yang]

Fix use-after-free in mpc831x_usb_cfg() and do some cleanups.
According to Markus's suggestion, split it into two small patches:
https://lkml.org/lkml/2019/7/8/520

Wen Yang (2):
  powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()
  powerpc/83xx: cleanup error paths in mpc831x_usb_cfg()

 arch/powerpc/platforms/83xx/usb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

-- 
2.9.5

[PATCH 1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[Wen Yang]

The immr_node variable is still being used after the of_node_put() call,
which may result in use-after-free.
Fix this issue by calling of_node_put() after the last usage.

Fixes: fd066e850351 ("powerpc/mpc8308: fix USB DR controller initialization")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 arch/powerpc/platforms/83xx/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 3d247d7..19dcef5 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -158,11 +158,10 @@ int mpc831x_usb_cfg(void)
 
 	iounmap(immap);
 
-	of_node_put(immr_node);
-
 	/* Map USB SOC space */
 	ret = of_address_to_resource(np, 0, &res);
 	if (ret) {
+		of_node_put(immr_node);
 		of_node_put(np);
 		return ret;
 	}
@@ -203,6 +202,7 @@ int mpc831x_usb_cfg(void)
 
 out:
 	iounmap(usb_regs);
+	of_node_put(immr_node);
 	of_node_put(np);
 	return ret;
 }
-- 
2.9.5

Re: [1/2] powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()

[Markus Elfring]

> The immr_node variable is still being used after the of_node_put() call,
> which may result in use-after-free.

Was any known source code analysis tool involved to point such
a questionable implementation detail out for further software
development considerations?

Regards,
Markus

[PATCH 2/2] powerpc/83xx: cleanup error paths in mpc831x_usb_cfg()

[Wen Yang]

Rename the jump labels according to the cleanup they perform,
and move reference handling to simplify cleanup.

Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
 arch/powerpc/platforms/83xx/usb.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 19dcef5..56b36fa 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -160,11 +160,9 @@ int mpc831x_usb_cfg(void)
 
 	/* Map USB SOC space */
 	ret = of_address_to_resource(np, 0, &res);
-	if (ret) {
-		of_node_put(immr_node);
-		of_node_put(np);
-		return ret;
-	}
+	if (ret)
+		goto out_put_node;
+
 	usb_regs = ioremap(res.start, resource_size(&res));
 
 	/* Using on-chip PHY */
@@ -173,7 +171,7 @@ int mpc831x_usb_cfg(void)
 		u32 refsel;
 
 		if (of_device_is_compatible(immr_node, "fsl,mpc8308-immr"))
-			goto out;
+			goto out_unmap;
 
 		if (of_device_is_compatible(immr_node, "fsl,mpc8315-immr"))
 			refsel = CONTROL_REFSEL_24MHZ;
@@ -200,8 +198,9 @@ int mpc831x_usb_cfg(void)
 		ret = -EINVAL;
 	}
 
-out:
+out_unmap:
 	iounmap(usb_regs);
+out_put_node:
 	of_node_put(immr_node);
 	of_node_put(np);
 	return ret;
-- 
2.9.5

Re: [PATCH 0/2] fix use-after-free in mpc831x_usb_cfg() and do some cleanups

[Markus Elfring]

> According to Markus's suggestion, split it into two small patches:

> https://lkml.org/lkml/2019/7/8/520

Thanks that you picked adjustment possibilities up from my feedback.
https://lore.kernel.org/lkml/99840e11-e0e6-b3f4-e35b-56ef4ec39417@web.de/


Now I wonder why you omitted message recipients from the cover letter.
Please keep the address lists usually complete also here for improvements
on the same source file in subsequent patch series.


Can a subject like “[PATCH 0/2] Fix mpc831x_usb_cfg()” be more succinct?


>  powerpc/83xx: fix use-after-free in mpc831x_usb_cfg()


This update variant is generally fine.
I would prefer to avoid the addition of function calls at two places
when the corresponding exception handling should be specified only once
at the end of such a function implementation.


>  powerpc/83xx: cleanup error paths in mpc831x_usb_cfg()

I would find it clearer to fix the error handling in the first update
step completely.
I guess that a renaming of the label “out” into “out_unmap” (or “unmap_io”?)
would be an auxiliary change for the second update step.


I am curious if different preferences for change combinations will trigger
further collateral evolution.

Regards,
Markus