netdev - Re: [PATCH] netlink: fix memory leak of dump

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:   Sun, 22 Jul 2018 18:39:25 +0200
From:   Florian Westphal <fw@...len.de>
To:     Shaochun Chen <cscnull@...il.com>
Cc:     pablo@...filter.org, kadlec@...ckhole.kfki.hu, fw@...len.de,
        davem@...emloft.net, johannes.berg@...el.com, Jason@...c4.com,
        ktkhai@...tuozzo.com, lucien.xin@...il.com,
        xiyou.wangcong@...il.com, dsahern@...il.com,
        netfilter-devel@...r.kernel.org, tom@...ntonium.net,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netlink: fix memory leak of dump

Shaochun Chen <cscnull@...il.com> wrote:

[ CC ing Tom, as he added ->start() ]

> 1) if netlink_dump_start start fail, the memory of c->data will leak.
> so free manually after netlink_dump_start return error.
> 
> 2) In netlink_dump_start, ignore the return of netlink_dump.
> Because if cb_running is set to true, cb->dump will be call in anyway.
> so if netlink_dump_start start successfully, just return -EINTR.

This now requires ->start() to not return -EINVAL, else we won't
release resources either, this seems fragile to me.

I can only think of three ways to address this:

1. Kill ->start() and force all users to defer
   state allocation to first ->dump() invocation, so existing
   ->done() can be used to release resources.

OR
2. add ->stop() and have core always pair it with ->start(),
   no matter if dump() got called or not, then convert all
   places that provide .start to use .stop, not .done.

OR
3. change meaning of ->done() so its always called once ->start()
   was invoked (and returned 0), this requires audit of all
   places that provide .done to make sure they won't trip.

3) seems to be what Tom intended when he added .start, so probably
best to investigate that first.