Date: Wed, 26 May 2021 20:37:45 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 22b58630d6ba - releng/13.0 - divert: Fix mbuf ownership confusion in div_output() Message-ID: <202105262037.14QKbjv5054401@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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@FreeBSD.org> AuthorDate: 2021-05-07 18:27:58 +0000 Commit: Mark Johnston <markj@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@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); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202105262037.14QKbjv5054401>