netdev - Re: [PATCH net] tun: properly test for IFF

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] [day] [month] [year] [list]

Message-ID: <8bf60184-c950-0d47-7bc9-adc68e7ae1b1@gmail.com>
Date:   Sat, 16 Mar 2019 13:02:51 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] tun: properly test for IFF_UP



On 03/14/2019 08:19 PM, Eric Dumazet wrote:
> Same reasons than the ones explained in commit 4179cb5a4c92
> ("vxlan: test dev->flags & IFF_UP before calling netif_rx()")
> 
> netif_rx_ni() or napi_gro_frags() must be called under a strict contract.
> 
> At device dismantle phase, core networking clears IFF_UP
> and flush_all_backlogs() is called after rcu grace period
> to make sure no incoming packet might be in a cpu backlog
> and still referencing the device.
> 
> A similar protocol is used for gro layer.
> 
> Most drivers call netif_rx() from their interrupt handler,
> and since the interrupts are disabled at device dismantle,
> netif_rx() does not have to check dev->flags & IFF_UP
> 
> Virtual drivers do not have this guarantee, and must
> therefore make the check themselves.
> 
> Fixes: 1bd4978a88ac ("tun: honor IFF_UP in tun_get_user()")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> ---
>  drivers/net/tun.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 1d68921723dc08532b3f5321a52865076ad66336..0d343359f647ff58fee35462358827e61857c837 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1763,9 +1763,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	int skb_xdp = 1;
>  	bool frags = tun_napi_frags_enabled(tfile);
>  
> -	if (!(tun->dev->flags & IFF_UP))
> -		return -EIO;
> -
>  	if (!(tun->flags & IFF_NO_PI)) {
>  		if (len < sizeof(pi))
>  			return -EINVAL;
> @@ -1867,6 +1864,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  			err = skb_copy_datagram_from_iter(skb, 0, from, len);
>  
>  		if (err) {
> +			err = -EFAULT;
> +drop:
>  			this_cpu_inc(tun->pcpu_stats->rx_dropped);
>  			kfree_skb(skb);
>  			if (frags) {
> @@ -1874,7 +1873,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  				mutex_unlock(&tfile->napi_mutex);
>  			}
>  
> -			return -EFAULT;
> +			return err;
>  		}
>  	}
>  
> @@ -1958,6 +1957,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	    !tfile->detached)
>  		rxhash = __skb_get_hash_symmetric(skb);
>  
> +	rcu_read_lock();
> +	if (unlikely(!(tun->dev->flags & IFF_UP))) {
> +		err = -EIO;

Hmm, it looks like I forgot a rcu_read_unlock() here.

> +		goto drop;
> +	}
> +
>  	if (frags) {
>  		/* Exercise flow dissector code path. */
>  		u32 headlen = eth_get_headlen(skb->data, skb_headlen(skb));
> @@ -1965,6 +1970,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		if (unlikely(headlen > skb_headlen(skb))) {
>  			this_cpu_inc(tun->pcpu_stats->rx_dropped);
>  			napi_free_frags(&tfile->napi);
> +			rcu_read_unlock();
>  			mutex_unlock(&tfile->napi_mutex);
>  			WARN_ON(1);
>  			return -ENOMEM;
> @@ -1992,6 +1998,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  	} else {
>  		netif_rx_ni(skb);
>  	}
> +	rcu_read_unlock();
>  
>  	stats = get_cpu_ptr(tun->pcpu_stats);
>  	u64_stats_update_begin(&stats->syncp);
>