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
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 <[email protected]>
Cc: Scott Wood <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Markus Elfring <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
Rename the jump labels according to the cleanup they perform,
and move reference handling to simplify cleanup.
Signed-off-by: Wen Yang <[email protected]>
Cc: Scott Wood <[email protected]>
Cc: Kumar Gala <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Markus Elfring <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
> 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/[email protected]/
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
> 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