[PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[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: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>
---
 drivers/net/phy/mdio_bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 53f034fc2ef7..6f4b4e5df639 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -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;
 	}
 
-- 
2.27.0

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Andrew Lunn]

On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu 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
> 
> 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: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[patchwork-bot+netdevbpf]

Hello:

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

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
> 
> [...]

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu wrote:
> Once device_register() failed, we should call put_device() to
> decrement reference count for cleanup. Or it will cause memory
> leak.
> 

[ snipped stack trace ]

> 
> Reported-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>
> ---
>  drivers/net/phy/mdio_bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 53f034fc2ef7..6f4b4e5df639 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 11:55:49AM +0300, Dan Carpenter wrote:
> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 12:26, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 11:55:49AM +0300, Dan Carpenter wrote:
>> 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.
> 
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/20210927112017.19108-1-paskripkin@gmail.com/




With regards,
Pavel Skripkin

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

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:

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index ee8313a4ac71..c380a30a77bc 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 13:39, Dan Carpenter wrote:
> 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.

> 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:
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index ee8313a4ac71..c380a30a77bc 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 01:46:56PM +0300, Pavel Skripkin wrote:
> On 9/28/21 13:39, Dan Carpenter wrote:
> > 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 01:46:56PM +0300, Pavel Skripkin wrote:
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index ee8313a4ac71..c380a30a77bc 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -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
> 

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 13:55, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 01:46:56PM +0300, Pavel Skripkin wrote:
>> On 9/28/21 13:39, Dan Carpenter wrote:
>> > 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.
> 

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 13:59, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 01:46:56PM +0300, Pavel Skripkin wrote:
>> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> > index ee8313a4ac71..c380a30a77bc 100644
>> > --- a/drivers/net/phy/mdio_bus.c
>> > +++ b/drivers/net/phy/mdio_bus.c
>> > @@ -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
>> 
> 
> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 14:06, Pavel Skripkin wrote:
>> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
> On 9/28/21 14:06, Pavel Skripkin wrote:
> > > 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.
> 

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Pavel Skripkin]

On 9/28/21 14:30, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
>> On 9/28/21 14:06, Pavel Skripkin wrote:
>> > > 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.
>> 
> 
> 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 :)

> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 02:45:39PM +0300, Pavel Skripkin wrote:
> On 9/28/21 14:30, Dan Carpenter wrote:
> > 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)
> 
> ...
> 
> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Russell King (Oracle)]

On Tue, Sep 28, 2021 at 02:30:55PM +0300, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
> > On 9/28/21 14:06, Pavel Skripkin wrote:
> > > > 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.
> > 
> 
> 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.

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.

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Russell King (Oracle)]

On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu 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
> 
> 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: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
> Signed-off-by: Yanfei Xu <yanfei.xu@windriver.com>
> ---
>  drivers/net/phy/mdio_bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 53f034fc2ef7..6f4b4e5df639 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -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.

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dan Carpenter]

On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
>
> 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.

> 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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Russell King (Oracle)]

On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
> On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
> >
> > 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.
> 
> > 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.

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Dongliang Mu]

On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
> > On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
> > >
> > > 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.
> >
> > > 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.
>

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

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Xu, Yanfei]

On 9/28/21 11:48 PM, Dongliang Mu wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
>>
>> On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
>>> On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
>>>>
>>>> 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

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

Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Denis Efremov]

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")

>  drivers/net/phy/mdio_bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 53f034fc2ef7..6f4b4e5df639 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -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