netdev - Re: BUG: KASAN: use-after-free Read in tun_net

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:   Wed, 24 Apr 2019 17:11:48 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     YueHaibing <yuehaibing@...wei.com>, netdev <netdev@...r.kernel.org>
Subject: Re: BUG: KASAN: use-after-free Read in tun_net_xmit


On 2019/4/24 上午12:41, Cong Wang wrote:
> On Mon, Apr 22, 2019 at 11:42 PM Jason Wang <jasowang@...hat.com> wrote:
>>
>> On 2019/4/23 下午2:00, Cong Wang wrote:
>>> On Mon, Apr 22, 2019 at 2:41 AM Jason Wang <jasowang@...hat.com> wrote:
>>>> On 2019/4/22 上午11:57, YueHaibing wrote:
>>>>> We get a KASAN report as below, but don't have any reproducer.
>>>>>
>>>>> Any comments are appreciated.
>>>>>
>>>>> ==================================================================
>>>>> BUG: KASAN: use-after-free in tun_net_xmit+0x1670/0x1750 drivers/net/tun.c:1104
>>>>> Read of size 8 at addr ffff88836cc26a70 by task swapper/3/0
>>>> Which kernel version did you use? The calltrace points out the a use
>>>> after free for tun_file structure which should be synchronized through
>>>> RCU + RTNL lock.
>>> The tfile socket has to be marked with SOCK_RCU_FREE in order
>>> to fully respect the RCU grace period.
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index e9ca1c088d0b..31c3210288cb 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -3431,6 +3431,7 @@ static int tun_chr_open(struct inode *inode,
>>> struct file * file)
>>>           file->private_data = tfile;
>>>           INIT_LIST_HEAD(&tfile->next);
>>>
>>> +       sock_set_flag(&tfile->sk, SOCK_RCU_FREE);
>>>           sock_set_flag(&tfile->sk, SOCK_ZEROCOPY);
>>>
>>>           return 0;
>>
>> We did a synchronize_net() when socket is detached from netdevice in
>> __tun_detach() so it looks to me this is unnecessary.
> I knew, but it is only called conditionally, that is:
>
>   695         if (tun && !tfile->detached) {
> ...
>   710
>   711                 synchronize_net();
>
> And it looks like syzbot just skipped this condition,


If tfile is detached, it should have gone for the path of 
synchronize_net() before. If tfile is never attached, tun_net_xmit() 
doesn't have the chance to access that. I wonder whether or not we 
should use WRITE_ONCE() for tun->numqueues-- in this fucntion. If the 
value was not committed to memory before synchronize_net(), we may race 
with tun_net_xmit() which check txq against tun->numqueues.


> this is why I believe
> you still need to respect RCU grace period _unconditionally_ for tfile.


This is true if I miss subtle race in the code.


Haibing: could you please try the following test?

1) start VM with multiple queue

2) using pktgen to inject packets to all queues through tap

3) using ethtool to change the combined channels in guest in a loop

4) kill the guest


Thanks


> Thanks.