2019-07-05 09:03:48

by Wen Yang

Subject: [PATCH] powerpc/83xx: fix use-after-free on mpc831x_usb_cfg()

The np variable is still being used after the of_node_put() call,
which may result in use-after-free.
We fix this issue by calling of_node_put() after the last usage.
This patatch also do some cleanup.

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: [email protected]
Cc: [email protected]
---
arch/powerpc/platforms/83xx/usb.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/usb.c b/arch/powerpc/platforms/83xx/usb.c
index 3d247d7..56b36fa 100644
--- a/arch/powerpc/platforms/83xx/usb.c
+++ b/arch/powerpc/platforms/83xx/usb.c
@@ -158,14 +158,11 @@ 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(np);
- return ret;
- }
+ if (ret)
+ goto out_put_node;
+
usb_regs = ioremap(res.start, resource_size(&res));

/* Using on-chip PHY */
@@ -174,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;
@@ -201,8 +198,10 @@ 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

2019-07-08 22:36:27

by Markus Elfring

Subject: Re: powerpc/83xx: fix use-after-free on mpc831x_usb_cfg()

> The np variable is still being used after the of_node_put() call,

> which may result in use-after-free.

> We fix this issue by calling of_node_put() after the last usage.


I imagine that this commit description can be improved a bit more
(by mentioning the influence of “immr_node”?).
How do you think about to omit the word “We” here?


> This patatch also do some cleanup.


Should the renaming of a jump label be contributed in a separate update
step of a small patch series besides a wording without a typo?

Regards,
Markus