From owner-dev-commits-src-all@freebsd.org Mon May 10 13:49:32 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id D02A2637D4E; Mon, 10 May 2021 13:49:32 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Ff2V45BlKz3PP6; Mon, 10 May 2021 13:49:32 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 9B0E2258F2; Mon, 10 May 2021 13:49:32 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 14ADnWpo026474; Mon, 10 May 2021 13:49:32 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 14ADnWq2026473; Mon, 10 May 2021 13:49:32 GMT (envelope-from git) Date: Mon, 10 May 2021 13:49:32 GMT Message-Id: <202105101349.14ADnWq2026473@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: eafeee082c50 - stable/13 - divert: Fix mbuf ownership confusion in div_output() MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: eafeee082c50850c2577f4fce0eaa7acb034f565 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 May 2021 13:49:32 -0000 The branch stable/13 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=eafeee082c50850c2577f4fce0eaa7acb034f565 commit eafeee082c50850c2577f4fce0eaa7acb034f565 Author: Mark Johnston AuthorDate: 2021-05-07 18:27:58 +0000 Commit: Mark Johnston CommitDate: 2021-05-10 13:36:08 +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. Reported by: Michael Schmiedgen Tested by: Michael Schmiedgen Reviewed by: donner Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D30129 (cherry picked from commit a1fadf7de25b973a308b86d04c4ada4fa8be193f) --- 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 c3f9c43b8f70..31b28656ece7 100644 --- a/sys/netinet/ip_divert.c +++ b/sys/netinet/ip_divert.c @@ -402,17 +402,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) @@ -435,6 +431,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; @@ -446,6 +443,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; @@ -485,6 +483,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); } } @@ -512,8 +511,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, @@ -533,8 +531,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 @@ -560,6 +560,7 @@ div_output_inbound(int family, struct socket *so, struct mbuf *m, break; #endif default: + m_freem(m); return (EINVAL); }