Skip to contentSkip to conversation

bus: vexpress-config: fix device reference leak

PATCH New Johan Hovold 4 participants 5 messages Nov 1, 2016 – Nov 16, 2016 Arm/versatile express platform

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 bus: vexpress-config: fix device reference leak

Make sure to drop the reference to the parent device taken by class_find_device() after populating the bus.

Fixes 3b9334ac835b ("mfd: vexpress: Convert custom func API to regmap")
---
 drivers/bus/vexpress-config.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/bus/vexpress-config.c +6 -1
index 9efdf1de4035..493e7b9fc813 100644
@@ -171,6 +171,7 @@ static int vexpress_config_populate(struct device_node *node)
 {
 	struct device_node *bridge;
 	struct device *parent;
+	int ret;
 
 	bridge = of_parse_phandle(node, "arm,vexpress,config-bridge", 0);
 	if (!bridge)
@@ -182,7 +183,11 @@ static int vexpress_config_populate(struct device_node *node)
 	if (WARN_ON(!parent))
 		return -ENODEV;
 
-	return of_platform_populate(node, NULL, NULL, parent);
+	ret = of_platform_populate(node, NULL, NULL, parent);
+
+	put_device(parent);
+
+	return ret;
 }
 
 static int __init vexpress_config_init(void)
Signature · 1 line
2.7.3
#1 lore raw

PATCH 1/1 bus: vexpress-config: fix device reference leak

Quoted 6 lines · Johan Hovold

Make sure to drop the reference to the parent device taken by class_find_device() after populating the bus.

Fixes 3b9334ac835b ("mfd: vexpress: Convert custom func API to

regmap")

You're right. May I ask how did you figure it out? The get_device() happening in class_find_device() is a bit obscure, so have you simply followed places where it's being used or used some static (?) analysis tool? If the latter, I'd be very curios to hear what was it :-)

Thanks!

Pawel

#2 lore raw

PATCH 1/1 bus: vexpress-config: fix device reference leak

Quoted 10 lines · Pawel Moll
Quoted 6 lines · Johan Hovold

Make sure to drop the reference to the parent device taken by class_find_device() after populating the bus.

Fixes 3b9334ac835b ("mfd: vexpress: Convert custom func API to

regmap")

You're right. May I ask how did you figure it out? The get_device() happening in class_find_device() is a bit obscure,

It's not obscure at all - all the functions that find a device do so under a lock to ensure that the device does not go away, and they take a reference count on the device before returning the pointer for exactly the same reason.

If they didn't do that, the find function could locate a struct device while another thread is deleting the struct device, and it would then return a stale pointer - and dereferencing that pointer would then be a use-after-free bug.

So not obscure, but rather fundamentally necessary.

Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#3 lore raw

PATCH 1/1 bus: vexpress-config: fix device reference leak

In reply to Pawel Moll
Quoted 4 lines · Pawel Moll
Quoted 3 lines · Johan Hovold

Make sure to drop the reference to the parent device taken by class_find_device() after populating the bus.

Quoted 8 lines
Quoted 3 lines
Fixes 3b9334ac835b ("mfd: vexpress: Convert custom func API to

regmap")

You're right. May I ask how did you figure it out? The get_device() happening in class_find_device() is a bit obscure, so have you simply followed places where it's being used or used some static (?) analysis tool? If the latter, I'd be very curios to hear what was it :-)

I stumbled over one of these leaks and grepped and searched for more. I've submitted a few patches this week fixing some of the more obvious ones, but there are more lurking behind various subsystem wrappers.

Thanks,
Johan

#4 lore raw

PATCH 1/1 bus: vexpress-config: fix device reference leak

In reply to Johan Hovold
Quoted 3 lines · Johan Hovold

Make sure to drop the reference to the parent device taken by class_find_device() after populating the bus.

Thanks for the fix. I somehow missed this patch, sorry for that. Not sure if arm-soc guys take this as a fix for v4.9, and I have already sent PR for v4.10. I will repost this along with the ack and check with them.

Signature · 2 lines
Regards,
Sudeep