[PATCH] net: mdio: Fix a double free issue in the .remove function

[PATCH] net: mdio: Fix a double free issue in the .remove function

[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

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Russell King - ARM Linux admin]

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!

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Andrew Lunn]

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

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Christophe JAILLET]

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

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Christophe JAILLET]

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

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Russell King - ARM Linux admin]

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!

Re: [PATCH] net: mdio: Fix a double free issue in the .remove function

[Christophe JAILLET]

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