netdev - Re: BUG: spinlock bad magic in tun_do

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9992abe5-57e6-4c2c-601d-fead7e441696@redhat.com>
Date:   Wed, 9 May 2018 11:38:11 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     syzbot <syzbot+e8b902c3c3fadf0a9dba@...kaller.appspotmail.com>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Petar Penkov <peterpenkov96@...il.com>,
        Sabrina Dubroca <sd@...asysnail.net>,
        syzkaller-bugs@...glegroups.com
Subject: Re: BUG: spinlock bad magic in tun_do_read



On 2018年05月09日 10:50, Cong Wang wrote:
> On Mon, May 7, 2018 at 11:04 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>>
>> On 05/07/2018 10:54 PM, Cong Wang wrote:
>>> Yeah, we should return early before hitting this uninitialized ptr ring...
>>> Something like:
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index ef33950a45d9..638c87a95247 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -2128,6 +2128,9 @@ static void *tun_ring_recv(struct tun_file
>>> *tfile, int noblock, int *err)
>>>          void *ptr = NULL;
>>>          int error = 0;
>>>
>>> +       if (!tfile->tx_ring.queue)
>>> +               goto out;
>>> +
>>>
>>> Or, checking if tun is detached...
>>>
>>>
>> tx_ring was properly initialized when first ptr_ring_consume() at line 2131 was attempted.
>>
>> The bug happens later at line 2143 , after a schedule() call, line 2155
>>
>> So a single check at function prologue wont solve the case the thread had to sleep,
>> then some uninit happened.
>
> Very good point. RTNL lock is supposed to protect cleanup path, but I don't
> think we can acquire RTNL for tun_chr_read_iter() path...

I think the root cause is we try to initialize ptr ring during TUNSETIFF 
since the length depends on the dev->tx_queue_len and try to destroy it 
when device is gone. We can solve this by initializing a zero size 
ptr_ring during open() and resize if necessary. Then there no need for 
any workaround like memset and checking against NULL.

Let me try to cook a patch for this.

Thanks