[PATCH] phy: mdio: fix memory leak

Syzbot reported memory leak in MDIO bus interface, the problem was in
wrong state logic.

MDIOBUS_ALLOCATED indicates 2 states:
	1. Bus is only allocated
	2. Bus allocated and __mdiobus_register() fails, but
	   device_register() was called

In case of device_register() has been called we should call put_device()
to correctly free the memory allocated for this device, but mdiobus_free()
was just calling kfree(dev) in case of MDIOBUS_ALLOCATED state

To avoid this behaviour we can add new intermediate state, which means,
that we have called device_regiter(), but failed on any of the next steps.
Clean up process for this state is the same as for MDIOBUS_UNREGISTERED,
but MDIOBUS_UNREGISTERED name does not fit to the logic described above.

Fixes: 46abc02175b3 ("phylib: give mdio buses a device tree presence")
Reported-and-tested-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---
 drivers/net/phy/mdio_bus.c | 4 +++-
 include/linux/phy.h        | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 53f034fc2ef7..ed764638b449 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -540,6 +540,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 		return -EINVAL;
 	}
 
+	bus->state = MDIOBUS_DEV_REGISTERED;
+
 	mutex_init(&bus->mdio_lock);
 	mutex_init(&bus->shared_lock);
 

@@ -647,7 +649,7 @@ void mdiobus_free(struct mii_bus *bus)
 		return;
 	}
 
-	BUG_ON(bus->state != MDIOBUS_UNREGISTERED);
+	BUG_ON(bus->state != MDIOBUS_UNREGISTERED && bus->state != MDIOBUS_DEV_REGISTERED);
 	bus->state = MDIOBUS_RELEASED;
 
 	put_device(&bus->dev);

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 736e1d1a47c4..41d2ccdacd5e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -343,6 +343,7 @@ struct mii_bus {
 		MDIOBUS_REGISTERED,
 		MDIOBUS_UNREGISTERED,
 		MDIOBUS_RELEASED,
+		MDIOBUS_DEV_REGISTERED,
 	} state;
 
 	/** @dev: Kernel device representation */

-- 
2.33.0

On 9/27/21 14:20, Pavel Skripkin wrote:

Syzbot reported memory leak in MDIO bus interface, the problem was in
wrong state logic.

MDIOBUS_ALLOCATED indicates 2 states:
	1. Bus is only allocated
	2. Bus allocated and __mdiobus_register() fails, but
	   device_register() was called

In case of device_register() has been called we should call put_device()
to correctly free the memory allocated for this device, but mdiobus_free()
was just calling kfree(dev) in case of MDIOBUS_ALLOCATED state

To avoid this behaviour we can add new intermediate state, which means,
that we have called device_regiter(), but failed on any of the next steps.
Clean up process for this state is the same as for MDIOBUS_UNREGISTERED,
but MDIOBUS_UNREGISTERED name does not fit to the logic described above.

Fixes: 46abc02175b3 ("phylib: give mdio buses a device tree presence")
Reported-and-tested-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>

I've just found, that this syzkaller bug has been closed by Yanfei's 
patch [1], but Yanfei's Reported-by: is wrong, IMO.

Yanfei's patch fixed other memory leak and it's not related to bug 
reported by syzkaller. If you take a look at log [2] you won't find 
error message about mii_bus registration failure, i.e the error happened 
a bit latter (more precisely in mdiobus_scan()).

Since, Yanfei's patch is already applied, we cannot remove this tag, so 
I am informing you about this situation to break possible confusions 
about 2 different patches with same Reported-by: tag :)


Thanks


[1] 
https://lore.kernel.org/netdev/20210926045313.2267655-1-yanfei.xu@windriver.com/

[2] https://syzkaller.appspot.com/text?tag=CrashLog&x=131c754b300000


With regards,
Pavel Skripkin

This reverts commit ab609f25d19858513919369ff3d9a63c02cd9e2e.

This patch is correct in the sense that we _should_ call device_put() in
case of device_register() failure, but the problem in this code is more
vast.

We need to set bus->state to UNMDIOBUS_REGISTERED before calling
device_register() to correctly release the device in mdiobus_free().
This patch prevents us from doing it, since in case of device_register()
failure put_device() will be called 2 times and it will cause UAF or
something else.

Also, Reported-by: tag in revered commit was wrong, since syzbot
reported different leak in same function.

Link: https://lore.kernel.org/netdev/20210928092657.GI2048@kadam/
Cc: Yanfei Xu <redacted>
Signed-off-by: Pavel Skripkin <redacted>
---

Changes in v2:
	Added this revert

---
 drivers/net/phy/mdio_bus.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6f4b4e5df639..53f034fc2ef7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -537,7 +537,6 @@ 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);
 		return -EINVAL;
 	}
 

-- 
2.33.0

Syzbot reported memory leak in MDIO bus interface, the problem was in
wrong state logic.

MDIOBUS_ALLOCATED indicates 2 states:
	1. Bus is only allocated
	2. Bus allocated and __mdiobus_register() fails, but
	   device_register() was called

In case of device_register() has been called we should call put_device()
to correctly free the memory allocated for this device, but mdiobus_free()
calls just kfree(dev) in case of MDIOBUS_ALLOCATED state

To avoid this behaviour we need to set bus->state to MDIOBUS_UNREGISTERED
_before_ calling device_register(), because put_device() should be
called even in case of device_register() failure.

Link: https://lore.kernel.org/netdev/YVMRWNDZDUOvQjHL@shell.armlinux.org.uk/
Fixes: 46abc02175b3 ("phylib: give mdio buses a device tree presence")
Reported-and-tested-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>
---

Changes in v2:
	Removed new state, since MDIOBUS_UNREGISTERED can be used. Also, moved
	bus->state initialization _before_ device_register() call, because
	Yanfei's patch is reverted in [v2 1/2]

---
 drivers/net/phy/mdio_bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 53f034fc2ef7..2d16a93af1ae 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

@@ -534,6 +534,13 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
 	bus->dev.groups = NULL;
 	dev_set_name(&bus->dev, "%s", bus->id);
 
+	/* We need to set state to MDIOBUS_UNREGISTERED to correctly realese
+	 * the device in mdiobus_free()
+	 *
+	 * State will be updated later in this function in case of success
+	 */
+	bus->state == MDIOBUS_UNREGISTERED;
+
 	err = device_register(&bus->dev);
 	if (err) {
 		pr_err("mii_bus %s failed to register\n", bus->id);

-- 
2.33.0

On Tue, Sep 28, 2021 at 11:40:15PM +0300, Pavel Skripkin wrote:

Syzbot reported memory leak in MDIO bus interface, the problem was in
wrong state logic.

MDIOBUS_ALLOCATED indicates 2 states:
	1. Bus is only allocated
	2. Bus allocated and __mdiobus_register() fails, but
	   device_register() was called

In case of device_register() has been called we should call put_device()
to correctly free the memory allocated for this device, but mdiobus_free()
calls just kfree(dev) in case of MDIOBUS_ALLOCATED state

To avoid this behaviour we need to set bus->state to MDIOBUS_UNREGISTERED
_before_ calling device_register(), because put_device() should be
called even in case of device_register() failure.

Link: https://lore.kernel.org/netdev/YVMRWNDZDUOvQjHL@shell.armlinux.org.uk/ 
Fixes: 46abc02175b3 ("phylib: give mdio buses a device tree presence")
Reported-and-tested-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <redacted>

Thanks!

Reviewed-by: Dan Carpenter <redacted>

regards,
dan carpenter

On 9/29/21 4:39 AM, Pavel Skripkin wrote:

This reverts commit ab609f25d19858513919369ff3d9a63c02cd9e2e.

This patch is correct in the sense that we_should_  call device_put() in
case of device_register() failure, but the problem in this code is more
vast.

We need to set bus->state to UNMDIOBUS_REGISTERED before calling
device_register() to correctly release the device in mdiobus_free().
This patch prevents us from doing it, since in case of device_register()
failure put_device() will be called 2 times and it will cause UAF or
something else.

Also, Reported-by: tag in revered commit was wrong, since syzbot
reported different leak in same function.

Link:https://lore.kernel.org/netdev/20210928092657.GI2048@kadam/
Cc: Yanfei Xu<redacted>
Signed-off-by: Pavel Skripkin<redacted>
---

Changes in v2:
         Added this revert

Acked-by: Yanfei Xu<redacted>

Thanks,
Yanfei

On Tue, 28 Sep 2021 23:40:15 +0300 Pavel Skripkin wrote:

+	/* We need to set state to MDIOBUS_UNREGISTERED to correctly realese
+	 * the device in mdiobus_free()
+	 *
+	 * State will be updated later in this function in case of success
+	 */
+	bus->state == MDIOBUS_UNREGISTERED;

IDK how syzbot has tested it but clearly we should blindly 
depend on that.

s/==/=/

Compiler would have told you this.

On 9/30/21 02:48, Jakub Kicinski wrote:

On Tue, 28 Sep 2021 23:40:15 +0300 Pavel Skripkin wrote:

quoted

+	/* We need to set state to MDIOBUS_UNREGISTERED to correctly realese
+	 * the device in mdiobus_free()
+	 *
+	 * State will be updated later in this function in case of success
+	 */
+	bus->state == MDIOBUS_UNREGISTERED;

IDK how syzbot has tested it but clearly we should blindly
depend on that.

s/==/=/

Compiler would have told you this.

whooops... sorry about that. syzbot has tested v1. v2 is same, but 
without new state (so, the logic in v1 and v2 is the same).

I guess, it's copy-paste error on my side :). Will send v3 this evening



With regards,
Pavel Skripkin