From: "Russell King (Oracle)" <firstname.lastname@example.org> To: Dan Carpenter <email@example.com> Cc: Pavel Skripkin <firstname.lastname@example.org>, Yanfei Xu <email@example.com>, Bartosz Golaszewski <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, Dongliang Mu <email@example.com> Subject: Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register Date: Tue, 28 Sep 2021 13:58:00 +0100 [thread overview] Message-ID: <YVMRWNDZDUOvQjHL@shell.armlinux.org.uk> (raw) In-Reply-To: <20210928113055.GN2083@kadam> 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!
next prev parent reply other threads:[~2021-09-28 12:58 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-26 4:53 Yanfei Xu 2021-09-26 15:34 ` Andrew Lunn 2021-09-27 12:30 ` patchwork-bot+netdevbpf 2021-09-28 8:55 ` Dan Carpenter 2021-09-28 9:26 ` Dan Carpenter 2021-09-28 9:45 ` Pavel Skripkin 2021-09-28 10:39 ` Dan Carpenter 2021-09-28 10:46 ` Pavel Skripkin 2021-09-28 10:55 ` Dan Carpenter 2021-09-28 11:04 ` Pavel Skripkin 2021-09-28 10:59 ` Dan Carpenter 2021-09-28 11:06 ` Pavel Skripkin 2021-09-28 11:09 ` Pavel Skripkin 2021-09-28 11:30 ` Dan Carpenter 2021-09-28 11:45 ` Pavel Skripkin 2021-09-28 12:23 ` Dan Carpenter 2021-09-28 12:58 ` Russell King (Oracle) [this message] 2021-09-28 13:52 ` Dan Carpenter 2021-09-28 15:41 ` Russell King (Oracle) 2021-09-28 15:48 ` Dongliang Mu 2021-09-29 2:05 ` Xu, Yanfei 2021-09-28 13:11 ` Russell King (Oracle) 2021-09-29 21:31 ` Denis Efremov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YVMRWNDZDUOvQjHL@shell.armlinux.org.uk \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
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).