Date: Fri, 7 May 2021 18:33:43 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: a1fadf7de25b - main - divert: Fix mbuf ownership confusion in div_output() Message-ID: <202105071833.147IXhFV079125@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=a1fadf7de25b973a308b86d04c4ada4fa8be193f commit a1fadf7de25b973a308b86d04c4ada4fa8be193f Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2021-05-07 18:27:58 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2021-05-07 18:31: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 <schmiedgen@gmx.net> Tested by: Michael Schmiedgen Reviewed by: donner MFC after: 3 days Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D30129 --- 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 65974051ad1f..f83a42cb36c9 100644 --- a/sys/netinet/ip_divert.c +++ b/sys/netinet/ip_divert.c @@ -415,17 +415,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) @@ -448,6 +444,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; @@ -459,6 +456,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; @@ -498,6 +496,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); } } @@ -525,8 +524,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, @@ -546,8 +544,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 @@ -573,6 +573,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?202105071833.147IXhFV079125>