netdev - Re: [PATCH net] net: vlan: fix a UAF in vlan_dev_real

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: <b573b01c-2cc9-4722-6289-f7b9e0a43e19@huawei.com>
Date:   Fri, 29 Oct 2021 15:04:35 +0800
From:   "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>
To:     Jakub Kicinski <kuba@...nel.org>, Jason Gunthorpe <jgg@...dia.com>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-rdma@...r.kernel.org>
Subject: Re: [PATCH net] net: vlan: fix a UAF in vlan_dev_real_dev()

> On Thu, 28 Oct 2021 08:45:03 -0300 Jason Gunthorpe wrote:
>>> But will make all the callers of vlan_dev_real_dev() feel like they
>>> should NULL-check the result, which is not necessary.  
>>
>> Isn't it better to reliably return NULL instead of a silent UAF in
>> this edge case? 
> 
> I don't know what the best practice is for maintaining sanity of
> unregistered objects.
> 
> If there really is a requirement for the real_dev pointer to be sane we
> may want to move the put_device(real_dev) to vlan_dev_free(). There
> should not be any risk of circular dependency but I'm not 100% sure.
> 
>>> RDMA must be calling this helper on a vlan which was already
>>> unregistered, can we fix RDMA instead?  
>>
>> RDMA holds a get on the netdev which prevents unregistration, however
>> unregister_vlan_dev() does:
>>
>>         unregister_netdevice_queue(dev, head);
>>         dev_put(real_dev);
>>
>> Which corrupts the still registered vlan device while it is sitting in
>> the queue waiting to unregister. So, it is not true that a registered
>> vlan device always has working vlan_dev_real_dev().
> 
> That's not my reading, unless we have a different definition of
> "registered". The RDMA code in question runs from a workqueue, at the
> time the UNREGISTER notification is generated all objects are still
> alive and no UAF can happen. Past UNREGISTER extra care is needed when
> accessing the object.
> 
> Note that unregister_vlan_dev() may queue the unregistration, without
> running it. If it clears real_dev the UNREGISTER notification will no
> longer be able to access real_dev, which used to be completely legal.
> .
> 

I am sorry. I have made a misunderstanding and given a wrong conclusion
that unregister_vlan_dev() just move the vlan_ndev to a list to unregister
later and it is possible the real_dev has been freed when we access in
netdevice_queue_work().

real_ndev UNREGISTE trigger NETDEV_UNREGISTER notification in
vlan_device_event(), unregister_vlan_dev() and unregister_netdevice_many()
are within real_ndev UNREGISTE process. real_dev and vlan_ndev are all
alive before real_ndev UNREGISTE finished.

Above is the correction for my previous misunderstanding. But the real
scenario of the problem is as following:

__rtnl_newlink
vlan_newlink
register_vlan_dev(vlan_ndev, ...)
register_netdevice(vlan_ndev)
netdevice_queue_work(..., vlan_ndev) [dev_hold(vlan_ndev)]
queue_work(gid_cache_wq, ...)
...
rtnl_configure_link(vlan_ndev, ...) [failed]
ops->dellink(vlan_ndev, &list_kill) [unregister_vlan_dev]
unregister_netdevice_many(&list_kill)
...
ppp_release
unregister_netdevice(real_dev)
ppp_destroy_interface
free_netdev(real_dev)
netdev_freemem(real_dev) [real_dev freed]
...
netdevice_event_work_handler [vlan_ndev NETDEV_REGISTER notifier work]
is_eth_port_of_netdev_filter
vlan_dev_real_dev [real_dev UAF]

So my first solution as following for the problem is correct.
https://lore.kernel.org/linux-rdma/20211025163941.GA393143@nvidia.com/T/#m44abbf1ea5e4b5237610c1b389c3340d92a03b8d

Thank you!