Fixed broken ICMP error generation, unified conversion of IP header

fields between host and network byte order.  The details:

o icmp_error() now does not add IP header length.  This fixes the problem
  when icmp_error() is called from ip_forward().  In this case the ip_len
  of the original IP datagram returned with ICMP error was wrong.

o icmp_error() expects all three fields, ip_len, ip_id and ip_off in host
  byte order, so DTRT and convert these fields back to network byte order
  before sending a message.  This fixes the problem described in PR 16240
  and PR 20877 (ip_id field was returned in host byte order).

o ip_ttl decrement operation in ip_forward() was moved down to make sure
  that it does not corrupt the copy of original IP datagram passed later
  to icmp_error().

o A copy of original IP datagram in ip_forward() was made a read-write,
  independent copy.  This fixes the problem I first reported to Garrett
  Wollman and Bill Fenner and later put in audit trail of PR 16240:
  ip_output() (not always) converts fields of original datagram to network
  byte order, but because copy (mcopy) and its original (m) most likely
  share the same mbuf cluster, ip_output()'s manipulations on original
  also corrupted the copy.

o ip_output() now expects all three fields, ip_len, ip_off and (what is
  significant) ip_id in host byte order.  It was a headache for years that
  ip_id was handled differently.  The only compatibility issue here is the
  raw IP socket interface with IP_HDRINCL socket option set and a non-zero
  ip_id field, but ip.4 manual page was unclear on whether in this case
  ip_id field should be in host or network byte order.

7 files changed, 47 insertions, 26 deletions

diff --git a/sys/netinet/ip_divert.c b/sys/netinet/ip_divert.c
index b2f92b8..12acdfd 100644
--- a/sys/netinet/ip_divert.c
+++ b/sys/netinet/ip_divert.c
@@ -293,6 +293,7 @@ div_output(so, m, addr, control)
 
 		/* Convert fields to host order for ip_output() */
 		NTOHS(ip->ip_len);
+		NTOHS(ip->ip_id);
 		NTOHS(ip->ip_off);
 
 		/* Send packet to output processing */
diff --git a/sys/netinet/ip_icmp.c b/sys/netinet/ip_icmp.c
index 58a4915..91c0bc0 100644
--- a/sys/netinet/ip_icmp.c
+++ b/sys/netinet/ip_icmp.c
@@ -191,7 +191,13 @@ icmp_error(n, type, code, dest, destifp)
 	icp->icmp_code = code;
 	bcopy((caddr_t)oip, (caddr_t)&icp->icmp_ip, icmplen);
 	nip = &icp->icmp_ip;
-	nip->ip_len = htons((u_short)(nip->ip_len + oiplen));
+
+	/*
+	 * Convert fields to network representation.
+	 */
+	HTONS(nip->ip_len);
+	HTONS(nip->ip_id);
+	HTONS(nip->ip_off);
 
 	/*
 	 * Now, copy old ip header (without options)
diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c
index bfd9c45..5ba5750 100644
--- a/sys/netinet/ip_input.c
+++ b/sys/netinet/ip_input.c
@@ -526,18 +526,12 @@ pass:
 			 * The packet is returned (relatively) intact; if
 			 * ip_mforward() returns a non-zero value, the packet
 			 * must be discarded, else it may be accepted below.
-			 *
-			 * (The IP ident field is put in the same byte order
-			 * as expected when ip_mforward() is called from
-			 * ip_output().)
 			 */
-			ip->ip_id = htons(ip->ip_id);
 			if (ip_mforward(ip, m->m_pkthdr.rcvif, m, 0) != 0) {
 				ipstat.ips_cantforward++;
 				m_freem(m);
 				return;
 			}
-			ip->ip_id = ntohs(ip->ip_id);
 
 			/*
 			 * The process-level routing demon needs to receive
@@ -1290,7 +1284,6 @@ nosourcerouting:
 	}
 	return (0);
 bad:
-	ip->ip_len -= IP_VHL_HL(ip->ip_vhl) << 2;   /* XXX icmp_error adds in hdr length */
 	icmp_error(m, type, code, 0, 0);
 	ipstat.ips_badoptions++;
 	return (1);
@@ -1496,7 +1489,6 @@ ip_forward(m, srcrt)
 		m_freem(m);
 		return;
 	}
-	HTONS(ip->ip_id);
 #ifdef IPSTEALTH
 	if (!ipstealth) {
 #endif
@@ -1505,7 +1497,6 @@ ip_forward(m, srcrt)
 			    dest, 0);
 			return;
 		}
-		ip->ip_ttl -= IPTTLDEC;
 #ifdef IPSTEALTH
 	}
 #endif
@@ -1534,6 +1525,16 @@ ip_forward(m, srcrt)
 	 * we need to generate an ICMP message to the src.
 	 */
 	mcopy = m_copy(m, 0, imin((int)ip->ip_len, 64));
+	if (mcopy && (mcopy->m_flags & M_EXT))
+		m_copydata(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
+
+#ifdef IPSTEALTH
+	if (!ipstealth) {
+#endif
+		ip->ip_ttl -= IPTTLDEC;
+#ifdef IPSTEALTH
+	}
+#endif
 
 	/*
 	 * If forwarding packet using same interface that it came in on,
@@ -1672,6 +1673,8 @@ ip_forward(m, srcrt)
 		m_freem(mcopy);
 		return;
 	}
+	if (mcopy->m_flags & M_EXT)
+		m_copyback(mcopy, 0, sizeof(struct ip), mtod(mcopy, caddr_t));
 	icmp_error(mcopy, type, code, dest, destifp);
 }
 
diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c
index 1d165f5..73ab039 100644
--- a/sys/netinet/ip_mroute.c
+++ b/sys/netinet/ip_mroute.c
@@ -1581,7 +1581,7 @@ encap_send(ip, vifp, m)
      */
     ip_copy = mtod(mb_copy, struct ip *);
     *ip_copy = multicast_encap_iphdr;
-    ip_copy->ip_id = htons(ip_id++);
+    ip_copy->ip_id = ip_id++;
     ip_copy->ip_len += len;
     ip_copy->ip_src = vifp->v_lcl_addr;
     ip_copy->ip_dst = vifp->v_rmt_addr;
@@ -1592,6 +1592,7 @@ encap_send(ip, vifp, m)
     ip = (struct ip *)((caddr_t)ip_copy + sizeof(multicast_encap_iphdr));
     --ip->ip_ttl;
     HTONS(ip->ip_len);
+    HTONS(ip->ip_id);
     HTONS(ip->ip_off);
     ip->ip_sum = 0;
     mb_copy->m_data += sizeof(multicast_encap_iphdr);
diff --git a/sys/netinet/ip_output.c b/sys/netinet/ip_output.c
index a5dd111..9fe7001 100644
--- a/sys/netinet/ip_output.c
+++ b/sys/netinet/ip_output.c
@@ -211,7 +211,7 @@ ip_output(m0, opt, ro, flags, imo)
 	if ((flags & (IP_FORWARDING|IP_RAWOUTPUT)) == 0) {
 		ip->ip_vhl = IP_MAKE_VHL(IPVERSION, hlen >> 2);
 		ip->ip_off &= IP_DF;
-		ip->ip_id = htons(ip_id++);
+		ip->ip_id = ip_id++;
 		ipstat.ips_localout++;
 	} else {
 		hlen = IP_VHL_HL(ip->ip_vhl) << 2;
@@ -520,6 +520,7 @@ sendit:
 
 			/* Restore packet header fields to original values */
 			HTONS(ip->ip_len);
+			HTONS(ip->ip_id);
 			HTONS(ip->ip_off);
 
 			/* Deliver packet to divert input routine */
@@ -593,8 +594,9 @@ sendit:
 				}
 				m->m_pkthdr.csum_flags |=
 				    CSUM_IP_CHECKED | CSUM_IP_VALID;
-				ip->ip_len = htons((u_short)ip->ip_len);
-				ip->ip_off = htons((u_short)ip->ip_off);
+				HTONS(ip->ip_len);
+				HTONS(ip->ip_id);
+				HTONS(ip->ip_off);
 				ip_input(m);
 				goto done;
 			}
@@ -712,8 +714,9 @@ pass:
 		m->m_pkthdr.csum_flags &= ~CSUM_DELAY_DATA;
 	}
 
-	ip->ip_len = htons((u_short)ip->ip_len);
-	ip->ip_off = htons((u_short)ip->ip_off);
+	HTONS(ip->ip_len);
+	HTONS(ip->ip_id);
+	HTONS(ip->ip_off);
 
 	error = ipsec4_output(&state, sp, flags);
 
@@ -772,8 +775,9 @@ pass:
 	}
 
 	/* make it flipped, again. */
-	ip->ip_len = ntohs((u_short)ip->ip_len);
-	ip->ip_off = ntohs((u_short)ip->ip_off);
+	NTOHS(ip->ip_len);
+	NTOHS(ip->ip_id);
+	NTOHS(ip->ip_off);
 skip_ipsec:
 #endif /*IPSEC*/
 
@@ -791,8 +795,9 @@ skip_ipsec:
 	 */
 	if ((u_short)ip->ip_len <= ifp->if_mtu ||
 	    ifp->if_hwassist & CSUM_FRAGMENT) {
-		ip->ip_len = htons((u_short)ip->ip_len);
-		ip->ip_off = htons((u_short)ip->ip_off);
+		HTONS(ip->ip_len);
+		HTONS(ip->ip_id);
+		HTONS(ip->ip_off);
 		ip->ip_sum = 0;
 		if (sw_csum & CSUM_DELAY_IP) {
 			if (ip->ip_vhl == IP_VHL_BORING) {
@@ -887,7 +892,8 @@ skip_ipsec:
 		m->m_pkthdr.len = mhlen + len;
 		m->m_pkthdr.rcvif = (struct ifnet *)0;
 		m->m_pkthdr.csum_flags = m0->m_pkthdr.csum_flags;
-		mhip->ip_off = htons((u_short)mhip->ip_off);
+		HTONS(mhip->ip_id);
+		HTONS(mhip->ip_off);
 		mhip->ip_sum = 0;
 		if (sw_csum & CSUM_DELAY_IP) {
 			if (mhip->ip_vhl == IP_VHL_BORING) {
@@ -915,7 +921,9 @@ skip_ipsec:
 	m_adj(m, hlen + firstlen - (u_short)ip->ip_len);
 	m->m_pkthdr.len = hlen + firstlen;
 	ip->ip_len = htons((u_short)m->m_pkthdr.len);
-	ip->ip_off = htons((u_short)(ip->ip_off | IP_MF));
+	HTONS(ip->ip_id);
+	ip->ip_off |= IP_MF;
+	HTONS(ip->ip_off);
 	ip->ip_sum = 0;
 	if (sw_csum & CSUM_DELAY_IP) {
 		if (ip->ip_vhl == IP_VHL_BORING) {
@@ -1855,8 +1863,9 @@ ip_mloopback(ifp, m, dst, hlen)
 		 * than the interface's MTU.  Can this possibly matter?
 		 */
 		ip = mtod(copym, struct ip *);
-		ip->ip_len = htons((u_short)ip->ip_len);
-		ip->ip_off = htons((u_short)ip->ip_off);
+		HTONS(ip->ip_len);
+		HTONS(ip->ip_id);
+		HTONS(ip->ip_off);
 		ip->ip_sum = 0;
 		if (ip->ip_vhl == IP_VHL_BORING) {
 			ip->ip_sum = in_cksum_hdr(ip);
diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
index 5d056ce..94046e8 100644
--- a/sys/netinet/raw_ip.c
+++ b/sys/netinet/raw_ip.c
@@ -221,7 +221,7 @@ rip_output(m, so, dst)
 			return EINVAL;
 		}
 		if (ip->ip_id == 0)
-			ip->ip_id = htons(ip_id++);
+			ip->ip_id = ip_id++;
 		/* XXX prevent ip_output from overwriting header fields */
 		flags |= IP_RAWOUTPUT;
 		ipstat.ips_rawout++;
diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
index 09b5c96..07b1166 100644
--- a/sys/netinet/udp_usrreq.c
+++ b/sys/netinet/udp_usrreq.c
@@ -353,11 +353,12 @@ udp_input(m, off, proto)
 			udpstat.udps_noportbcast++;
 			goto bad;
 		}
-		*ip = save_ip;
 		if (badport_bandlim(0) < 0)
 			goto bad;
 		if (blackhole)
 			goto bad;
+		*ip = save_ip;
+		ip->ip_len += iphlen;
 		icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_PORT, 0, 0);
 		return;
 	}