Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

[Russell King (Oracle)]

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:
> > On 9/28/21 14:06, Pavel Skripkin wrote:
> > > > 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!