Hi,
ip_insertoptions() may prepend a mbuf. In this case "goto bad" has to free the new chain. Currently we leak the new mbuf in front of the old chain. NetBSD has fixed this bug here: ---------------------------- revision 1.33 date: 1996-10-11 18:19:08 +0000; author: is; state: Exp; lines: +2 -2; Fix a mbuf leak in ip_output(). Scenario: If ip_insertoptions() prepends a new mbuf to the chain, the bad: label's m_freem(m0) still would free only the original mbuf chain if the transmission failed for, e.g., no route to host; resulting in one lost mbuf per failed packet. (The original posting included a demonstration program). Original report of this bug was by [hidden email] (JINMEI Tatuya) on comp.bugs.4bsd. ---------------------------- Free m instead of m0 in the bad case. This allows to simplify a bunch of goto done. ok? bluhm Index: netinet/ip_output.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v retrieving revision 1.365 diff -u -p -r1.365 ip_output.c --- netinet/ip_output.c 10 Feb 2021 18:28:06 -0000 1.365 +++ netinet/ip_output.c 22 Feb 2021 15:48:38 -0000 @@ -111,9 +111,6 @@ ip_output(struct mbuf *m0, struct mbuf * #if NPF > 0 u_int orig_rtableid; #endif -#ifdef MROUTING - int rv; -#endif NET_ASSERT_LOCKED(); @@ -250,8 +247,7 @@ reroute: /* Should silently drop packet */ if (error == -EINVAL) error = 0; - m_freem(m); - goto done; + goto bad; } if (tdb != NULL) { /* @@ -348,13 +344,13 @@ reroute: */ if (ipmforwarding && ip_mrouter[ifp->if_rdomain] && (flags & IP_FORWARDING) == 0) { + int rv; + KERNEL_LOCK(); rv = ip_mforward(m, ifp); KERNEL_UNLOCK(); - if (rv != 0) { - m_freem(m); - goto done; - } + if (rv != 0) + goto bad; } } #endif @@ -366,10 +362,8 @@ reroute: * loop back a copy if this host actually belongs to the * destination group on the loopback interface. */ - if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0) { - m_freem(m); - goto done; - } + if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0) + goto bad; goto sendit; } @@ -427,8 +421,7 @@ sendit: if (pf_test(AF_INET, (flags & IP_FORWARDING) ? PF_FWD : PF_OUT, ifp, &m) != PF_PASS) { error = EACCES; - m_freem(m); - goto done; + goto bad; } if (m == NULL) goto done; @@ -453,8 +446,7 @@ sendit: if (ipsec_in_use && (flags & IP_FORWARDING) && (ipforwarding == 2) && (m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL) == NULL)) { error = EHOSTUNREACH; - m_freem(m); - goto done; + goto bad; } #endif @@ -534,7 +526,7 @@ done: if_put(ifp); return (error); bad: - m_freem(m0); + m_freem(m); goto done; } |
On Mon, Feb 22, 2021 at 04:51:09PM +0100, Alexander Bluhm wrote:
> Free m instead of m0 in the bad case. This allows to simplify a > bunch of goto done. OK kn |
In reply to this post by Alexander Bluhm
ok mvs@
> On 22 Feb 2021, at 18:51, Alexander Bluhm <[hidden email]> wrote: > > Hi, > > ip_insertoptions() may prepend a mbuf. In this case "goto bad" has > to free the new chain. Currently we leak the new mbuf in front of > the old chain. NetBSD has fixed this bug here: > > ---------------------------- > revision 1.33 > date: 1996-10-11 18:19:08 +0000; author: is; state: Exp; lines: +2 -2; > Fix a mbuf leak in ip_output(). > > Scenario: If ip_insertoptions() prepends a new mbuf to the chain, the > bad: label's m_freem(m0) still would free only the original mbuf chain > if the transmission failed for, e.g., no route to host; resulting in > one lost mbuf per failed packet. (The original posting included a > demonstration program). > > Original report of this bug was by [hidden email] > (JINMEI Tatuya) on comp.bugs.4bsd. > ---------------------------- > > Free m instead of m0 in the bad case. This allows to simplify a > bunch of goto done. > > ok? > > bluhm > > Index: netinet/ip_output.c > =================================================================== > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v > retrieving revision 1.365 > diff -u -p -r1.365 ip_output.c > --- netinet/ip_output.c 10 Feb 2021 18:28:06 -0000 1.365 > +++ netinet/ip_output.c 22 Feb 2021 15:48:38 -0000 > @@ -111,9 +111,6 @@ ip_output(struct mbuf *m0, struct mbuf * > #if NPF > 0 > u_int orig_rtableid; > #endif > -#ifdef MROUTING > - int rv; > -#endif > > NET_ASSERT_LOCKED(); > > @@ -250,8 +247,7 @@ reroute: > /* Should silently drop packet */ > if (error == -EINVAL) > error = 0; > - m_freem(m); > - goto done; > + goto bad; > } > if (tdb != NULL) { > /* > @@ -348,13 +344,13 @@ reroute: > */ > if (ipmforwarding && ip_mrouter[ifp->if_rdomain] && > (flags & IP_FORWARDING) == 0) { > + int rv; > + > KERNEL_LOCK(); > rv = ip_mforward(m, ifp); > KERNEL_UNLOCK(); > - if (rv != 0) { > - m_freem(m); > - goto done; > - } > + if (rv != 0) > + goto bad; > } > } > #endif > @@ -366,10 +362,8 @@ reroute: > * loop back a copy if this host actually belongs to the > * destination group on the loopback interface. > */ > - if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0) { > - m_freem(m); > - goto done; > - } > + if (ip->ip_ttl == 0 || (ifp->if_flags & IFF_LOOPBACK) != 0) > + goto bad; > > goto sendit; > } > @@ -427,8 +421,7 @@ sendit: > if (pf_test(AF_INET, (flags & IP_FORWARDING) ? PF_FWD : PF_OUT, > ifp, &m) != PF_PASS) { > error = EACCES; > - m_freem(m); > - goto done; > + goto bad; > } > if (m == NULL) > goto done; > @@ -453,8 +446,7 @@ sendit: > if (ipsec_in_use && (flags & IP_FORWARDING) && (ipforwarding == 2) && > (m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL) == NULL)) { > error = EHOSTUNREACH; > - m_freem(m); > - goto done; > + goto bad; > } > #endif > > @@ -534,7 +526,7 @@ done: > if_put(ifp); > return (error); > bad: > - m_freem(m0); > + m_freem(m); > goto done; > } > > |
Free forum by Nabble | Edit this page |