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]

Message-ID: <5D555796.9020104@huawei.com>
Date:   Thu, 15 Aug 2019 21:01:10 +0800
From:   Yang Yingliang <yangyingliang@...wei.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, <netdev@...r.kernel.org>
CC:     <jasowang@...hat.com>, <xiyou.wangcong@...il.com>,
        <davem@...emloft.net>, Yangyingliang <yangyingliang@...wei.com>
Subject: Re: [PATCH] tun: fix use-after-free when register netdev failed



On 2019/8/15 17:35, Eric Dumazet wrote:
>
> 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().

register_netdevice() is failed, so the dev is freed directly in 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.
I will try Wang's sugguestion later, if it's OK, I will drop this patch.
>
> .
>