| 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
| ||
|
Date: Tue, 3 Sep 2019 06:50:11 -0400 (EDT)
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
----- Original Message -----
>
>
> On 2019/9/3 14:06, Jason Wang wrote:
> >
> > 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@...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?
> >> 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.
>
> The device is not released in tun_put().
> This is how the UAF occurs:
>
> CPUA CPUB
> tun_set_iff()
> alloc_netdev_mqs()
> tun_attach()
> tun_chr_read_iter()
> tun_get()
> tun_do_read()
> tun_ring_recv()
> register_netdevice() <-- inject error
> goto err_detach
> tun_detach_all() <-- set RCV_SHUTDOWN
> free_netdev() <-- called from
> err_free_dev path
> netdev_freemem() <-- free the memory
> without check refcount
> (In this path, the refcount cannot prevent
> freeing the memory of dev, and the memory
> will be used by dev_put() called by
> tun_chr_read_iter() on CPUB.)
> (Break from
> tun_ring_recv(),
> because RCV_SHUTDOWN
> is set)
> tun_put()
> dev_put() <-- use the
> memory freed by
> netdev_freemem()
>
>
My bad, thanks for the patience. Since all evil come from the
tfile->tun, how about delay the publishing of tfile->tun until the
success of registration to make sure dev_put() and dev_hold() work.
(Compile test only)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index db16d7a13e00..aab0be40d443 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -787,7 +787,8 @@ static void tun_detach_all(struct net_device *dev)
}
static int tun_attach(struct tun_struct *tun, struct file *file,
- bool skip_filter, bool napi, bool napi_frags)
+ bool skip_filter, bool napi, bool napi_frags,
+ bool publish_tun)
{
struct tun_file *tfile = file->private_data;
struct net_device *dev = tun->dev;
@@ -870,7 +871,8 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
* initialized tfile; otherwise we risk using half-initialized
* object.
*/
- rcu_assign_pointer(tfile->tun, tun);
+ if (publish_tun)
+ rcu_assign_pointer(tfile->tun, tun);
rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
tun->numqueues++;
tun_set_real_num_queues(tun);
@@ -2730,7 +2732,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
err = tun_attach(tun, file, ifr->ifr_flags & IFF_NOFILTER,
ifr->ifr_flags & IFF_NAPI,
- ifr->ifr_flags & IFF_NAPI_FRAGS);
+ ifr->ifr_flags & IFF_NAPI_FRAGS, true);
if (err < 0)
return err;
@@ -2829,13 +2831,17 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
INIT_LIST_HEAD(&tun->disabled);
err = tun_attach(tun, file, false, ifr->ifr_flags & IFF_NAPI,
- ifr->ifr_flags & IFF_NAPI_FRAGS);
+ ifr->ifr_flags & IFF_NAPI_FRAGS, false);
if (err < 0)
goto err_free_flow;
err = register_netdevice(tun->dev);
if (err < 0)
goto err_detach;
+ /* free_netdev() won't check refcnt, to aovid race
+ * with dev_put() we need publish tun after registration.
+ */
+ rcu_assign_pointer(tfile->tun, tun);
}
netif_carrier_on(tun->dev);
@@ -2978,7 +2984,7 @@ static int tun_set_queue(struct file *file, struct ifreq *ifr)
if (ret < 0)
goto unlock;
ret = tun_attach(tun, file, false, tun->flags & IFF_NAPI,
- tun->flags & IFF_NAPI_FRAGS);
+ tun->flags & IFF_NAPI_FRAGS, true);
} else if (ifr->ifr_flags & IFF_DETACH_QUEUE) {
tun = rtnl_dereference(tfile->tun);
if (!tun || !(tun->flags & IFF_MULTI_QUEUE) || tfile->detached)
--
2.18.1
> >
> > Thanks
> >
>
>
>
Powered by blists - more mailing lists