netdev - Re: [PATCH net] net: phy: phy_device: free the phy_device on the phy_device

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]

Message-ID: <ZdjFW3EZewQvyQt7@shell.armlinux.org.uk>
Date: Fri, 23 Feb 2024 16:18:35 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	davem@...emloft.net, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Johan Hovold <johan@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH net] net: phy: phy_device: free the phy_device on the
 phy_device_create error path

On Fri, Feb 23, 2024 at 05:01:54PM +0100, Maxime Chevallier wrote:
> When error'ing out from phy_device_create(), the previously kzalloc'd "dev"
> pointer gets overwritten with an error pointer, without freeing it
> beforehand, thus leaking the allocated phy_device. Add the missing kfree
> back.
> 
> Fixes: d02cbc461361 ("net: phy: fix memory leak in device-create error path")

No, it doesn't fix anything.

Sadly, this is the second patch that I've received recently which shows
a complete lack of understanding of the driver model, so I suspect
someone has documented something as a task, and that documentation is
either incomplete, or basically wrong.

In this case:

        /* We allocate the device, and initialize the default values */
        dev = kzalloc(sizeof(*dev), GFP_KERNEL);
        if (!dev)
                return ERR_PTR(-ENOMEM);

        mdiodev = &dev->mdio;
...
        device_initialize(&mdiodev->dev);

This sets the reference count on dev->mdio.dev to '1', and means that
at _this_ point, "dev" becomes a refcounted object. device_initialize()
is documented thusly:

/**
 * device_initialize - init device structure.
 * @dev: device.
 *
 * This prepares the device for use by other layers by initializing
 * its fields.
...
 * NOTE: Use put_device() to give up your reference instead of freeing
 * @dev directly once you have called this function.
 */

Now, the error path does this:

        if (ret) {
                put_device(&mdiodev->dev);
                dev = ERR_PTR(ret);
        }

which is (a) compliant with the device_initialize() documentation, and
(b) will drop the reference count of '1' down to '0' resulting in the
release function being called - and it is the responsibility of the
release function to free the memory.

Adding a kfree() in this path will lead to a double-kfree() of the
allocated memory, and that is _incorrect_.

So, given that this is the second such instance of someone wanting to
incorrectly kfree() a structure after a call to device_initialize(),
can I please ask everyone who reads this message, and who receives a
patch like this to _please_ not assume that it is correct, and check
it _very_ _carefully_.

Can I also ask those who propose to send out such patches _also_ do
the due dilligence and check this before creating noise.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!