* [PATCH] net: mdio: Fix a double free issue in the .remove function @ 2021-05-12 21:35 Christophe JAILLET 2021-05-12 21:44 ` Russell King - ARM Linux admin 2021-05-12 21:44 ` Andrew Lunn 0 siblings, 2 replies; 7+ messages in thread From: Christophe JAILLET @ 2021-05-12 21:35 UTC (permalink / raw) To: andrew, hkallweit1, linux, davem, kuba, david.daney Cc: netdev, linux-kernel, kernel-janitors, Christophe JAILLET '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(-) 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); -- 2.30.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-12 21:35 [PATCH] net: mdio: Fix a double free issue in the .remove function Christophe JAILLET @ 2021-05-12 21:44 ` Russell King - ARM Linux admin 2021-05-13 6:29 ` Christophe JAILLET 2021-05-12 21:44 ` Andrew Lunn 1 sibling, 1 reply; 7+ messages in thread From: Russell King - ARM Linux admin @ 2021-05-12 21:44 UTC (permalink / raw) To: Christophe JAILLET Cc: andrew, hkallweit1, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-12 21:44 ` Russell King - ARM Linux admin @ 2021-05-13 6:29 ` Christophe JAILLET 2021-05-13 8:09 ` Russell King - ARM Linux admin 0 siblings, 1 reply; 7+ messages in thread From: Christophe JAILLET @ 2021-05-13 6:29 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: andrew, hkallweit1, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-13 6:29 ` Christophe JAILLET @ 2021-05-13 8:09 ` Russell King - ARM Linux admin 2021-05-13 8:22 ` Christophe JAILLET 0 siblings, 1 reply; 7+ messages in thread From: Russell King - ARM Linux admin @ 2021-05-13 8:09 UTC (permalink / raw) To: Christophe JAILLET Cc: andrew, hkallweit1, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-13 8:09 ` Russell King - ARM Linux admin @ 2021-05-13 8:22 ` Christophe JAILLET 0 siblings, 0 replies; 7+ messages in thread From: Christophe JAILLET @ 2021-05-13 8:22 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: andrew, hkallweit1, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-12 21:35 [PATCH] net: mdio: Fix a double free issue in the .remove function Christophe JAILLET 2021-05-12 21:44 ` Russell King - ARM Linux admin @ 2021-05-12 21:44 ` Andrew Lunn 2021-05-13 6:21 ` Christophe JAILLET 1 sibling, 1 reply; 7+ messages in thread From: Andrew Lunn @ 2021-05-12 21:44 UTC (permalink / raw) To: Christophe JAILLET Cc: hkallweit1, linux, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net: mdio: Fix a double free issue in the .remove function 2021-05-12 21:44 ` Andrew Lunn @ 2021-05-13 6:21 ` Christophe JAILLET 0 siblings, 0 replies; 7+ messages in thread From: Christophe JAILLET @ 2021-05-13 6:21 UTC (permalink / raw) To: Andrew Lunn Cc: hkallweit1, linux, davem, kuba, david.daney, netdev, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-13 8:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-12 21:35 [PATCH] net: mdio: Fix a double free issue in the .remove function Christophe JAILLET 2021-05-12 21:44 ` Russell King - ARM Linux admin 2021-05-13 6:29 ` Christophe JAILLET 2021-05-13 8:09 ` Russell King - ARM Linux admin 2021-05-13 8:22 ` Christophe JAILLET 2021-05-12 21:44 ` Andrew Lunn 2021-05-13 6:21 ` Christophe JAILLET
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).