Re: panic: TCP header not in one mbuf

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:No, it still panics at the same place with you patch applied.
:I also updated the source to the latest(just before the update of
:newvers.sh) and compiled the kernel with gcc2, but the same panic.

    Ok.  Further investigation has revealed that if the ip_mport() function
    has to m_pullup() an mbuf and succeeds, the caller of ip_mport() will
    still use the original (possibly now freed) mbuf rather then the one
    modified by ip_mport().  Worse, if an error occurs in ip_mport() the
    mbuf is not always freed, and the caller always frees it, leading to
    a potential double-free.

    Try this patch.

						-Matt

Index: net/netisr.c
===================================================================
RCS file: /cvs/src/sys/net/netisr.c,v
retrieving revision 1.20
diff -u -r1.20 netisr.c
--- net/netisr.c	16 Jul 2004 05:48:08 -0000	1.20
+++ net/netisr.c	18 Jul 2004 04:52:56 -0000
@@ -242,7 +242,7 @@
 	return (EIO);
     }
 
-    if (!(port = ni->ni_mport(m)))
+    if ((port = ni->ni_mport(&m)) == NULL)
 	return (EIO);
 
     /* use better message allocation system with limits later XXX JH */
@@ -281,7 +281,7 @@
  * Return message port for default handler thread on CPU 0.
  */
 lwkt_port_t
-cpu0_portfn(struct mbuf *m)
+cpu0_portfn(struct mbuf **mptr)
 {
     return (&netisr_cpu[0].td_msgport);
 }
Index: net/netisr.h
===================================================================
RCS file: /cvs/src/sys/net/netisr.h,v
retrieving revision 1.19
diff -u -r1.19 netisr.h
--- net/netisr.h	8 Jul 2004 22:07:34 -0000	1.19
+++ net/netisr.h	18 Jul 2004 04:53:32 -0000
@@ -199,7 +199,7 @@
 int netmsg_so_notify(lwkt_msg_t);
 int netmsg_so_notify_abort(lwkt_msg_t);
 
-typedef lwkt_port_t (*lwkt_portfn_t)(struct mbuf *);
+typedef lwkt_port_t (*lwkt_portfn_t)(struct mbuf **);
 
 struct netisr {
 	lwkt_port	ni_port;	/* must be first */
@@ -210,7 +210,7 @@
 
 extern lwkt_port netisr_afree_rport;
 
-lwkt_port_t	cpu0_portfn(struct mbuf *m);
+lwkt_port_t	cpu0_portfn(struct mbuf **mptr);
 void		netisr_dispatch(int, struct mbuf *);
 int		netisr_queue(int, struct mbuf *);
 void		netisr_register(int, lwkt_portfn_t, netisr_fn_t);
Index: netinet/ip_demux.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_demux.c,v
retrieving revision 1.24
diff -u -r1.24 ip_demux.c
--- netinet/ip_demux.c	8 Jul 2004 22:43:01 -0000	1.24
+++ netinet/ip_demux.c	18 Jul 2004 04:57:04 -0000
@@ -99,62 +99,122 @@
 }
 
 /*
- * Map a packet to a protocol processing thread.
+ * Map a packet to a protocol processing thread and return the thread's port.
+ * If an error occurs, the passed mbuf will be freed, *mptr will be set
+ * to NULL, and NULL will be returned.  If no error occurs, the passed mbuf
+ * may be modified and a port pointer will be returned.
  */
 lwkt_port_t
-ip_mport(struct mbuf *m)
+ip_mport(struct mbuf **mptr)
 {
 	struct ip *ip;
 	int iphlen;
 	struct tcphdr *th;
 	struct udphdr *uh;
+	struct mbuf *m = *mptr;
 	int thoff;				/* TCP data offset */
 	lwkt_port_t port;
 	int cpu;
 
+	/*
+	 * The packet must be at least the size of an IP header
+	 */
 	if (m->m_pkthdr.len < sizeof(struct ip)) {
 		ipstat.ips_tooshort++;
+		m_freem(m);
+		*mptr = NULL;
 		return (NULL);
 	}
 
+	/*
+	 * The first mbuf must entirely contain the IP header
+	 */
 	if (m->m_len < sizeof(struct ip) &&
 	    (m = m_pullup(m, sizeof(struct ip))) == NULL) {
 		ipstat.ips_toosmall++;
+		*mptr = NULL;
 		return (NULL);
 	}
-
 	ip = mtod(m, struct ip *);
 
+	/*
+	 * Extract the actual IP header length and do a bounds check.  The
+	 * first mbuf must entirely contain the extended IP header.
+	 */
 	iphlen = ip->ip_hl << 2;
 	if (iphlen < sizeof(struct ip)) {	/* minimum header length */
 		ipstat.ips_badhlen++;
+		m_freem(m);
 		return (NULL);
 	}
+	if (m->m_len < iphlen) {
+		m = m_pullup(m, iphlen);
+		if (m == NULL) {
+			ipstat.ips_badhlen++;
+			*mptr = NULL;
+			return (NULL);
+		}
+		ip = mtod(m, struct ip *);
+	}
+
+	/*
+	 * The TCP/IP or UDP/IP header must be entirely contained within
+	 * the first fragment of a packet.  Packet filters will break if they
+	 * aren't.
+	 */
+	if ((ntohs(ip->ip_off) & IP_OFFMASK) == 0) {
+		switch (ip->ip_p) {
+		case IPPROTO_TCP:
+			if (m->m_len < iphlen + sizeof(struct tcphdr)) {
+				m = m_pullup(m, iphlen + sizeof(struct tcphdr));
+				if (m == NULL) {
+					tcpstat.tcps_rcvshort++;
+					*mptr = NULL;
+					return (NULL);
+				}
+				ip = mtod(m, struct ip *);
+			}
+			break;
+		case IPPROTO_UDP:
+			if (m->m_len < iphlen + sizeof(struct udphdr)) {
+				m = m_pullup(m, iphlen + sizeof(struct udphdr));
+				if (m == NULL) {
+					udpstat.udps_hdrops++;
+					*mptr = NULL;
+					return (NULL);
+				}
+				ip = mtod(m, struct ip *);
+			}
+			break;
+		default:
+			break;
+		}
+	}
 
 	/*
 	 * XXX generic packet handling defrag on CPU 0 for now.
 	 */
-	if (ntohs(ip->ip_off) & (IP_MF | IP_OFFMASK))
+	if (ntohs(ip->ip_off) & (IP_MF | IP_OFFMASK)) {
+		*mptr = m;
 		return (&netisr_cpu[0].td_msgport);
+	}
 
 	switch (ip->ip_p) {
 	case IPPROTO_TCP:
-		if (m->m_len < iphlen + sizeof(struct tcphdr) &&
-		    (m = m_pullup(m, iphlen + sizeof(struct tcphdr))) == NULL) {
-			tcpstat.tcps_rcvshort++;
-			return (NULL);
-		}
 		th = (struct tcphdr *)((caddr_t)ip + iphlen);
 		thoff = th->th_off << 2;
 		if (thoff < sizeof(struct tcphdr) ||
 		    thoff > ntohs(ip->ip_len)) {
 			tcpstat.tcps_rcvbadoff++;
+			m_freem(m);
+			*mptr = NULL;
 			return (NULL);
 		}
 		if (m->m_len < iphlen + thoff) {
 			m = m_pullup(m, iphlen + thoff);
 			if (m == NULL) {
 				tcpstat.tcps_rcvshort++;
+				*mptr = NULL;
 				return (NULL);
 			}
 			ip = mtod(m, struct ip *);
@@ -166,14 +226,6 @@
 		port = &tcp_thread[cpu].td_msgport;
 		break;
 	case IPPROTO_UDP:
-		if (m->m_len < iphlen + sizeof(struct udphdr)) {
-			m = m_pullup(m, iphlen + sizeof(struct udphdr));
-			if (m == NULL) {
-				udpstat.udps_hdrops++;
-				return (NULL);
-			}
-			ip = mtod(m, struct ip *);
-		}
 		uh = (struct udphdr *)((caddr_t)ip + iphlen);
 
 		if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) ||
@@ -186,15 +238,11 @@
 		port = &udp_thread[cpu].td_msgport;
 		break;
 	default:
-		if (m->m_len < iphlen && (m = m_pullup(m, iphlen)) == NULL) {
-			ipstat.ips_badhlen++;
-			return (NULL);
-		}
 		port = &netisr_cpu[0].td_msgport;
 		break;
 	}
 	KKASSERT(port->mp_putport != NULL);
-
+	*mptr = m;
 	return (port);
 }
 
Index: netinet/ip_input.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_input.c,v
retrieving revision 1.33
diff -u -r1.33 ip_input.c
--- netinet/ip_input.c	8 Jul 2004 22:07:35 -0000	1.33
+++ netinet/ip_input.c	18 Jul 2004 04:56:45 -0000
@@ -1096,7 +1096,6 @@
 		lwkt_initmsg(&msg->nm_lmsg, &netisr_afree_rport, 0,
 			lwkt_cmd_func(transport_processing_handler),
 			lwkt_cmd_op_none);
-		msg->nm_mbuf = m;
 		msg->nm_hlen = hlen;
 		msg->nm_hasnexthop = (args.next_hop != NULL);
 		if (msg->nm_hasnexthop)
@@ -1104,13 +1103,14 @@
 
 		ip->ip_off = htons(ip->ip_off);
 		ip->ip_len = htons(ip->ip_len);
-		port = ip_mport(m);
-		if (port == NULL)
-			goto bad;
-		ip->ip_len = ntohs(ip->ip_len);
-		ip->ip_off = ntohs(ip->ip_off);
-
-		lwkt_sendmsg(port, &msg->nm_lmsg);
+		port = ip_mport(&m);
+		if (port) {
+		    msg->nm_mbuf = m;
+		    ip = mtod(m, struct ip *);
+		    ip->ip_len = ntohs(ip->ip_len);
+		    ip->ip_off = ntohs(ip->ip_off);
+		    lwkt_sendmsg(port, &msg->nm_lmsg);
+		}
 	} else {
 		transport_processing_oncpu(m, hlen, ip, args.next_hop);
 	}
Index: netinet/ip_var.h
===================================================================
RCS file: /cvs/src/sys/netinet/ip_var.h,v
retrieving revision 1.10
diff -u -r1.10 ip_var.h
--- netinet/ip_var.h	24 Jun 2004 08:15:17 -0000	1.10
+++ netinet/ip_var.h	18 Jul 2004 04:50:56 -0000
@@ -183,7 +183,7 @@
 extern int	 (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *,
 			  struct ip_moptions *);
 struct lwkt_port *
-	 ip_mport(struct mbuf *);
+	 ip_mport(struct mbuf **);
 int	 ip_output(struct mbuf *,
 	    struct mbuf *, struct route *, int, struct ip_moptions *,
 	    struct inpcb *);