linux-kernel - Re: [PATCH] tun: Fix use-after-free 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:   Tue, 30 Apr 2019 10:44:26 +0800
From:   YueHaibing <yuehaibing@...wei.com>
To:     Cong Wang <xiyou.wangcong@...il.com>,
        Jason Wang <jasowang@...hat.com>
CC:     "weiyongjun (A)" <weiyongjun1@...wei.com>,
        David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        "Jesper Dangaard Brouer" <brouer@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        "Li,Rongqing" <lirongqing@...du.com>,
        nicolas dichtel <nicolas.dichtel@...nd.com>,
        Chas Williams <3chas3@...il.com>, <wangli39@...du.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "Linux Kernel Network Developers" <netdev@...r.kernel.org>,
        Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH] tun: Fix use-after-free in tun_net_xmit



On 2019/4/30 0:38, Cong Wang wrote:
> On Sun, Apr 28, 2019 at 7:23 PM Jason Wang <jasowang@...hat.com> wrote:
>>
>>
>> On 2019/4/29 上午1:59, Cong Wang wrote:
>>> On Sun, Apr 28, 2019 at 12:51 AM Jason Wang <jasowang@...hat.com> wrote:
>>>>> tun_net_xmit() doesn't have the chance to
>>>>> access the change because it holding the rcu_read_lock().
>>>>
>>>>
>>>> The problem is the following codes:
>>>>
>>>>
>>>>          --tun->numqueues;
>>>>
>>>>          ...
>>>>
>>>>          synchronize_net();
>>>>
>>>> We need make sure the decrement of tun->numqueues be visible to readers
>>>> after synchronize_net(). And in tun_net_xmit():
>>>
>>> It doesn't matter at all. Readers are okay to read it even they still use the
>>> stale tun->numqueues, as long as the tfile is not freed readers can read
>>> whatever they want...
>>
>> This is only true if we set SOCK_RCU_FREE, isn't it?
> 
> 
> Sure, this is how RCU is supposed to work.
> 
>>
>>>
>>> The decrement of tun->numqueues is just how we unpublish the old
>>> tfile, it is still valid for readers to read it _after_ unpublish, we only need
>>> to worry about free, not about unpublish. This is the whole spirit of RCU.
>>>
>>
>> The point is we don't convert tun->numqueues to RCU but use
>> synchronize_net().
> 
> Why tun->numqueues needs RCU? It is an integer, and reading a stale
> value is _perfectly_ fine.
> 
> If you actually meant to say tun->tfiles[] itself, no, it is a fixed-size array,
> it doesn't shrink or grow, so we don't need RCU for it. This is also why
> a stale tun->numqueues is fine, as long as it never goes out-of-bound.
> 
> 
>>
>>> You need to rethink about my SOCK_RCU_FREE patch.
>>
>> The code is wrote before SOCK_RCU_FREE is introduced and assume no
>> de-reference from device after synchronize_net(). It doesn't harm to
>> figure out the root cause which may give us more confidence to the fix
>> (e.g like SOCK_RCU_FREE).
> 
> I believe SOCK_RCU_FREE is the fix for the root cause, not just a
> cover-up.

With SOCK_RCU_FREE tfile is ok ,

but tfile->sk is freed by sock_put in __tun_detach, it will trgger

use-after-free in tun_net_xmit if tun->numqueues check passed.

> 
> 
>>
>> I don't object to fix with SOCK_RCU_FREE, but then we should remove
>> the redundant synchronize_net(). But I still prefer to synchronize
>> everything explicitly like (completely untested):
> 
> I agree that synchronize_net() can be removed. However I don't
> understand your untested patch at all, it looks like to fix a completely
> different problem rather than this use-after-free.
> 
> Thanks.
> 
> .
>