@@ -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
From: Dan Carpenter <hidden> Date: 2021-09-28 09:27:38
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
From: Pavel Skripkin <hidden> Date: 2021-09-28 09:45:26
On 9/28/21 12:26, Dan Carpenter wrote:
On Tue, Sep 28, 2021 at 11:55:49AM +0300, Dan Carpenter wrote:
quoted
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
From: Dan Carpenter <hidden> Date: 2021-09-28 10:39:55
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:
From: Pavel Skripkin <hidden> Date: 2021-09-28 10:47:03
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.
quoted hunk
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:
@@ -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
From: Pavel Skripkin <hidden> Date: 2021-09-28 11:04:55
On 9/28/21 13:55, Dan Carpenter wrote:
On Tue, Sep 28, 2021 at 01:46:56PM +0300, Pavel Skripkin wrote:
quoted
On 9/28/21 13:39, Dan Carpenter wrote:
quoted
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
From: Pavel Skripkin <hidden> Date: 2021-09-28 11:09:13
On 9/28/21 14:06, Pavel Skripkin wrote:
quoted
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
From: Dan Carpenter <hidden> Date: 2021-09-28 11:31:50
On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
On 9/28/21 14:06, Pavel Skripkin wrote:
quoted
quoted
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
From: Pavel Skripkin <hidden> Date: 2021-09-28 11:45:47
On 9/28/21 14:30, Dan Carpenter wrote:
On Tue, Sep 28, 2021 at 02:09:06PM +0300, Pavel Skripkin wrote:
quoted
On 9/28/21 14:06, Pavel Skripkin wrote:
quoted
quoted
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
From: Dan Carpenter <hidden> Date: 2021-09-28 12:24:41
On Tue, Sep 28, 2021 at 02:45:39PM +0300, Pavel Skripkin wrote:
On 9/28/21 14:30, Dan Carpenter wrote:
quoted
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
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-28 12:58:19
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:
quoted
On 9/28/21 14:06, Pavel Skripkin wrote:
quoted
quoted
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!
@@ -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!
From: Dan Carpenter <hidden> Date: 2021-09-28 13:53:04
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
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-28 15:41:58
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:
quoted
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
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!
From: Dongliang Mu <hidden> Date: 2021-09-28 15:49:44
On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
[off-list ref] wrote:
On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
quoted
On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
quoted
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
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.
[Please note: This e-mail is from an EXTERNAL e-mail address]
On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
[off-list ref] wrote:
quoted
On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
quoted
On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
quoted
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
quoted
quoted
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.
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")