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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 15 Jan 2018 15:07:41 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.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 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())

Thanks