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]
Date:   Tue, 3 Sep 2019 11:03:36 +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/9/3 上午9:45, Yang Yingliang wrote:
>
>
> On 2019/9/2 13:32, Jason Wang wrote:
>>
>> 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?
> No, it's not.
>
> err_free_dev:
>         free_netdev(dev);
>
> void free_netdev(struct net_device *dev)
> {
> ...
>         /* pcpu_refcnt can be freed without checking refcount */
>         free_percpu(dev->pcpu_refcnt);
>         dev->pcpu_refcnt = NULL;
>
>         /*  Compatibility with error handling in drivers */
>         if (dev->reg_state == NETREG_UNINITIALIZED) {
>                 /* dev can be freed without checking refcount */
>                 netdev_freemem(dev);
>                 return;
>         }
> ...
> }


Right, but what I meant is in my patch, when code reaches free_netdev() 
the refcnt is zero. What did I miss?

Thanks


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