mbuf leak ip_insertoptions

[Alexander Bluhm]

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

[Klemens Nanni-2]

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

[Vitaliy Makkoveev-3]

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