'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Remove the incorrect 'mdiobus_free' in the remove function.
Fixes: 379d7ac7ca31 ("phy: mdio-thunder: Add driver for Cavium Thunder SoC MDIO buses.")
Signed-off-by: Christophe JAILLET <redacted>
---
drivers/net/mdio/mdio-thunder.c | 1 -
1 file changed, 1 deletion(-)
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-05-12 22:23:13
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Remove the incorrect 'mdiobus_free' in the remove function.
Yes, this looks correct, thanks.
Reviewed-by: Russell King <redacted>
However, there's another issue in this driver that ought to be fixed.
If devm_mdiobus_alloc_size() succeeds, but of_mdiobus_register() fails,
we continue on to the next bus (which I think is reasonable.) We don't
free the bus.
When we come to the remove method however, we will call
mdiobus_unregister() on this existent but not-registered bus. Surely we
don't want to do that?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-05-12 22:23:13
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Hi Christophe
[PATCH] net: mdio: Fix a double free issue in the .remove function
Please indicate in the subject which mdio bus driver has a double
free.
Also, octeon_mdiobus_remove() appears to have the same problem.
Andrew
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
quoted
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Hi Christophe
[PATCH] net: mdio: Fix a double free issue in the .remove function
Please indicate in the subject which mdio bus driver has a double
free.
Ok, will do.
But looking at [1], it was not not self-explanatory that it was the rule
here :)
Also, octeon_mdiobus_remove() appears to have the same problem.
In fact, even a little worse. It also calls 'mdiobus_free()' in the
error handling path of the probe (which is why my coccinelle script
didn't spot it. It looks for discrepancy between error handling path in
the probe and the remove function. If both are wrong, it looks safe :) )
I'll send another patch for this driver.
CJ
Le 12/05/2021 à 23:44, Russell King - ARM Linux admin a écrit :
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
quoted
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Remove the incorrect 'mdiobus_free' in the remove function.
Yes, this looks correct, thanks.
Reviewed-by: Russell King <redacted>
However, there's another issue in this driver that ought to be fixed.
If devm_mdiobus_alloc_size() succeeds, but of_mdiobus_register() fails,
we continue on to the next bus (which I think is reasonable.) We don't
free the bus.
When we come to the remove method however, we will call
mdiobus_unregister() on this existent but not-registered bus. Surely we
don't want to do that?
Hmmm, I don't agree here.
'nexus' is 'kzalloc()'ed.
So the pointers in 'buses[]' are all NULL by default.
We set 'nexus->buses[i] = bus' only when all functions that can fail in
the loop have been called. (the very last 'break' is when the array is full)
And in the remove function, we have:
struct cavium_mdiobus *bus = nexus->buses[i];
if (!bus)
continue;
So, this looks safe to me.
CJ
From: Russell King - ARM Linux admin <linux@armlinux.org.uk> Date: 2021-05-13 08:10:10
On Thu, May 13, 2021 at 08:29:01AM +0200, Christophe JAILLET wrote:
Le 12/05/2021 à 23:44, Russell King - ARM Linux admin a écrit :
quoted
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
quoted
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Remove the incorrect 'mdiobus_free' in the remove function.
Yes, this looks correct, thanks.
Reviewed-by: Russell King <redacted>
However, there's another issue in this driver that ought to be fixed.
If devm_mdiobus_alloc_size() succeeds, but of_mdiobus_register() fails,
we continue on to the next bus (which I think is reasonable.) We don't
free the bus.
When we come to the remove method however, we will call
mdiobus_unregister() on this existent but not-registered bus. Surely we
don't want to do that?
Hmmm, I don't agree here.
'nexus' is 'kzalloc()'ed.
So the pointers in 'buses[]' are all NULL by default.
We set 'nexus->buses[i] = bus' only when all functions that can fail in the
loop have been called. (the very last 'break' is when the array is full)
And in the remove function, we have:
struct cavium_mdiobus *bus = nexus->buses[i];
if (!bus)
continue;
So, this looks safe to me.
It isn't safe. Please look closer.
device_for_each_child_node(&pdev->dev, fwn) {
mii_bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*bus));
if (!mii_bus)
break;
bus = mii_bus->priv;
bus->mii_bus = mii_bus;
nexus->buses[i] = bus;
This succeeds and sets nexus->buses[i] to a non-NULL value.
err = of_mdiobus_register(bus->mii_bus, node);
if (err)
dev_err(&pdev->dev, "of_mdiobus_register failed\n");
dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
if (i >= ARRAY_SIZE(nexus->buses))
break;
}
If of_mdiobus_register() fails, the bus is not registered, and we just
move on to the next bus, leaving nexus->buses[i] set to a non-NULL
value.
If we now look at the remove code:
for (i = 0; i < ARRAY_SIZE(nexus->buses); i++) {
struct cavium_mdiobus *bus = nexus->buses[i];
if (!bus)
continue;
mdiobus_unregister(bus->mii_bus);
nexus->buses[i] is non-NULL, but the bus is _not_ registered. We end up
calling mdiobus_unregister() on an allocated but _unregistered_ bus.
This is a bug.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Le 13/05/2021 à 10:09, Russell King - ARM Linux admin a écrit :
On Thu, May 13, 2021 at 08:29:01AM +0200, Christophe JAILLET wrote:
quoted
Le 12/05/2021 à 23:44, Russell King - ARM Linux admin a écrit :
quoted
On Wed, May 12, 2021 at 11:35:38PM +0200, Christophe JAILLET wrote:
quoted
'bus->mii_bus' have been allocated with 'devm_mdiobus_alloc_size()' in the
probe function. So it must not be freed explicitly or there will be a
double free.
Remove the incorrect 'mdiobus_free' in the remove function.
Yes, this looks correct, thanks.
Reviewed-by: Russell King <redacted>
However, there's another issue in this driver that ought to be fixed.
If devm_mdiobus_alloc_size() succeeds, but of_mdiobus_register() fails,
we continue on to the next bus (which I think is reasonable.) We don't
free the bus.
When we come to the remove method however, we will call
mdiobus_unregister() on this existent but not-registered bus. Surely we
don't want to do that?
Hmmm, I don't agree here.
'nexus' is 'kzalloc()'ed.
So the pointers in 'buses[]' are all NULL by default.
We set 'nexus->buses[i] = bus' only when all functions that can fail in the
loop have been called. (the very last 'break' is when the array is full)
And in the remove function, we have:
struct cavium_mdiobus *bus = nexus->buses[i];
if (!bus)
continue;
So, this looks safe to me.
It isn't safe. Please look closer.
device_for_each_child_node(&pdev->dev, fwn) {
mii_bus = devm_mdiobus_alloc_size(&pdev->dev, sizeof(*bus));
if (!mii_bus)
break;
bus = mii_bus->priv;
bus->mii_bus = mii_bus;
nexus->buses[i] = bus;
This succeeds and sets nexus->buses[i] to a non-NULL value.
err = of_mdiobus_register(bus->mii_bus, node);
if (err)
dev_err(&pdev->dev, "of_mdiobus_register failed\n");
dev_info(&pdev->dev, "Added bus at %llx\n", r.start);
if (i >= ARRAY_SIZE(nexus->buses))
break;
}
If of_mdiobus_register() fails, the bus is not registered, and we just
move on to the next bus, leaving nexus->buses[i] set to a non-NULL
value.