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

[Jason Wang <jasowang@redhat.com>]

On 2019/9/3 下午1:42, Yang Yingliang wrote:
>
>
> On 2019/9/3 11:03, Jason Wang wrote:
>>
>> 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@huawei.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?
> Yes, but it can't fix the UAF problem.


Well, it looks to me that the dev_put() in tun_put() won't release the 
device in this case.

Thanks


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