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
| ||
|
Date: Mon, 5 Feb 2018 10:39:53 +0200 From: Tommi Rantala <tommi.t.rantala@...ia.com> To: netdev@...r.kernel.org Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, linux-sctp@...r.kernel.org, Alexey Kodanev <alexey.kodanev@...cle.com> Subject: Re: [PATCH net] sctp: fix dst reference leak in sctp_v4_get_dst On 05.02.2018 03:01, Marcelo Ricardo Leitner wrote: > Hi, > > On Sun, Feb 04, 2018 at 11:02:39AM +0200, Tommi Rantala wrote: >> Fix dst reference leak in sctp_v4_get_dst() introduced in commit >> 410f03831 ("sctp: add routing output fallback"): >> >> When walking the address_list, successive ip_route_output_key() calls >> may return the same rt->dst with the reference incremented on each call. >> >> The code would not decrement the dst refcount when the dst pointer was >> identical from the previous iteration, causing the dst leak. Ugh, I think this patch still does not get the refcounting right, please don't apply. For example: - on first iteration of the loop: "odev" check fails, and we set dst with refcnt=1. - on second iteration: the same dst refcnt is incremented to 2 via ip_route_output_key(). "odev" check succeeds. dst == &rt->dst, so we do not touch the refcnt. break out of the loop => oops, dst has refcnt=2 >> Testcase: >> ip netns add TEST >> ip netns exec TEST ip link set lo up >> ip link add dummy0 type dummy >> ip link add dummy1 type dummy >> ip link add dummy2 type dummy >> ip link set dev dummy0 netns TEST >> ip link set dev dummy1 netns TEST >> ip link set dev dummy2 netns TEST >> ip netns exec TEST ip addr add 192.168.1.1/24 dev dummy0 >> ip netns exec TEST ip link set dummy0 up >> ip netns exec TEST ip addr add 192.168.1.2/24 dev dummy1 >> ip netns exec TEST ip link set dummy1 up >> ip netns exec TEST ip addr add 192.168.1.3/24 dev dummy2 >> ip netns exec TEST ip link set dummy2 up >> ip netns exec TEST sctp_test -H 192.168.1.2 -P 20002 -h 192.168.1.1 -p 20000 -s -B 192.168.1.3 >> ip netns del TEST > > Thanks for the detailed steps. I'll adapt and include it on > github/sctp/sctp-tests ok, good! >> In 4.4 and 4.9 kernels this results to: > > I wonder why you couldn't reproduce this on newer kernels. Maybe > something masked it? I didn't check what exactly changed here. commit 955ec4cb3b54c7c389a9f830be7d3ae2056b9212 was mentioned elsewhere. Also the commits by Wei Wang changed the dst handling, see e.g. 5b7c9a8ff828287af5aebe93e707271bf1a82cc3 >> [ 354.179591] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> [ 364.419674] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> [ 374.663664] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> [ 384.903717] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> [ 395.143724] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> [ 405.383645] unregister_netdevice: waiting for lo to become free. Usage count = 1 >> ... >> >> Fixes: 410f03831 ("sctp: add routing output fallback") >> Acked-by: Neil Horman <nhorman@...driver.com> >> Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com> >> Cc: Alexey Kodanev <alexey.kodanev@...cle.com> >> Signed-off-by: Tommi Rantala <tommi.t.rantala@...ia.com> >> --- >> net/sctp/protocol.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c >> index 8b4ff315695e..aadb9e7f60e2 100644 >> --- a/net/sctp/protocol.c >> +++ b/net/sctp/protocol.c >> @@ -508,21 +508,20 @@ static void sctp_v4_get_dst(struct sctp_transport *t, union sctp_addr *saddr, >> if (IS_ERR(rt)) >> continue; >> >> - if (!dst) >> - dst = &rt->dst; >> - >> /* Ensure the src address belongs to the output >> * interface. >> */ >> odev = __ip_dev_find(sock_net(sk), laddr->a.v4.sin_addr.s_addr, >> false); >> if (!odev || odev->ifindex != fl4->flowi4_oif) { >> - if (&rt->dst != dst) >> + if (!dst) >> + dst = &rt->dst; >> + else >> dst_release(&rt->dst); >> continue; >> } >> >> - if (dst != &rt->dst) >> + if (dst && dst != &rt->dst) >> dst_release(dst); >> dst = &rt->dst; >> break; >> -- >> 2.15.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in >> the body of a message to majordomo@...r.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
Powered by blists - more mailing lists