netdev - Re: [Patch net v2] tun: fix a memory leak for tfile->tx

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]

Date:   Mon, 15 Jan 2018 11:36:52 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Subject: Re: [Patch net v2] tun: fix a memory leak for tfile->tx_array

On Sun, Jan 14, 2018 at 11:07 PM, Jason Wang <jasowang@...hat.com> wrote:
>
>
> On 2018年01月14日 01:31, Cong Wang wrote:
>>
>> On Thu, Jan 11, 2018 at 2:16 AM, Jason Wang <jasowang@...hat.com> wrote:
>>>
>>> It looks to me what is actual missed is the cleanups tun_detach_all().
>>> For
>>> me the only case that could leak is
>>>
>>> open
>>> attach
>>> ip link del link dev tap0
>>> close or another set_iff()
>>>
>>> So in this case, clean during close is not sufficient since it could be
>>> attached to another device.
>>
>> In this case, close() still calls tun_detach() with clean=true, so
>> with my patch, the tx_array is still cleaned. What am I missing here?
>> Are you implying clean=true is not sufficient?
>
>
> Consider the corner case:
>
> 1) open
> 2) tun_set_iff() (which calls tun_attach to initialize skb_array)
> 3) ip link del link dev tap0 (which calls tun_detach_all())
> 4) tun_set_iff() (current codes does not forbid this and it will allocate
> skb array again)
>
> Consider the skb array was only initialized when attach it to a real device,
> we should do the cleanup when we detach it from a device which happens on
> two places:
>
> - actively: close to an tun fd (__tun_deatch())
> - passively: tun device was destroyed (tun_detach_all())

Fair enough, I will send out v3.

Thanks.