netdev - Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Sep 2021 11:55:49 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Yanfei Xu <yanfei.xu@...driver.com>
Cc:     andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
        davem@...emloft.net, kuba@...nel.org, p.zabel@...gutronix.de,
        syzbot+398e7dc692ddbbb4cfec@...kaller.appspotmail.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu wrote:
> Once device_register() failed, we should call put_device() to
> decrement reference count for cleanup. Or it will cause memory
> leak.
> 

[ snipped stack trace ]

> 
> Reported-by: syzbot+398e7dc692ddbbb4cfec@...kaller.appspotmail.com
> Signed-off-by: Yanfei Xu <yanfei.xu@...driver.com>
> ---
>  drivers/net/phy/mdio_bus.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 53f034fc2ef7..6f4b4e5df639 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
>  	err = device_register(&bus->dev);
>  	if (err) {
>  		pr_err("mii_bus %s failed to register\n", bus->id);
> +		put_device(&bus->dev);

No this isn't right.  The dev.class is &mdio_bus_class.  It's set a few
lines earlier.

	bus->dev.class = &mdio_bus_class;

The release function is mdiobus_release().  It will free bus and lead to
use after frees in the callers.  Look at greth_mdio_init().  There are
a lot of callers which will crash now.

This patch was a clear layering violation.  If you didn't allocate "bus"
then you should not free it.  Keeping that rule in mind can help prevent
future bugs.  Also he other error handling paths are careful not to call
put_device() so why would this one be special?  It should have stood out
that this one error path is the only place that doesn't use a goto to
clean up.

I don't have a solution.  I have commented before that I hate kobjects
for this reason because they lead to unfixable memory leaks during
probe.  But this leak will only happen with fault injection and so it
doesn't affect real life.  And even if it did, a leak it preferable to a
crash.

Unfortunately, this patch has already been applied so please can you
send a patch to revert it?

regards,
dan carpenter