RE: [PATCH] tun: Fix use-after-free in tun_net_xmit

[weiyongjun (A)]

> > On Sun, Apr 28, 2019 at 12:51 AM Jason Wang <jasowang@xxxxxxxxxx>
> wrote:
> >>> tun_net_xmit() doesn't have the chance to
> >>> access the change because it holding the rcu_read_lock().
> >>
> >>
> >> The problem is the following codes:
> >>
> >>
> >>          --tun->numqueues;
> >>
> >>          ...
> >>
> >>          synchronize_net();
> >>
> >> We need make sure the decrement of tun->numqueues be visible to
> readers
> >> after synchronize_net(). And in tun_net_xmit():
> >
> > It doesn't matter at all. Readers are okay to read it even they still use the
> > stale tun->numqueues, as long as the tfile is not freed readers can read
> > whatever they want...
> 
> This is only true if we set SOCK_RCU_FREE, isn't it?
> 
> >
> > The decrement of tun->numqueues is just how we unpublish the old
> > tfile, it is still valid for readers to read it _after_ unpublish, we only need
> > to worry about free, not about unpublish. This is the whole spirit of RCU.
> >
> 
> The point is we don't convert tun->numqueues to RCU but use
> synchronize_net().
> 
> > You need to rethink about my SOCK_RCU_FREE patch.
> 
> The code is wrote before SOCK_RCU_FREE is introduced and assume no
> de-reference from device after synchronize_net(). It doesn't harm to
> figure out the root cause which may give us more confidence to the fix
> (e.g like SOCK_RCU_FREE).
> 
> I don't object to fix with SOCK_RCU_FREE, but then we should remove
> the redundant synchronize_net(). But I still prefer to synchronize
> everything explicitly like (completely untested):
> 
> From df91f77d35a6aa7943b6f2a7d4b329990896a0fe Mon Sep 17 00:00:00
> 2001
> From: Jason Wang <jasowang@xxxxxxxxxx>
> Date: Mon, 29 Apr 2019 10:21:06 +0800
> Subject: [PATCH] tuntap: synchronize through tfiles instead of numqueues
> 
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> ---
>  drivers/net/tun.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 80bff1b4ec17..03715f605fb5 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -698,6 +698,7 @@ static void __tun_detach(struct tun_file *tfile, bool
> clean)
> 
>  		rcu_assign_pointer(tun->tfiles[index],
>  				   tun->tfiles[tun->numqueues - 1]);
> +		rcu_assign_pointer(tun->tfiles[tun->numqueues], NULL);

Should be "rcu_assign_pointer(tun->tfiles[tun->numqueues - 1], NULL);"

>  		ntfile = rtnl_dereference(tun->tfiles[index]);
>  		ntfile->queue_index = index;
> 
> @@ -1082,7 +1083,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff
> *skb, struct net_device *dev)
>  	tfile = rcu_dereference(tun->tfiles[txq]);
> 
>  	/* Drop packet if interface is not attached */
> -	if (txq >= tun->numqueues)
> +	if (!tfile)
>  		goto drop;
> 
>  	if (!rcu_dereference(tun->steering_prog))
> @@ -1305,15 +1306,13 @@ static int tun_xdp_xmit(struct net_device *dev,
> int n,
> 
>  	rcu_read_lock();
> 
> -	numqueues = READ_ONCE(tun->numqueues);
> -	if (!numqueues) {
> +	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
> +					    tun->numqueues]);
> +	if (!tfile) {
>  		rcu_read_unlock();
>  		return -ENXIO; /* Caller will free/return all frames */
>  	}
> 
> -	tfile = rcu_dereference(tun->tfiles[smp_processor_id() %
> -					    numqueues]);
> -
>  	spin_lock(&tfile->tx_ring.producer_lock);
>  	for (i = 0; i < n; i++) {
>  		struct xdp_frame *xdp = frames[i];
> --
> 2.19.1