From: Vitaliy Makkoveev <otto@bsdbox.dev>
Subject: Re: route reference leak
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 8 Apr 2024 22:06:23 +0300

> On 8 Apr 2024, at 22:03, Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> 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?
> 

sure, ok mvs

> 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);