git: 22b58630d6ba - releng/13.0 - divert: Fix mbuf ownership confusion in div_output()

[Mark Johnston]

The branch releng/13.0 has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=22b58630d6ba256242a4622c70c2f68ca6892de2

commit 22b58630d6ba256242a4622c70c2f68ca6892de2
Author:     Mark Johnston <markj at FreeBSD.org>
AuthorDate: 2021-05-07 18:27:58 +0000
Commit:     Mark Johnston <markj at FreeBSD.org>
CommitDate: 2021-05-26 19:30:51 +0000

    divert: Fix mbuf ownership confusion in div_output()
    
    div_output_outbound() and div_output_inbound() relied on the caller to
    free the mbuf if an error occurred.  However, this is contrary to the
    semantics of their callees, ip_output(), ip6_output() and
    netisr_queue_src(), which always consume the mbuf.  So, if one of these
    functions returned an error, that would get propagated up to
    div_output(), resulting in a double free.
    
    Fix the problem by making div_output_outbound() and div_output_inbound()
    responsible for freeing the mbuf in all cases.
    
    Approved by:    so
    Security:       EN-21:12.divert
    Reported by:    Michael Schmiedgen <schmiedgen at gmx.net>
    Tested by:      Michael Schmiedgen
    Reviewed by:    donner
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D30129
    
    (cherry picked from commit a1fadf7de25b973a308b86d04c4ada4fa8be193f)
    (cherry picked from commit eafeee082c50850c2577f4fce0eaa7acb034f565)
---
 sys/netinet/ip_divert.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/sys/netinet/ip_divert.c b/sys/netinet/ip_divert.c
index 65f1d263b5fa..6e38740fe263 100644
--- a/sys/netinet/ip_divert.c
+++ b/sys/netinet/ip_divert.c
@@ -392,17 +392,13 @@ div_output(struct socket *so, struct mbuf *m, struct sockaddr_in *sin,
 	}
 	NET_EPOCH_EXIT(et);
 
-	if (error != 0)
-		m_freem(m);
-
 	return (error);
 }
 
 /*
  * Sends mbuf @m to the wire via ip[6]_output().
  *
- * Returns 0 on success, @m is consumed.
- * On failure, returns error code. It is caller responsibility to free @m.
+ * Returns 0 on success or an errno value on failure.  @m is always consumed.
  */
 static int
 div_output_outbound(int family, struct socket *so, struct mbuf *m)
@@ -425,6 +421,7 @@ div_output_outbound(int family, struct socket *so, struct mbuf *m)
 		    inp->inp_options != NULL) ||
 		    ((u_short)ntohs(ip->ip_len) > m->m_pkthdr.len)) {
 			INP_RUNLOCK(inp);
+			m_freem(m);
 			return (EINVAL);
 		}
 		break;
@@ -436,6 +433,7 @@ div_output_outbound(int family, struct socket *so, struct mbuf *m)
 		/* Don't allow packet length sizes that will crash */
 		if (((u_short)ntohs(ip6->ip6_plen) > m->m_pkthdr.len)) {
 			INP_RUNLOCK(inp);
+			m_freem(m);
 			return (EINVAL);
 		}
 		break;
@@ -475,6 +473,7 @@ div_output_outbound(int family, struct socket *so, struct mbuf *m)
 		options = m_dup(inp->inp_options, M_NOWAIT);
 		if (options == NULL) {
 			INP_RUNLOCK(inp);
+			m_freem(m);
 			return (ENOBUFS);
 		}
 	}
@@ -502,8 +501,7 @@ div_output_outbound(int family, struct socket *so, struct mbuf *m)
 /*
  * Schedules mbuf @m for local processing via IPv4/IPv6 netisr queue.
  *
- * Returns 0 on success, @m is consumed.
- * Returns error code on failure. It is caller responsibility to free @m.
+ * Returns 0 on success or an errno value on failure.  @m is always consumed.
  */
 static int
 div_output_inbound(int family, struct socket *so, struct mbuf *m,
@@ -523,8 +521,10 @@ div_output_inbound(int family, struct socket *so, struct mbuf *m,
 		bzero(sin->sin_zero, sizeof(sin->sin_zero));
 		sin->sin_port = 0;
 		ifa = ifa_ifwithaddr((struct sockaddr *) sin);
-		if (ifa == NULL)
+		if (ifa == NULL) {
+			m_freem(m);
 			return (EADDRNOTAVAIL);
+		}
 		m->m_pkthdr.rcvif = ifa->ifa_ifp;
 	}
 #ifdef MAC
@@ -550,6 +550,7 @@ div_output_inbound(int family, struct socket *so, struct mbuf *m,
 		break;
 #endif
 	default:
+		m_freem(m);
 		return (EINVAL);
 	}