netdev - Re: [PATCH v3] tun: fix use-after-free when register netdev failed

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: <1be732b2-6eda-4ea6-772d-780694557910@redhat.com>
Date:   Mon, 2 Sep 2019 13:32:44 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Yang Yingliang <yangyingliang@...wei.com>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        eric dumazet <eric.dumazet@...il.com>,
        xiyou wangcong <xiyou.wangcong@...il.com>,
        weiyongjun1@...wei.com
Subject: Re: [PATCH v3] tun: fix use-after-free when register netdev failed


On 2019/8/23 下午5:36, Yang Yingliang wrote:
>
>
> On 2019/8/23 11:05, Jason Wang wrote:
>> ----- Original Message -----
>>>
>>> On 2019/8/22 14:07, Yang Yingliang wrote:
>>>>
>>>> On 2019/8/22 10:13, Jason Wang wrote:
>>>>> On 2019/8/20 上午10:28, Jason Wang wrote:
>>>>>> On 2019/8/20 上午9:25, David Miller wrote:
>>>>>>> From: Yang Yingliang <yangyingliang@...wei.com>
>>>>>>> Date: Mon, 19 Aug 2019 21:31:19 +0800
>>>>>>>
>>>>>>>> Call tun_attach() after register_netdevice() to make sure 
>>>>>>>> tfile->tun
>>>>>>>> is not published until the netdevice is registered. So the 
>>>>>>>> read/write
>>>>>>>> thread can not use the tun pointer that may freed by 
>>>>>>>> free_netdev().
>>>>>>>> (The tun and dev pointer are allocated by alloc_netdev_mqs(), they
>>>>>>>> can
>>>>>>>> be freed by netdev_freemem().)
>>>>>>> register_netdevice() must always be the last operation in the 
>>>>>>> order of
>>>>>>> network device setup.
>>>>>>>
>>>>>>> At the point register_netdevice() is called, the device is visible
>>>>>>> globally
>>>>>>> and therefore all of it's software state must be fully 
>>>>>>> initialized and
>>>>>>> ready for us.
>>>>>>>
>>>>>>> You're going to have to find another solution to these problems.
>>>>>>
>>>>>> The device is loosely coupled with sockets/queues. Each side is
>>>>>> allowed to be go away without caring the other side. So in this
>>>>>> case, there's a small window that network stack think the device has
>>>>>> one queue but actually not, the code can then safely drop them.
>>>>>> Maybe it's ok here with some comments?
>>>>>>
>>>>>> Or if not, we can try to hold the device before tun_attach and drop
>>>>>> it after register_netdevice().
>>>>>
>>>>> Hi Yang:
>>>>>
>>>>> I think maybe we can try to hold refcnt instead of playing real num
>>>>> queues here. Do you want to post a V4?
>>>> I think the refcnt can prevent freeing the memory in this case.
>>>> When register_netdevice() failed, free_netdev() will be called 
>>>> directly,
>>>> dev->pcpu_refcnt and dev are freed without checking refcnt of dev.
>>> How about using patch-v1 that using a flag to check whether the device
>>> registered successfully.
>>>
>> As I said, it lacks sufficient locks or barriers. To be clear, I meant
>> something like (compile-test only):
>>
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index db16d7a13e00..e52678f9f049 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
>> @@ -2828,6 +2828,7 @@ static int tun_set_iff(struct net *net, struct 
>> file *file, struct ifreq *ifr)
>>                                (ifr->ifr_flags & TUN_FEATURES);
>>                    INIT_LIST_HEAD(&tun->disabled);
>> +               dev_hold(dev);
>>                  err = tun_attach(tun, file, false, ifr->ifr_flags & 
>> IFF_NAPI,
>>                                   ifr->ifr_flags & IFF_NAPI_FRAGS);
>>                  if (err < 0)
>> @@ -2836,6 +2837,7 @@ static int tun_set_iff(struct net *net, struct 
>> file *file, struct ifreq *ifr)
>>                  err = register_netdevice(tun->dev);
>>                  if (err < 0)
>>                          goto err_detach;
>> +               dev_put(dev);
>>          }
>>            netif_carrier_on(tun->dev);
>> @@ -2852,11 +2854,13 @@ static int tun_set_iff(struct net *net, 
>> struct file *file, struct ifreq *ifr)
>>          return 0;
>>     err_detach:
>> +       dev_put(dev);
>>          tun_detach_all(dev);
>>          /* register_netdevice() already called tun_free_netdev() */
>>          goto err_free_dev;
>>     err_free_flow:
>> +       dev_put(dev);
>>          tun_flow_uninit(tun);
>>          security_tun_dev_free_security(tun->security);
>>   err_free_stat:
>>
>> What's your thought?
>
> The dev pointer are freed without checking the refcount in 
> free_netdev() called by err_free_dev
>
> path, so I don't understand how the refcount protects this pointer.
>

The refcount are guaranteed to be zero there, isn't it?

Thanks


> Thanks,
> Yang
>
>>
>> Thanks
>>
>> .
>>
>
>