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 <paskripkin@gmail.com> --- 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 <paskripkin@gmail.com> 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 <yanfei.xu@windriver.com> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- 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 <paskripkin@gmail.com> --- 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 <paskripkin@gmail.com>
Thanks!
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
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<yanfei.xu@windriver.com>
> Signed-off-by: Pavel Skripkin<paskripkin@gmail.com>
> ---
>
> Changes in v2:
> Added this revert
Acked-by: Yanfei Xu<yanfei.xu@windriver.com>
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:
>> + /* 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