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

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

Date:   Thu, 15 Aug 2019 11:35:21 +0200
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Yang Yingliang <yangyingliang@...wei.com>, netdev@...r.kernel.org
Cc:     jasowang@...hat.com, xiyou.wangcong@...il.com, davem@...emloft.net
Subject: Re: [PATCH] tun: fix use-after-free when register netdev failed



On 8/15/19 10:18 AM, Yang Yingliang wrote:
> I got a UAF repport in tun driver when doing fuzzy test:
> 
>
> [  466.368604] page:ffffea000dc84e00 refcount:1 mapcount:0 mapping:ffff8883df1b4f00 index:0x0 compound_mapcount: 0
> [  466.371582] flags: 0x2fffff80010200(slab|head)
> [  466.372910] raw: 002fffff80010200 dead000000000100 dead000000000122 ffff8883df1b4f00
> [  466.375209] raw: 0000000000000000 0000000000070007 00000001ffffffff 0000000000000000
> [  466.377778] page dumped because: kasan: bad access detected
> [  466.379730]
> [  466.380288] Memory state around the buggy address:
> [  466.381844]  ffff888372139100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  466.384009]  ffff888372139180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  466.386131] >ffff888372139200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  466.388257]                                                  ^
> [  466.390234]  ffff888372139280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  466.392512]  ffff888372139300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [  466.394667] ==================================================================
> 
> tun_chr_read_iter() accessed the memory which freed by free_netdev()
> called by tun_set_iff():
> 
> 	CPUA				CPUB
>     tun_set_iff()
>       alloc_netdev_mqs()
>       tun_attach()
> 				    tun_chr_read_iter()
> 				      tun_get()
>       register_netdevice()
>       tun_detach_all()
>         synchronize_net()
> 				      tun_do_read()
> 				        tun_ring_recv()
> 				          schedule()
>       free_netdev()
> 				      tun_put() <-- UAF

UAF on what exactly ? The dev_hold() should prevent the free_netdev().

> 
> Set a new bit in tun->flag if register_netdevice() successed,
> without this bit, tun_get() returns NULL to avoid using a
> freed tun pointer.
> 
> Fixes: eb0fb363f920 ("tuntap: attach queue 0 before registering netdevice")
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> ---
>  drivers/net/tun.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index db16d7a13e00..cbd60c276c40 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -115,6 +115,7 @@ do {								\
>  /* High bits in flags field are unused. */
>  #define TUN_VNET_LE     0x80000000
>  #define TUN_VNET_BE     0x40000000
> +#define TUN_DEV_REGISTERED	0x20000000
>  
>  #define TUN_FEATURES (IFF_NO_PI | IFF_ONE_QUEUE | IFF_VNET_HDR | \
>  		      IFF_MULTI_QUEUE | IFF_NAPI | IFF_NAPI_FRAGS)
> @@ -719,8 +720,10 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>  			netif_carrier_off(tun->dev);
>  
>  			if (!(tun->flags & IFF_PERSIST) &&
> -			    tun->dev->reg_state == NETREG_REGISTERED)
> +			    tun->dev->reg_state == NETREG_REGISTERED) {
>  				unregister_netdevice(tun->dev);
> +				tun->flags &= ~TUN_DEV_REGISTERED;

Isn't this done too late ?

> +			}
>  		}
>  		if (tun)
>  			xdp_rxq_info_unreg(&tfile->xdp_rxq);
> @@ -884,8 +887,10 @@ static struct tun_struct *tun_get(struct tun_file *tfile)
>  
>  	rcu_read_lock();
>  	tun = rcu_dereference(tfile->tun);
> -	if (tun)
> +	if (tun && (tun->flags & TUN_DEV_REGISTERED))
>  		dev_hold(tun->dev);
> +	else
> +		tun = NULL;
>  	rcu_read_unlock();
>  
>  	return tun;
> @@ -2836,6 +2841,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;
> +		tun->flags |= TUN_DEV_REGISTERED;
>  	}
>  
>  	netif_carrier_on(tun->dev);
> 


So tun_get() will return NULL as long as  tun_set_iff() (TUNSETIFF ioctl()) has not yet been called ?

This could break some applications, since tun_get() is used from poll() and other syscalls.