| Message ID | f8ad939e6d5df4cb0273ea71a418a3ca1835338d.1620855222.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: mdio: Fix a double free issue in the .remove function | expand |
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 <rmk+kernel@armlinux.org.uk> 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!
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
Le 12/05/2021 à 23:44, Andrew Lunn a écrit : > 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. 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 > > Andrew > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/net/mdio
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: >> '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 <rmk+kernel@armlinux.org.uk> > > 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
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 : > > 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 <rmk+kernel@armlinux.org.uk> > > > > 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: >> 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: >>>> '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 <rmk+kernel@armlinux.org.uk> >>> >>> 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. Got it. Thx for the explanation. CJ
diff --git a/drivers/net/mdio/mdio-thunder.c b/drivers/net/mdio/mdio-thunder.c index cb1761693b69..822d2cdd2f35 100644 --- a/drivers/net/mdio/mdio-thunder.c +++ b/drivers/net/mdio/mdio-thunder.c @@ -126,7 +126,6 @@ static void thunder_mdiobus_pci_remove(struct pci_dev *pdev) continue; mdiobus_unregister(bus->mii_bus); - mdiobus_free(bus->mii_bus); oct_mdio_writeq(0, bus->register_base + SMI_EN); } pci_release_regions(pdev);
'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 <christophe.jaillet@wanadoo.fr> --- drivers/net/mdio/mdio-thunder.c | 1 - 1 file changed, 1 deletion(-)