From: Yang Yingliang <yangyingliang@huawei.com>
To: Jason Wang <jasowang@redhat.com>, <netdev@vger.kernel.org>
Cc: <xiyou.wangcong@gmail.com>, <davem@davemloft.net>
Subject: Re: [PATCH] tun: fix use-after-free when register netdev failed
Date: Thu, 15 Aug 2019 20:55:53 +0800 [thread overview]
Message-ID: <5D555659.60307@huawei.com> (raw)
In-Reply-To: <f9dd102e-e3b4-6522-6094-63aa31b890ea@redhat.com>
On 2019/8/15 17:21, Jason Wang wrote:
>
> On 2019/8/15 下午4:18, Yang Yingliang wrote:
>> I got a UAF repport in tun driver when doing fuzzy test:
>>
>> [ 466.269490]
>> ==================================================================
>> [ 466.271792] BUG: KASAN: use-after-free in
>> tun_chr_read_iter+0x2ca/0x2d0
>> [ 466.271806] Read of size 8 at addr ffff888372139250 by task
>> tun-test/2699
>> [ 466.271810]
>> [ 466.271824] CPU: 1 PID: 2699 Comm: tun-test Not tainted
>> 5.3.0-rc1-00001-g5a9433db2614-dirty #427
>> [ 466.271833] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.org 04/01/2014
>> [ 466.271838] Call Trace:
>> [ 466.271858] dump_stack+0xca/0x13e
>> [ 466.271871] ? tun_chr_read_iter+0x2ca/0x2d0
>> [ 466.271890] print_address_description+0x79/0x440
>> [ 466.271906] ? vprintk_func+0x5e/0xf0
>> [ 466.271920] ? tun_chr_read_iter+0x2ca/0x2d0
>> [ 466.271935] __kasan_report+0x15c/0x1df
>> [ 466.271958] ? tun_chr_read_iter+0x2ca/0x2d0
>> [ 466.271976] kasan_report+0xe/0x20
>> [ 466.271987] tun_chr_read_iter+0x2ca/0x2d0
>> [ 466.272013] do_iter_readv_writev+0x4b7/0x740
>> [ 466.272032] ? default_llseek+0x2d0/0x2d0
>> [ 466.272072] do_iter_read+0x1c5/0x5e0
>> [ 466.272110] vfs_readv+0x108/0x180
>> [ 466.299007] ? compat_rw_copy_check_uvector+0x440/0x440
>> [ 466.299020] ? fsnotify+0x888/0xd50
>> [ 466.299040] ? __fsnotify_parent+0xd0/0x350
>> [ 466.299064] ? fsnotify_first_mark+0x1e0/0x1e0
>> [ 466.304548] ? vfs_write+0x264/0x510
>> [ 466.304569] ? ksys_write+0x101/0x210
>> [ 466.304591] ? do_preadv+0x116/0x1a0
>> [ 466.304609] do_preadv+0x116/0x1a0
>> [ 466.309829] do_syscall_64+0xc8/0x600
>> [ 466.309849] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [ 466.309861] RIP: 0033:0x4560f9
>> [ 466.309875] Code: 00 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00
>> 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24
>> 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64
>> 89 01 48
>> [ 466.309889] RSP: 002b:00007ffffa5166e8 EFLAGS: 00000206 ORIG_RAX:
>> 0000000000000127
>> [ 466.322992] RAX: ffffffffffffffda RBX: 0000000000400460 RCX:
>> 00000000004560f9
>> [ 466.322999] RDX: 0000000000000003 RSI: 00000000200008c0 RDI:
>> 0000000000000003
>> [ 466.323007] RBP: 00007ffffa516700 R08: 0000000000000004 R09:
>> 0000000000000000
>> [ 466.323014] R10: 0000000000000000 R11: 0000000000000206 R12:
>> 000000000040cb10
>> [ 466.323021] R13: 0000000000000000 R14: 00000000006d7018 R15:
>> 0000000000000000
>> [ 466.323057]
>> [ 466.323064] Allocated by task 2605:
>> [ 466.335165] save_stack+0x19/0x80
>> [ 466.336240] __kasan_kmalloc.constprop.8+0xa0/0xd0
>> [ 466.337755] kmem_cache_alloc+0xe8/0x320
>> [ 466.339050] getname_flags+0xca/0x560
>> [ 466.340229] user_path_at_empty+0x2c/0x50
>> [ 466.341508] vfs_statx+0xe6/0x190
>> [ 466.342619] __do_sys_newstat+0x81/0x100
>> [ 466.343908] do_syscall_64+0xc8/0x600
>> [ 466.345303] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [ 466.347034]
>> [ 466.347517] Freed by task 2605:
>> [ 466.348471] save_stack+0x19/0x80
>> [ 466.349476] __kasan_slab_free+0x12e/0x180
>> [ 466.350726] kmem_cache_free+0xc8/0x430
>> [ 466.351874] putname+0xe2/0x120
>> [ 466.352921] filename_lookup+0x257/0x3e0
>> [ 466.354319] vfs_statx+0xe6/0x190
>> [ 466.355498] __do_sys_newstat+0x81/0x100
>> [ 466.356889] do_syscall_64+0xc8/0x600
>> [ 466.358037] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> [ 466.359567]
>> [ 466.360050] The buggy address belongs to the object at
>> ffff888372139100
>> [ 466.360050] which belongs to the cache names_cache of size 4096
>> [ 466.363735] The buggy address is located 336 bytes inside of
>> [ 466.363735] 4096-byte region [ffff888372139100, ffff88837213a100)
>> [ 466.367179] The buggy address belongs to the page:
>> [ 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
>>
>> 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.
>
>
> Good catch.
>
> Some comments inline.
>
>
>>
>> Fixes: eb0fb363f920 ("tuntap: attach queue 0 before registering
>> netdevice")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.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;
>> + }
>> }
>> 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);
>
>
> This looks just a duplicated of netdev->state? However it lacks
> sufficient synchronization like barriers or locks. How about:
It's not same, register_netdevice() will return error if
call_netdevice_notifiers() failed after dev->reg_state is set to
NETREG_REGISTERED.
>
> - call tun_set_real_num_queues() before register_netdevice() this can
> have the same result as what eb0fb363f920 did.
> - move tun_attach() after register_netdevice() this makes sure we
> won't publish tfile->tun until we are sure at least one refcnt is held
> by register_netdevice()?
Yes, I think this way is better, I will try this later.
>
> Thanks
>
>
> .
>
next prev parent reply other threads:[~2019-08-15 13:12 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-15 8:18 [PATCH] tun: fix use-after-free when register netdev failed Yang Yingliang
2019-08-15 9:21 ` Jason Wang
2019-08-15 12:55 ` Yang Yingliang [this message]
2019-08-15 9:35 ` Eric Dumazet
2019-08-15 13:01 ` Yang Yingliang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5D555659.60307@huawei.com \
--to=yangyingliang@huawei.com \
--cc=davem@davemloft.net \
--cc=jasowang@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.