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]

Message-ID: <c86fecd6-6412-fec8-1dce-81e99c059e38@gmail.com>
Date:   Tue, 28 Sep 2021 14:45:39 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Yanfei Xu <yanfei.xu@...driver.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        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,
        Dongliang Mu <mudongliangabcd@...il.com>
Subject: Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register

On 9/28/21 14:30, 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.
> 

mdiobus_free() is called in case of ->probe() failure, because devres 
clean up function for bus is devm_mdiobus_free(). It simply calls 
mdiobus_free().


So, i imagine following calltrace:

ax88772_bind
   ax88772_init_mdio
     devm_mdiobus_alloc() <- bus registered as devres
     devm_mdiobus_register() <- fail (->probe failure)

...

devres_release_all
   mdiobus_free()


Also, syzbot has tested my patch :)

> 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.
> 
> regards,
> dan carpenter
> 



With regards,
Pavel Skripkin