From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: route reference leak
To: Vitaliy Makkoveev <otto@bsdbox.dev>
Cc: tech@openbsd.org
Date: Mon, 8 Apr 2024 21:03:41 +0200

On Mon, Apr 08, 2024 at 08:38:08PM +0300, Vitaliy Makkoveev wrote:
> > On 6 Apr 2024, at 18:18, Alexander Bluhm <bluhm@openbsd.org> wrote:
> > 
> > Hi,
> > 
> > We are leaking a route reference counter in ip_output() and
> > ip6_output().
> > 
> > If no struct route was passed to ip_output(), it uses its own iproute
> > on the stack.  If that is the case, we have to free any route entry
> > that we put into the local route cache.
> > 
> > In the pf reroute case, we reset struct route to NULL.  Before that,
> > free the route entry if it was allocated locally on the stack.
> > 
> > ok?
> > 
> 
> It???s safe to pass NULL to rtfree(), you don't need to check
> ro->ro_rt.

I did this to be consistent with existing code.  But we can also
simplify old and new checks.

ok?

bluhm

Index: netinet/ip_output.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
diff -u -p -r1.396 ip_output.c
--- netinet/ip_output.c	22 Feb 2024 14:25:58 -0000	1.396
+++ netinet/ip_output.c	8 Apr 2024 18:56:17 -0000
@@ -417,6 +417,8 @@ sendit:
 	else if (m->m_pkthdr.pf.flags & PF_TAG_REROUTE) {
 		/* tag as generated to skip over pf_test on rerun */
 		m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
+		if (ro == &iproute)
+			rtfree(ro->ro_rt);
 		ro = NULL;
 		if_put(ifp); /* drop reference since target changed */
 		ifp = NULL;
@@ -481,7 +483,7 @@ sendit:
 	ipstat_inc(ips_fragmented);
 
 done:
-	if (ro == &iproute && ro->ro_rt)
+	if (ro == &iproute)
 		rtfree(ro->ro_rt);
 	if_put(ifp);
 #ifdef IPSEC
Index: netinet6/ip6_output.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_output.c,v
diff -u -p -r1.288 ip6_output.c
--- netinet6/ip6_output.c	28 Feb 2024 10:57:20 -0000	1.288
+++ netinet6/ip6_output.c	8 Apr 2024 18:57:35 -0000
@@ -635,6 +635,8 @@ reroute:
 		/* tag as generated to skip over pf_test on rerun */
 		m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
 		finaldst = ip6->ip6_dst;
+		if (ro == &iproute)
+			rtfree(ro->ro_rt);
 		ro = NULL;
 		if_put(ifp); /* drop reference since destination changed */
 		ifp = NULL;
@@ -758,11 +760,10 @@ reroute:
  bad:
 	m_freem(m);
  done:
-	if (ro == &iproute && ro->ro_rt) {
+	if (ro == &iproute)
 		rtfree(ro->ro_rt);
-	} else if (ro_pmtu == &iproute && ro_pmtu->ro_rt) {
+	else if (ro_pmtu == &iproute)
 		rtfree(ro_pmtu->ro_rt);
-	}
 	if_put(ifp);
 #ifdef IPSEC
 	tdb_unref(tdb);