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 <sys/lock.h> H> #include <sys/mutex.h> H> #include <sys/sysctl.h> H> +#include <sys/socket.h> H> H> +#include <net/if.h> H> +#include <net/if_var.h> H> #include <net/rss_config.h> H> #include <net/netisr.h> H> #include <net/vnet.h> 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-unsubscr...@freebsd.org" -- Gleb Smirnoff _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
