From owner-svn-src-all@freebsd.org Wed Oct 16 16:57:24 2019 Return-Path: Delivered-To: svn-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 93EC614B159; Wed, 16 Oct 2019 16:57:24 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (glebi.us [162.251.186.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 46tdkr2Hc1z41rt; Wed, 16 Oct 2019 16:57:23 +0000 (UTC) (envelope-from glebius@freebsd.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id x9GGvM45040750 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Wed, 16 Oct 2019 09:57:22 -0700 (PDT) (envelope-from glebius@freebsd.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id x9GGvMQr040749; Wed, 16 Oct 2019 09:57:22 -0700 (PDT) (envelope-from glebius@freebsd.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@freebsd.org using -f Date: Wed, 16 Oct 2019 09:57:22 -0700 From: Gleb Smirnoff To: Hans Petter Selasky Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r353635 - in head/sys: netinet netinet6 Message-ID: <20191016165722.GU4086@FreeBSD.org> References: <201910160911.x9G9BonH076337@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201910160911.x9G9BonH076337@repo.freebsd.org> User-Agent: Mutt/1.12.2 (2019-09-21) X-Rspamd-Queue-Id: 46tdkr2Hc1z41rt X-Spamd-Bar: ----- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-5.99 / 15.00]; NEURAL_HAM_MEDIUM(-0.99)[-0.991,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Oct 2019 16:57:24 -0000 Hans, as far as I remember I was against this changeset and I had several other developers agreed that this should be fixed in different way. Why did you proceed with checking it in? :( On Wed, Oct 16, 2019 at 09:11:50AM +0000, Hans Petter Selasky wrote: H> Author: hselasky H> Date: Wed Oct 16 09:11:49 2019 H> New Revision: 353635 H> URL: https://svnweb.freebsd.org/changeset/base/353635 H> H> Log: H> Fix panic in network stack due to use after free when receiving H> partial fragmented packets before a network interface is detached. H> H> When sending IPv4 or IPv6 fragmented packets and a fragment is lost H> before the network device is freed, the mbuf making up the fragment H> will remain in the temporary hashed fragment list and cause a panic H> when it times out due to accessing a freed network interface H> structure. H> H> H> 1) Make sure the m_pkthdr.rcvif always points to a valid network H> interface. Else the rcvif field should be set to NULL. H> H> 2) Use the rcvif of the last received fragment as m_pkthdr.rcvif for H> the fully defragged packet, instead of the first received fragment. H> H> Panic backtrace for IPv6: H> H> panic() H> icmp6_reflect() # tries to access rcvif->if_afdata[AF_INET6]->xxx H> icmp6_error() H> frag6_freef() H> frag6_slowtimo() H> pfslowtimo() H> softclock_call_cc() H> softclock() H> ithread_loop() H> H> Reviewed by: bz H> Differential Revision: https://reviews.freebsd.org/D19622 H> MFC after: 1 week H> Sponsored by: Mellanox Technologies H> H> Modified: H> head/sys/netinet/ip_reass.c H> head/sys/netinet6/frag6.c H> H> Modified: head/sys/netinet/ip_reass.c H> ============================================================================== H> --- head/sys/netinet/ip_reass.c Wed Oct 16 09:04:53 2019 (r353634) H> +++ head/sys/netinet/ip_reass.c Wed Oct 16 09:11:49 2019 (r353635) H> @@ -47,7 +47,10 @@ __FBSDID("$FreeBSD$"); H> #include H> #include H> #include H> +#include H> H> +#include H> +#include H> #include H> #include H> #include H> @@ -181,6 +184,7 @@ ip_reass(struct mbuf *m) H> struct ip *ip; H> struct mbuf *p, *q, *nq, *t; H> struct ipq *fp; H> + struct ifnet *srcifp; H> struct ipqhead *head; H> int i, hlen, next, tmpmax; H> u_int8_t ecn, ecn0; H> @@ -241,6 +245,11 @@ ip_reass(struct mbuf *m) H> } H> H> /* H> + * Store receive network interface pointer for later. H> + */ H> + srcifp = m->m_pkthdr.rcvif; H> + H> + /* H> * Attempt reassembly; if it succeeds, proceed. H> * ip_reass() will return a different mbuf. H> */ H> @@ -490,8 +499,11 @@ ip_reass(struct mbuf *m) H> m->m_len += (ip->ip_hl << 2); H> m->m_data -= (ip->ip_hl << 2); H> /* some debugging cruft by sklower, below, will go away soon */ H> - if (m->m_flags & M_PKTHDR) /* XXX this should be done elsewhere */ H> + if (m->m_flags & M_PKTHDR) { /* XXX this should be done elsewhere */ H> m_fixhdr(m); H> + /* set valid receive interface pointer */ H> + m->m_pkthdr.rcvif = srcifp; H> + } H> IPSTAT_INC(ips_reassembled); H> IPQ_UNLOCK(hash); H> H> @@ -607,6 +619,43 @@ ipreass_drain(void) H> } H> } H> H> +/* H> + * Drain off all datagram fragments belonging to H> + * the given network interface. H> + */ H> +static void H> +ipreass_cleanup(void *arg __unused, struct ifnet *ifp) H> +{ H> + struct ipq *fp, *temp; H> + struct mbuf *m; H> + int i; H> + H> + KASSERT(ifp != NULL, ("%s: ifp is NULL", __func__)); H> + H> + /* H> + * Skip processing if IPv4 reassembly is not initialised or H> + * torn down by ipreass_destroy(). H> + */ H> + if (V_ipq_zone == NULL) H> + return; H> + H> + CURVNET_SET_QUIET(ifp->if_vnet); H> + for (i = 0; i < IPREASS_NHASH; i++) { H> + IPQ_LOCK(i); H> + /* Scan fragment list. */ H> + TAILQ_FOREACH_SAFE(fp, &V_ipq[i].head, ipq_list, temp) { H> + for (m = fp->ipq_frags; m != NULL; m = m->m_nextpkt) { H> + /* clear no longer valid rcvif pointer */ H> + if (m->m_pkthdr.rcvif == ifp) H> + m->m_pkthdr.rcvif = NULL; H> + } H> + } H> + IPQ_UNLOCK(i); H> + } H> + CURVNET_RESTORE(); H> +} H> +EVENTHANDLER_DEFINE(ifnet_departure_event, ipreass_cleanup, NULL, 0); H> + H> #ifdef VIMAGE H> /* H> * Destroy IP reassembly structures. H> @@ -617,6 +666,7 @@ ipreass_destroy(void) H> H> ipreass_drain(); H> uma_zdestroy(V_ipq_zone); H> + V_ipq_zone = NULL; H> for (int i = 0; i < IPREASS_NHASH; i++) H> mtx_destroy(&V_ipq[i].lock); H> } H> H> Modified: head/sys/netinet6/frag6.c H> ============================================================================== H> --- head/sys/netinet6/frag6.c Wed Oct 16 09:04:53 2019 (r353634) H> +++ head/sys/netinet6/frag6.c Wed Oct 16 09:11:49 2019 (r353635) H> @@ -246,7 +246,7 @@ frag6_freef(struct ip6q *q6, uint32_t bucket) H> * Return ICMP time exceeded error for the 1st fragment. H> * Just free other fragments. H> */ H> - if (af6->ip6af_off == 0) { H> + if (af6->ip6af_off == 0 && m->m_pkthdr.rcvif != NULL) { H> H> /* Adjust pointer. */ H> ip6 = mtod(m, struct ip6_hdr *); H> @@ -272,6 +272,43 @@ frag6_freef(struct ip6q *q6, uint32_t bucket) H> } H> H> /* H> + * Drain off all datagram fragments belonging to H> + * the given network interface. H> + */ H> +static void H> +frag6_cleanup(void *arg __unused, struct ifnet *ifp) H> +{ H> + struct ip6q *q6, *q6n, *head; H> + struct ip6asfrag *af6; H> + struct mbuf *m; H> + int i; H> + H> + KASSERT(ifp != NULL, ("%s: ifp is NULL", __func__)); H> + H> + CURVNET_SET_QUIET(ifp->if_vnet); H> + for (i = 0; i < IP6REASS_NHASH; i++) { H> + IP6QB_LOCK(i); H> + head = IP6QB_HEAD(i); H> + /* Scan fragment list. */ H> + for (q6 = head->ip6q_next; q6 != head; q6 = q6n) { H> + q6n = q6->ip6q_next; H> + H> + for (af6 = q6->ip6q_down; af6 != (struct ip6asfrag *)q6; H> + af6 = af6->ip6af_down) { H> + m = IP6_REASS_MBUF(af6); H> + H> + /* clear no longer valid rcvif pointer */ H> + if (m->m_pkthdr.rcvif == ifp) H> + m->m_pkthdr.rcvif = NULL; H> + } H> + } H> + IP6QB_UNLOCK(i); H> + } H> + CURVNET_RESTORE(); H> +} H> +EVENTHANDLER_DEFINE(ifnet_departure_event, frag6_cleanup, NULL, 0); H> + H> +/* H> * Like in RFC2460, in RFC8200, fragment and reassembly rules do not agree with H> * each other, in terms of next header field handling in fragment header. H> * While the sender will use the same value for all of the fragmented packets, H> @@ -307,6 +344,7 @@ int H> frag6_input(struct mbuf **mp, int *offp, int proto) H> { H> struct ifnet *dstifp; H> + struct ifnet *srcifp; H> struct in6_ifaddr *ia6; H> struct ip6_hdr *ip6; H> struct ip6_frag *ip6f; H> @@ -338,6 +376,11 @@ frag6_input(struct mbuf **mp, int *offp, int proto) H> return (IPPROTO_DONE); H> #endif H> H> + /* H> + * Store receive network interface pointer for later. H> + */ H> + srcifp = m->m_pkthdr.rcvif; H> + H> dstifp = NULL; H> /* Find the destination interface of the packet. */ H> ia6 = in6ifa_ifwithaddr(&ip6->ip6_dst, 0 /* XXX */); H> @@ -534,6 +577,9 @@ frag6_input(struct mbuf **mp, int *offp, int proto) H> frag6_deq(af6, bucket); H> free(af6, M_FRAG6); H> H> + /* Set a valid receive interface pointer. */ H> + merr->m_pkthdr.rcvif = srcifp; H> + H> /* Adjust pointer. */ H> ip6err = mtod(merr, struct ip6_hdr *); H> H> @@ -720,6 +766,8 @@ insert: H> for (t = m; t; t = t->m_next) H> plen += t->m_len; H> m->m_pkthdr.len = plen; H> + /* Set a valid receive interface pointer. */ H> + m->m_pkthdr.rcvif = srcifp; H> } H> H> #ifdef RSS H> _______________________________________________ H> svn-src-all@freebsd.org mailing list H> https://lists.freebsd.org/mailman/listinfo/svn-src-all H> To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" -- Gleb Smirnoff