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(+)
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
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: [email protected]
> Signed-off-by: Yanfei Xu <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
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
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: [email protected]
> Signed-off-by: Yanfei Xu <[email protected]>
> ---
> 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
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
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/[email protected]/
With regards,
Pavel Skripkin
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
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
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
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
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
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
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
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
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
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
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!
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: [email protected]
> Signed-off-by: Yanfei Xu <[email protected]>
> ---
> 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!
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
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!
On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
<[email protected]> 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!
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)
> <[email protected]> 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!
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