netdev - Re: [PATCH iproute2] libnetlink: Fix memory leak in __rtnl_talk

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] [day] [month] [year] [list]

Message-ID: <20221201080024.719fe24e@hermes.local>
Date:   Thu, 1 Dec 2022 08:00:24 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Gilad Naaman <gnaaman@...venets.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Lahav Daniel Schlesinger <lschlesinger@...venets.com>
Subject: Re: [PATCH iproute2] libnetlink: Fix memory leak in
 __rtnl_talk_iov()

On Wed, 23 Nov 2022 16:15:41 +0000
Gilad Naaman <gnaaman@...venets.com> wrote:

> If `__rtnl_talk_iov()` fails then callers are not expected to free `answer`.
> 
> Currently if `NLMSG_ERROR` was received with an error then the netlink
> buffer was stored in `answer`, while still returning an error
> 
> This leak can be observed by running this snippet over time.
> This triggers an `NLMSG_ERROR` because for each neighbour update, `ip`
> will try to query for the name of interface 9999 in the wrong netns.
> (which in itself is a separate bug)
> 
> 	set -e
> 
> 	ip netns del test-a || true
> 	ip netns add test-a
> 	ip netns del test-b || true
> 	ip netns add test-b
> 
> 	ip -n test-a netns set test-b auto
> 	ip -n test-a link add veth_a index 9999 type veth peer name veth_b netns test-b
> 	ip -n test-b link set veth_b up
> 
> 	ip -n test-a monitor link address prefix neigh nsid label all-nsid > /dev/null &
> 	monitor_pid=$!
> 	clean() {
> 		kill $monitor_pid
> 		ip netns del test-a
> 		ip netns del test-b
> 	}
> 	trap clean EXIT
> 
> 	while true; do
> 		ip -n test-b neigh add dev veth_b 1.2.3.4 lladdr AA:AA:AA:AA:AA:AA
> 		ip -n test-b neigh del dev veth_b 1.2.3.4
> 	done
> 
> Fixes: 55870df ("Improve batch and dump times by caching link lookups")
> Signed-off-by: Lahav Schlesinger <lschlesinger@...venets.com>
> Signed-off-by: Gilad Naaman <gnaaman@...venets.com>
> ---
> lib/libnetlink.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)

The patch got mangled by MS Exchange.  Please resubmit with proper formatting.

$ checkpatch.pl /tmp/leak.mbox 
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#148: 
If `__rtnl_talk_iov()` fails then callers are not expected to free `answer`=

WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 55870dfe7f8b ("Improve batch and dump times by caching link lookups")'
#186: 
Fixes: 55870df ("Improve batch and dump times by caching link lookups")

ERROR: patch seems to be corrupt (line wrapped?)
#198: FILE: lib/libnetlink.c:1091:
						rtnl_talk_error(h, err, errfn);

WARNING: suspect code indent for conditional statements (32, 32)
#204: FILE: lib/libnetlink.c:1093:
+				if (i < iovlen) {
					free(buf);

WARNING: space prohibited between function name and open parenthesis '('
#218: FILE: lib/libnetlink.c:1102:
+					*answer =3D (struct nlmsghdr *)buf;

ERROR: spaces required around that '=' (ctx:WxV)
#218: FILE: lib/libnetlink.c:1102:
+					*answer =3D (struct nlmsghdr *)buf;
 					        ^

total: 2 errors, 4 warnings, 29 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/leak.mbox has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.