net: mdiobus: Fix memory leak in __mdiobus_register

PATCH Applied Yanfei Xu 8 participants 23 messages Sep 26, 2021 – Sep 29, 2021

Subsystems (3)

Networking Network drivers Ethernet PHY

lore

Share & tools

lore

Yanfei Xu

author

2021-09-26 04:54 UTC #0 lore raw

PATCH 1/1 net: mdiobus: Fix memory leak in __mdiobus_register

Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.

BUG: memory leak
unreferenced object 0xffff888114032e00 (size 256):
  comm "kworker/1:3", pid 2960, jiffies 4294943572 (age 15.920s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 08 2e 03 14 81 88 ff ff  ................
    08 2e 03 14 81 88 ff ff 90 76 65 82 ff ff ff ff  .........ve.....
  backtrace:
    [<ffffffff8265cfab>] kmalloc include/linux/slab.h:591 [inline]
    [<ffffffff8265cfab>] kzalloc include/linux/slab.h:721 [inline]
    [<ffffffff8265cfab>] device_private_init drivers/base/core.c:3203 [inline]
    [<ffffffff8265cfab>] device_add+0x89b/0xdf0 drivers/base/core.c:3253
    [<ffffffff828dd643>] __mdiobus_register+0xc3/0x450 drivers/net/phy/mdio_bus.c:537
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969
    [<ffffffff82660916>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
    [<ffffffff8265cd0b>] device_add+0x5fb/0xdf0 drivers/base/core.c:3359
    [<ffffffff82c343b9>] usb_set_configuration+0x9d9/0xb90 drivers/usb/core/message.c:2170
    [<ffffffff82c4473c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238

BUG: memory leak
unreferenced object 0xffff888116f06900 (size 32):
  comm "kworker/0:2", pid 2670, jiffies 4294944448 (age 7.160s)
  hex dump (first 32 bytes):
    75 73 62 2d 30 30 31 3a 30 30 33 00 00 00 00 00  usb-001:003.....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff81484516>] kstrdup+0x36/0x70 mm/util.c:60
    [<ffffffff814845a3>] kstrdup_const+0x53/0x80 mm/util.c:83
    [<ffffffff82296ba2>] kvasprintf_const+0xc2/0x110 lib/kasprintf.c:48
    [<ffffffff82358d4b>] kobject_set_name_vargs+0x3b/0xe0 lib/kobject.c:289
    [<ffffffff826575f3>] dev_set_name+0x63/0x90 drivers/base/core.c:3147
    [<ffffffff828dd63b>] __mdiobus_register+0xbb/0x450 drivers/net/phy/mdio_bus.c:535
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969

Reported-by [email protected]

Signed-off-by Yanfei Xu <[email protected]>

---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/phy/mdio_bus.c +1 -0

index 53f034fc2ef7..6f4b4e5df639 100644
@@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
+		put_device(&bus->dev);
 		return -EINVAL;
 	}

Signature · 1 line

2.27.0

Andrew Lunn

maintainer Reviewed-by on v1 1/1

2021-09-26 15:34 UTC #1 lore raw

Quoted 61 lines · Yanfei Xu

Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.

BUG: memory leak
unreferenced object 0xffff888114032e00 (size 256):
  comm "kworker/1:3", pid 2960, jiffies 4294943572 (age 15.920s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 08 2e 03 14 81 88 ff ff  ................
    08 2e 03 14 81 88 ff ff 90 76 65 82 ff ff ff ff  .........ve.....
  backtrace:
    [<ffffffff8265cfab>] kmalloc include/linux/slab.h:591 [inline]
    [<ffffffff8265cfab>] kzalloc include/linux/slab.h:721 [inline]
    [<ffffffff8265cfab>] device_private_init drivers/base/core.c:3203 [inline]
    [<ffffffff8265cfab>] device_add+0x89b/0xdf0 drivers/base/core.c:3253
    [<ffffffff828dd643>] __mdiobus_register+0xc3/0x450 drivers/net/phy/mdio_bus.c:537
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969
    [<ffffffff82660916>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
    [<ffffffff8265cd0b>] device_add+0x5fb/0xdf0 drivers/base/core.c:3359
    [<ffffffff82c343b9>] usb_set_configuration+0x9d9/0xb90 drivers/usb/core/message.c:2170
    [<ffffffff82c4473c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238

BUG: memory leak
unreferenced object 0xffff888116f06900 (size 32):
  comm "kworker/0:2", pid 2670, jiffies 4294944448 (age 7.160s)
  hex dump (first 32 bytes):
    75 73 62 2d 30 30 31 3a 30 30 33 00 00 00 00 00  usb-001:003.....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff81484516>] kstrdup+0x36/0x70 mm/util.c:60
    [<ffffffff814845a3>] kstrdup_const+0x53/0x80 mm/util.c:83
    [<ffffffff82296ba2>] kvasprintf_const+0xc2/0x110 lib/kasprintf.c:48
    [<ffffffff82358d4b>] kobject_set_name_vargs+0x3b/0xe0 lib/kobject.c:289
    [<ffffffff826575f3>] dev_set_name+0x63/0x90 drivers/base/core.c:3147
    [<ffffffff828dd63b>] __mdiobus_register+0xbb/0x450 drivers/net/phy/mdio_bus.c:535
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969

Quoted 2 lines

Reported-by [email protected]
Signed-off-by Yanfei Xu <[email protected]>

Reviewed-by Andrew Lunn <[email protected]>

Andrew

[email protected]

CI Applied

2021-09-27 12:30 UTC #2 lore raw

In reply to Yanfei Xu

This patch was applied to netdev/net.git (refs/heads/master):

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

Quoted 34 lines · On Sun, 26 Sep 2021 12:53:13 +0800 you wrote:

Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.

BUG: memory leak
unreferenced object 0xffff888114032e00 (size 256):
  comm "kworker/1:3", pid 2960, jiffies 4294943572 (age 15.920s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 08 2e 03 14 81 88 ff ff  ................
    08 2e 03 14 81 88 ff ff 90 76 65 82 ff ff ff ff  .........ve.....
  backtrace:
    [<ffffffff8265cfab>] kmalloc include/linux/slab.h:591 [inline]
    [<ffffffff8265cfab>] kzalloc include/linux/slab.h:721 [inline]
    [<ffffffff8265cfab>] device_private_init drivers/base/core.c:3203 [inline]
    [<ffffffff8265cfab>] device_add+0x89b/0xdf0 drivers/base/core.c:3253
    [<ffffffff828dd643>] __mdiobus_register+0xc3/0x450 drivers/net/phy/mdio_bus.c:537
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969
    [<ffffffff82660916>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
    [<ffffffff8265cd0b>] device_add+0x5fb/0xdf0 drivers/base/core.c:3359
    [<ffffffff82c343b9>] usb_set_configuration+0x9d9/0xb90 drivers/usb/core/message.c:2170
    [<ffffffff82c4473c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238

Quoted 1 line

[...]

Here is the summary with links:
- net: mdiobus: Fix memory leak in __mdiobus_register

    https://git.kernel.org/netdev/net/c/ab609f25d198

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Dan Carpenter

2021-09-28 08:56 UTC #3 lore raw

In reply to Yanfei Xu

Quoted 4 lines · Yanfei Xu

Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.

[ snipped stack trace ]

Quoted 12 lines

Reported-by [email protected]
Signed-off-by Yanfei Xu <[email protected]>
---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/phy/mdio_bus.c +0 -0
index 53f034fc2ef7..6f4b4e5df639 100644

drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner) · line 540

Quoted 4 lines

drivers/net/phy/mdio_bus.c +1 -0

@@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
+		put_device(&bus->dev);

No this isn't right. The dev.class is &mdio_bus_class. It's set a few lines earlier.

	bus->dev.class = &mdio_bus_class;

The release function is mdiobus_release(). It will free bus and lead to use after frees in the callers. Look at greth_mdio_init(). There are a lot of callers which will crash now.

This patch was a clear layering violation. If you didn't allocate "bus" then you should not free it. Keeping that rule in mind can help prevent future bugs. Also he other error handling paths are careful not to call put_device() so why would this one be special? It should have stood out that this one error path is the only place that doesn't use a goto to clean up.

I don't have a solution. I have commented before that I hate kobjects for this reason because they lead to unfixable memory leaks during probe. But this leak will only happen with fault injection and so it doesn't affect real life. And even if it did, a leak it preferable to a crash.

Unfortunately, this patch has already been applied so please can you send a patch to revert it?

regards,
dan carpenter

Dan Carpenter

2021-09-28 09:27 UTC #4 lore raw

Quoted 5 lines · Dan Carpenter

I don't have a solution. I have commented before that I hate kobjects for this reason because they lead to unfixable memory leaks during probe. But this leak will only happen with fault injection and so it doesn't affect real life. And even if it did, a leak it preferable to a crash.

The fix for this should have gone in devm_of_mdiobus_register() but it's quite tricky.

drivers/net/phy/mdio_devres.c
106 int devm_of_mdiobus_register(struct device *dev, struct mii_bus *mdio,
107 struct device_node *np)
108 {
109 struct mdiobus_devres *dr;
110 int ret;
111
112 if (WARN_ON(!devres_find(dev, devm_mdiobus_free,
113 mdiobus_devres_match, mdio)))
114 return -EINVAL;

This leaks the bus. Fix this leak by calling mdiobus_release(mdio);

   115  
   116          dr = devres_alloc(devm_mdiobus_unregister, sizeof(*dr), GFP_KERNEL);
   117          if (!dr)
   118                  return -ENOMEM;

Fix this path by calling mdiobus_release(mdio);

   119  
   120          ret = of_mdiobus_register(mdio, np);
   121          if (ret) {

Ideally here we can could call device_put(mdio), but that won't work for the one error path that occurs before device_initialize(). /* Do not continue if the node is disabled */.

Maybe the code could be modified to call device_initialize() on the error path? Sort of ugly but it would work.

   122                  devres_free(dr);
   123                  return ret;
   124          }
   125  
   126          dr->mii = mdio;
   127          devres_add(dev, dr);
   128          return 0;
   129  }

Then audit the callers, and there is only one which references the mdio_bus after devm_of_mdiobus_register() fails. It's realtek_smi_setup_mdio(). Modify that debug statement.

regards,
dan carpenter

Pavel Skripkin

2021-09-28 09:45 UTC #5 lore raw

Quoted 50 lines · Dan Carpenter

Quoted 5 lines · Dan Carpenter
I don't have a solution. I have commented before that I hate kobjects for this reason because they lead to unfixable memory leaks during probe. But this leak will only happen with fault injection and so it doesn't affect real life. And even if it did, a leak it preferable to a crash.
The fix for this should have gone in devm_of_mdiobus_register() but it's quite tricky.
drivers/net/phy/mdio_devres.c
    106  int devm_of_mdiobus_register(struct device *dev, struct mii_bus *mdio,
    107                               struct device_node *np)
    108  {
    109          struct mdiobus_devres *dr;
    110          int ret;
    111
    112          if (WARN_ON(!devres_find(dev, devm_mdiobus_free,
    113                                   mdiobus_devres_match, mdio)))
    114                  return -EINVAL;
This leaks the bus. Fix this leak by calling mdiobus_release(mdio);
    115
    116          dr = devres_alloc(devm_mdiobus_unregister, sizeof(*dr), GFP_KERNEL);
    117          if (!dr)
    118                  return -ENOMEM;
Fix this path by calling mdiobus_release(mdio);
    119
    120          ret = of_mdiobus_register(mdio, np);
    121          if (ret) {
Ideally here we can could call device_put(mdio), but that won't work for the one error path that occurs before device_initialize(). /* Do not continue if the node is disabled */.
Maybe the code could be modified to call device_initialize() on the error path? Sort of ugly but it would work.
    122                  devres_free(dr);
    123                  return ret;
    124          }
    125
    126          dr->mii = mdio;
    127          devres_add(dev, dr);
    128          return 0;
    129  }

Quoted 4 lines

Then audit the callers, and there is only one which references the mdio_bus after devm_of_mdiobus_register() fails. It's realtek_smi_setup_mdio(). Modify that debug statement.

Thank you, Dan, for analysis, and it sounds reasonable to me.

Back to bug reported by syzbot: error happened in other place:

int __mdiobus_register(struct mii_bus *bus, struct module *owner) {
....

	phydev = mdiobus_scan(bus, i);		<-- here
	if (IS_ERR(phydev) && (PTR_ERR(phydev) != -ENODEV)) {
		err = PTR_ERR(phydev);
		goto error;
	}

....
}

(You can take a look at the log [1] you won't find error message about mii_bus registration failure. I found this place while debugging locally)

So, Yanfei's patch is completely unrelated to bug reported by syzkaller and Reported-by tag is also wrong.

Can you, please, take a look at [2]. I think, I found the root case of the reported bug. Thank you :)

[1] https://syzkaller.appspot.com/text?tag=CrashLog&x=131c754b300000

[2] https://lore.kernel.org/lkml/[email protected]/

With regards,
Pavel Skripkin

Dan Carpenter

2021-09-28 10:39 UTC #6 lore raw

No, the syzbot link was correct.

You gave me that link again but I think you must be complaining about a different bug which involves mdiobus_free(). Your bug is something like this:

drivers/staging/netlogic/xlr_net.c
838 err = mdiobus_register(priv->mii_bus);
839 if (err) {
840 mdiobus_free(priv->mii_bus);

This error path will leak.

   841                  pr_err("mdio bus registration failed\n");
   842                  return err;
   843          }

Your patch is more complicated than necessary... Just do:

drivers/net/phy/mdio_bus.c +1 -0

index ee8313a4ac71..c380a30a77bc 100644
@@ -538,6 +538,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	bus->dev.groups = NULL;
 	dev_set_name(&bus->dev, "%s", bus->id);
 
+	bus->state = MDIOBUS_UNREGISTERED;
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);

regards,
dan carpenter

Pavel Skripkin

2021-09-28 10:47 UTC #7 lore raw

Quoted 2 lines · Dan Carpenter

No, the syzbot link was correct.

Link is correct, but Yanfei's patch does not fix this bug. Syzbot reported leak, that you described below, not the Yanfei one.

Quoted 24 lines

You gave me that link again but I think you must be complaining about a different bug which involves mdiobus_free(). Your bug is something like this:
drivers/staging/netlogic/xlr_net.c
    838          err = mdiobus_register(priv->mii_bus);
    839          if (err) {
    840                  mdiobus_free(priv->mii_bus);
This error path will leak.
    841                  pr_err("mdio bus registration failed\n");
    842                  return err;
    843          }
Your patch is more complicated than necessary... Just do:
drivers/net/phy/mdio_bus.c +0 -0
index ee8313a4ac71..c380a30a77bc 100644

drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner) · line 544

Quoted 7 lines

drivers/net/phy/mdio_bus.c +1 -0

@@ -538,6 +538,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
  	bus->dev.groups = NULL;
  	dev_set_name(&bus->dev, "%s", bus->id);
 
+	bus->state = MDIOBUS_UNREGISTERED;
  	err = device_register(&bus->dev);
  	if (err) {
  		pr_err("mii_bus %s failed to register\n", bus->id);

yep, it's the same as mine, but I thought, that MDIOBUS_UNREGISTERED is not correct name for this state :) Anyway, thank you for suggestion

With regards,
Pavel Skripkin

Dan Carpenter

2021-09-28 10:55 UTC #8 lore raw

Quoted 7 lines · Pavel Skripkin

Quoted 2 lines · Dan Carpenter
No, the syzbot link was correct.
Link is correct, but Yanfei's patch does not fix this bug. Syzbot reported leak, that you described below, not the Yanfei one.

I promise you that Yanfei's link was correct. That bug was in __devm_mdiobus_register(). It's a totally separate issue.

regards,
dan carpenter

Pavel Skripkin

2021-09-28 11:04 UTC #9 lore raw

Quoted 5 lines · Dan Carpenter

Quoted 4 lines · Pavel Skripkin
Quoted 2 lines · Dan Carpenter
No, the syzbot link was correct.

Quoted 7 lines

Quoted 3 lines
Link is correct, but Yanfei's patch does not fix this bug. Syzbot reported leak, that you described below, not the Yanfei one.
I promise you that Yanfei's link was correct. That bug was in __devm_mdiobus_register(). It's a totally separate issue.

I must be missing something, or we are talking about different links :)

Let me explain why I think, that Yanfei's patch cannot fix leak reported by syzkaller [1] (I hope, we are talking about this link)

Yanfei has changed this code part:

	err = device_register(&bus->dev);
	if (err) {
(*)		pr_err("mii_bus %s failed to register\n", bus->id);
		return -EINVAL;
	}

So, if executing gets into this branch we should see error message (*), right? There is no such message into log file on bug report page [1], so how is it possible?

[1] https://syzkaller.appspot.com/bug?id=fa99459691911a0369622248e0f4e3285fcedd97

With regards,
Pavel Skripkin

Dan Carpenter

2021-09-28 11:00 UTC #10 lore raw

In reply to Pavel Skripkin

Quoted 7 lines · Pavel Skripkin

Quoted 7 lines

drivers/net/phy/mdio_bus.c +0 -0

index ee8313a4ac71..c380a30a77bc 100644
@@ -538,6 +538,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
  	bus->dev.groups = NULL;
  	dev_set_name(&bus->dev, "%s", bus->id);

Quoted 9 lines

drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner)
Quoted 6 lines
drivers/net/phy/mdio_bus.c +1 -0
+	bus->state = MDIOBUS_UNREGISTERED;
  	err = device_register(&bus->dev);
  	if (err) {
  		pr_err("mii_bus %s failed to register\n", bus->id);
 
yep, it's the same as mine, but I thought, that MDIOBUS_UNREGISTERED is not correct name for this state :) Anyway, thank you for suggestion

It's not actually the same. The state has to be set before the device_register() or there is still a leak.

regards,
dan carpenter

Pavel Skripkin

2021-09-28 11:06 UTC #11 lore raw

Quoted 18 lines · Dan Carpenter

Quoted 7 lines · Pavel Skripkin
Quoted 7 lines
drivers/net/phy/mdio_bus.c +0 -0
index ee8313a4ac71..c380a30a77bc 100644
@@ -538,6 +538,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
  	bus->dev.groups = NULL;
  	dev_set_name(&bus->dev, "%s", bus->id);
Quoted 9 lines
drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner)
Quoted 6 lines
drivers/net/phy/mdio_bus.c +1 -0
+	bus->state = MDIOBUS_UNREGISTERED;
  	err = device_register(&bus->dev);
  	if (err) {
  		pr_err("mii_bus %s failed to register\n", bus->id);
 
yep, it's the same as mine, but I thought, that MDIOBUS_UNREGISTERED is not correct name for this state :) Anyway, thank you for suggestion

Quoted 3 lines

It's not actually the same. The state has to be set before the device_register() or there is still a leak.

Ah, I see... I forgot to handle possible device_register() error. Will send v2 soon, thank you

With regards,
Pavel Skripkin

Pavel Skripkin

2021-09-28 11:09 UTC #12 lore raw

Quoted 8 lines · Pavel Skripkin

Quoted 3 lines
It's not actually the same. The state has to be set before the device_register() or there is still a leak.
Ah, I see... I forgot to handle possible device_register() error. Will send v2 soon, thank you

Wait... Yanfei's patch is already applied to net tree and if I understand correctly, calling put_device() 2 times will cause UAF or smth else.

With regards,
Pavel Skripkin

Dan Carpenter

2021-09-28 11:31 UTC #13 lore raw

Quoted 4 lines · Pavel Skripkin

Quoted 3 lines · Pavel Skripkin
Quoted 3 lines
It's not actually the same. The state has to be set before the device_register() or there is still a leak.

Quoted 8 lines

Quoted 5 lines
Ah, I see... I forgot to handle possible device_register() error. Will send v2 soon, thank you
Wait... Yanfei's patch is already applied to net tree and if I understand correctly, calling put_device() 2 times will cause UAF or smth else.

Yes. It causes a UAF.

Huh... You're right that the log should say "failed to register". But I don't think that's the correct syzbot link for your patch either because I don't think anyone calls mdiobus_free() if __devm_mdiobus_register() fails. I have looked at these callers. It would be a bug as well.

Anyway, your patch is required and the __devm_mdiobus_register() function has leaks as well. And perhaps there are more bugs we have not discovered.

regards,
dan carpenter

Pavel Skripkin

2021-09-28 11:45 UTC #14 lore raw

Quoted 14 lines · Dan Carpenter

Quoted 4 lines · Pavel Skripkin
Quoted 3 lines · Pavel Skripkin
Quoted 3 lines
It's not actually the same. The state has to be set before the device_register() or there is still a leak.
Quoted 8 lines
Quoted 5 lines
Ah, I see... I forgot to handle possible device_register() error. Will send v2 soon, thank you
Wait... Yanfei's patch is already applied to net tree and if I understand correctly, calling put_device() 2 times will cause UAF or smth else.

Quoted 8 lines

Yes. It causes a UAF.
Huh... You're right that the log should say "failed to register". But I don't think that's the correct syzbot link for your patch either because I don't think anyone calls mdiobus_free() if __devm_mdiobus_register() fails. I have looked at these callers. It would be a bug as well.

mdiobus_free() is called in case of ->probe() failure, because devres clean up function for bus is devm_mdiobus_free(). It simply calls mdiobus_free().

So, i imagine following calltrace:

ax88772_bind
ax88772_init_mdio
devm_mdiobus_alloc() <- bus registered as devres devm_mdiobus_register() <- fail (->probe failure)

...

devres_release_all
mdiobus_free()

Also, syzbot has tested my patch :)

Quoted 7 lines

Anyway, your patch is required and the __devm_mdiobus_register() function has leaks as well. And perhaps there are more bugs we have not discovered.
regards,
dan carpenter

With regards,
Pavel Skripkin

Dan Carpenter

2021-09-28 12:24 UTC #15 lore raw

Quoted 21 lines · Pavel Skripkin

Quoted 8 lines · Dan Carpenter
On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
Huh... You're right that the log should say "failed to register". But I don't think that's the correct syzbot link for your patch either because I don't think anyone calls mdiobus_free() if __devm_mdiobus_register() fails. I have looked at these callers. It would be a bug as well.
mdiobus_free() is called in case of ->probe() failure, because devres clean up function for bus is devm_mdiobus_free(). It simply calls mdiobus_free().
So, i imagine following calltrace:
ax88772_bind
ax88772_init_mdio
devm_mdiobus_alloc() <- bus registered as devres devm_mdiobus_register() <- fail (->probe failure)

Quoted 4 lines

...
devres_release_all
mdiobus_free()

Argh... Crap. You're right. There is just one bug. No need to change __devm_mdiobus_register() and trying to do that would lead to a UAF.

Your patch is the correct fix but with the modifications we discussed.

regards,
dan carpenter

Russell King (Oracle)

reviewer

2021-09-28 12:58 UTC #16 lore raw

In reply to Dan Carpenter

Quoted 22 lines · Dan Carpenter

Quoted 4 lines · Pavel Skripkin
Quoted 3 lines · Pavel Skripkin
Quoted 3 lines
It's not actually the same. The state has to be set before the device_register() or there is still a leak.
Quoted 8 lines
Quoted 5 lines
Ah, I see... I forgot to handle possible device_register() error. Will send v2 soon, thank you
Wait... Yanfei's patch is already applied to net tree and if I understand correctly, calling put_device() 2 times will cause UAF or smth else.
Yes. It causes a UAF.
Huh... You're right that the log should say "failed to register". But I don't think that's the correct syzbot link for your patch either because I don't think anyone calls mdiobus_free() if __devm_mdiobus_register() fails. I have looked at these callers. It would be a bug as well.

Quoted 3 lines

Anyway, your patch is required and the __devm_mdiobus_register() function has leaks as well. And perhaps there are more bugs we have not discovered.

This thread seems to be getting out of hand.

Going back to the start of the thread, the commit message contains a stack trace, and in that stack trace is ax88772_init_mdio(), which is in drivers/net/usb/asix_devices.c. This function does:

        priv->mdio = devm_mdiobus_alloc(&dev->udev->dev);
	...
	return devm_mdiobus_register(&dev->udev->dev, priv->mdio);

If devm_mdiobus_register() and we unwind the devm resources, then we will call the registered free method for devm_mdiobus_alloc(), which is devm_mdiobus_free(). This will call mdiobus_free().

Firstly, the driver is correct in what it is doing - using the devm_* functions it doesn't get a choice about how the cleanup happens.

The problem appears to be:

- bus->state is MDIOBUS_ALLOCATED
- we call into __mdiobus_register()
- device_register() succeeds
- devm_gpiod_get_optional() returns an error code
- device_del(&bus->dev) undoes _part_ of the device_register()
- we do not update bus->state to MDIOBUS_UNREGISTERED

We *must* to the last step, because we haven't finished undoing the effects of registering the bus device with the driver model by causing the device to be properly released by the driver model - that being that dev->p has been allocated and its name has been allocated and set.

device_del() does _not_ undo those allocations. Only the very last put_device() does.

mdiobus_free() decides whether it can simply free the device or whether it needs to use put_device() depending on bus->state - if bus->state is MDIOBUS_ALLOCATED, that means the bus has _never_ been registered with the driver model, and it is safe to kfree() it. If it is MDIOBUS_UNREGISTERED, then that means the device has been registered and needs put_device() to be called on it.

So, I would suggest a simple fix is to set bus->state to MDIOBUS_UNREGISTERED immediately _after_ the successful device_register() call with a comment that it is updated later in the function - or to set bus->state to MDIOBUS_UNREGISTERED immediately before the call to device_del() in __mdiobus_register().

A better fix would be to sort out the mess here and make __mdiobus_register() respect the "get resources and setup first before you register anything" rule.

In other words, initialise the mutexes and get the reset GPIO _before_ registering the bus with the driver model. That will cut down on the uevent noise to userspace if the gpio defers and also simplify some of the problem here - we then end up with one path where we call device_del() in MDIOBUS_UNREGISTERED, rather than two.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Dan Carpenter

2021-09-28 13:53 UTC #17 lore raw

Quoted 2 lines · Russell King (Oracle)

This thread seems to be getting out of hand.

The thread was closed. We need to revert Yanfei's patch and apply Pavel's patch. He's going to resend.

Quoted 3 lines

So, I would suggest a simple fix is to set bus->state to MDIOBUS_UNREGISTERED immediately _after_ the successful device_register().

Not after. It has to be set to MDIOBUS_UNREGISTERED if device_register() fails, otherwise there will still be a leak.

Adding a comment is a good idea.

regards,
dan carpenter

Russell King (Oracle)

reviewer

2021-09-28 15:41 UTC #18 lore raw

Quoted 7 lines · Dan Carpenter

Quoted 2 lines · Russell King (Oracle)
This thread seems to be getting out of hand.
The thread was closed. We need to revert Yanfei's patch and apply Pavel's patch. He's going to resend.

Quoted 6 lines

Quoted 3 lines
So, I would suggest a simple fix is to set bus->state to MDIOBUS_UNREGISTERED immediately _after_ the successful device_register().
Not after. It has to be set to MDIOBUS_UNREGISTERED if device_register() fails, otherwise there will still be a leak.

Ah yes, you are correct - the device name may not be freed. Also...

* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up your
* reference instead.

So yes, we must set to MDIOBUS_UNREGISTERED even if device_register() fails.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Dongliang Mu

2021-09-28 15:49 UTC #19 lore raw

Quoted 16 lines · Russell King (Oracle)

Quoted 7 lines · Dan Carpenter
Quoted 2 lines · Russell King (Oracle)
This thread seems to be getting out of hand.
The thread was closed. We need to revert Yanfei's patch and apply Pavel's patch. He's going to resend.
Quoted 6 lines
Quoted 3 lines
So, I would suggest a simple fix is to set bus->state to MDIOBUS_UNREGISTERED immediately _after_ the successful device_register().
Not after. It has to be set to MDIOBUS_UNREGISTERED if device_register() fails, otherwise there will still be a leak.

Quoted 9 lines

Ah yes, you are correct - the device name may not be freed. Also...
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up your
* reference instead.
So yes, we must set to MDIOBUS_UNREGISTERED even if device_register() fails.

So we have reached an agreement. Pavel's patch fixes the syzbot link [1], other than Yanfei's patch. However, Yanfei's patch also fixes another memory link nearby.

Right?

[1] https://syzkaller.appspot.com/bug?id=fa99459691911a0369622248e0f4e3285fcedd97

Quoted 3 lines

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Xu, Yanfei

author

2021-09-29 02:06 UTC #20 lore raw

Quoted 13 lines · Dongliang Mu

[Please note: This e-mail is from an EXTERNAL e-mail address]
Quoted 9 lines · Russell King (Oracle)
Quoted 7 lines · Dan Carpenter
Quoted 2 lines · Russell King (Oracle)
This thread seems to be getting out of hand.
The thread was closed. We need to revert Yanfei's patch and apply Pavel's patch. He's going to resend.

Sorry for my patch, it is my bad. :( And thanks for Pavel's v2 which help to revert mine.

Regards,
Yanfei

Quoted 28 lines

Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
So, I would suggest a simple fix is to set bus->state to MDIOBUS_UNREGISTERED immediately _after_ the successful device_register().
Not after. It has to be set to MDIOBUS_UNREGISTERED if device_register() fails, otherwise there will still be a leak.
Quoted 9 lines
Ah yes, you are correct - the device name may not be freed. Also...
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up your
* reference instead.
So yes, we must set to MDIOBUS_UNREGISTERED even if device_register() fails.
So we have reached an agreement. Pavel's patch fixes the syzbot link [1], other than Yanfei's patch. However, Yanfei's patch also fixes another memory link nearby.
Right?
[1] https://syzkaller.appspot.com/bug?id=fa99459691911a0369622248e0f4e3285fcedd97
Quoted 3 lines
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Russell King (Oracle)

reviewer

2021-09-28 13:11 UTC #21 lore raw

In reply to Yanfei Xu

Quoted 72 lines · Yanfei Xu

Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.

BUG: memory leak
unreferenced object 0xffff888114032e00 (size 256):
  comm "kworker/1:3", pid 2960, jiffies 4294943572 (age 15.920s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 08 2e 03 14 81 88 ff ff  ................
    08 2e 03 14 81 88 ff ff 90 76 65 82 ff ff ff ff  .........ve.....
  backtrace:
    [<ffffffff8265cfab>] kmalloc include/linux/slab.h:591 [inline]
    [<ffffffff8265cfab>] kzalloc include/linux/slab.h:721 [inline]
    [<ffffffff8265cfab>] device_private_init drivers/base/core.c:3203 [inline]
    [<ffffffff8265cfab>] device_add+0x89b/0xdf0 drivers/base/core.c:3253
    [<ffffffff828dd643>] __mdiobus_register+0xc3/0x450 drivers/net/phy/mdio_bus.c:537
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969
    [<ffffffff82660916>] bus_probe_device+0xc6/0xe0 drivers/base/bus.c:487
    [<ffffffff8265cd0b>] device_add+0x5fb/0xdf0 drivers/base/core.c:3359
    [<ffffffff82c343b9>] usb_set_configuration+0x9d9/0xb90 drivers/usb/core/message.c:2170
    [<ffffffff82c4473c>] usb_generic_driver_probe+0x8c/0xc0 drivers/usb/core/generic.c:238

BUG: memory leak
unreferenced object 0xffff888116f06900 (size 32):
  comm "kworker/0:2", pid 2670, jiffies 4294944448 (age 7.160s)
  hex dump (first 32 bytes):
    75 73 62 2d 30 30 31 3a 30 30 33 00 00 00 00 00  usb-001:003.....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff81484516>] kstrdup+0x36/0x70 mm/util.c:60
    [<ffffffff814845a3>] kstrdup_const+0x53/0x80 mm/util.c:83
    [<ffffffff82296ba2>] kvasprintf_const+0xc2/0x110 lib/kasprintf.c:48
    [<ffffffff82358d4b>] kobject_set_name_vargs+0x3b/0xe0 lib/kobject.c:289
    [<ffffffff826575f3>] dev_set_name+0x63/0x90 drivers/base/core.c:3147
    [<ffffffff828dd63b>] __mdiobus_register+0xbb/0x450 drivers/net/phy/mdio_bus.c:535
    [<ffffffff828cb835>] __devm_mdiobus_register+0x75/0xf0 drivers/net/phy/mdio_devres.c:87
    [<ffffffff82b92a00>] ax88772_init_mdio drivers/net/usb/asix_devices.c:676 [inline]
    [<ffffffff82b92a00>] ax88772_bind+0x330/0x480 drivers/net/usb/asix_devices.c:786
    [<ffffffff82baa33f>] usbnet_probe+0x3ff/0xdf0 drivers/net/usb/usbnet.c:1745
    [<ffffffff82c36e17>] usb_probe_interface+0x177/0x370 drivers/usb/core/driver.c:396
    [<ffffffff82661d17>] call_driver_probe drivers/base/dd.c:517 [inline]
    [<ffffffff82661d17>] really_probe.part.0+0xe7/0x380 drivers/base/dd.c:596
    [<ffffffff826620bc>] really_probe drivers/base/dd.c:558 [inline]
    [<ffffffff826620bc>] __driver_probe_device+0x10c/0x1e0 drivers/base/dd.c:751
    [<ffffffff826621ba>] driver_probe_device+0x2a/0x120 drivers/base/dd.c:781
    [<ffffffff82662a26>] __device_attach_driver+0xf6/0x140 drivers/base/dd.c:898
    [<ffffffff8265eca7>] bus_for_each_drv+0xb7/0x100 drivers/base/bus.c:427
    [<ffffffff826625a2>] __device_attach+0x122/0x260 drivers/base/dd.c:969

Reported-by [email protected]

Signed-off-by Yanfei Xu <[email protected]>

---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/phy/mdio_bus.c +0 -0

index 53f034fc2ef7..6f4b4e5df639 100644

drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner) · line 542

Quoted 6 lines

drivers/net/phy/mdio_bus.c +1 -0

@@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
+		put_device(&bus->dev);
 		return -EINVAL;
 	}

This patch is incorrect:

1) the reported failure does not involve this path.
2) device_register() failing does not need a put_device() because the contained device_add() undoes everything that it attempted to do.

The above backtraces occur because we have had a successful device_register() fall, but later call device_del() and then kfree() the mdiobus, which has an embedded the struct device that has pointers to memory that has not been cleaned up - because kfree() is the wrong way to handle this.

bus->state needs to be set to indicate that the embedded struct device has been registered but no longer is registered if we fail after device_register() has been called.

If device_register() fails, then there is no problem.

Signature · 2 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Denis Efremov

2021-09-29 21:34 UTC #22 lore raw

In reply to Yanfei Xu

Hi,

Just want to note that highly likely this patch reintroduces
CVE-2019-12819 that was fixed in commit
6ff7b060535e ("mdio_bus: Fix use-after-free on device_register fails")
and added by the same patch in commit
0c692d07842a ("drivers/net/phy/mdio_bus.c: call put_device on device_register() failure")

Quoted 8 lines

 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

drivers/net/phy/mdio_bus.c +0 -0

index 53f034fc2ef7..6f4b4e5df639 100644

drivers/net/phy/mdio_bus.c int __mdiobus_register(struct mii_bus *bus, struct module *owner) · line 542

Quoted 6 lines

drivers/net/phy/mdio_bus.c +1 -0

@@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);
+		put_device(&bus->dev);
 		return -EINVAL;
 	}

Thanks,
Denis