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

From: Jason Wang <[email-masked]>
Date: Tue, 3 Sep 2019 06:50:11 -0400 (EDT)

----- 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 <[email-masked]>
> >>>>>>>>>>>> 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