[Qemu-devel] [PATCH] Fix scrambling of >32KB packets in slirp

* [Qemu-devel] [PATCH] Fix scrambling of >32KB packets in slirp
@ 2006-05-01  2:55 Ed Swierk
  2006-05-01 11:08 ` Fabrice Bellard
  0 siblings, 1 reply; 5+ messages in thread
From: Ed Swierk @ 2006-05-01  2:55 UTC (permalink / raw
  To: qemu-devel

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

In several places in qemu's slirp code, signed and unsigned ints are
used interchangeably when dealing with IP packet lengths and offsets.
This causes IP packets greater than 32K in length to be scrambled in
various interesting ways that are extremely difficult to troubleshoot.

Although large IP packets are fairly rare in practice, certain
UDP-based protocols like NFS use them extensively.

The attached patch wraps IP packet lengths and offsets in macros that
ensure they are always properly treated as unsigned values.

--Ed

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: qemu-slirp-32k-packets.patch --]
[-- Type: text/x-patch; name="qemu-slirp-32k-packets.patch", Size: 4430 bytes --]

diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/ip.h qemu-snapshot-2006-03-27_23/slirp/ip.h

--- qemu-snapshot-2006-03-27_23.orig/slirp/ip.h	2004-04-21 17:10:47.000000000 -0700
+++ qemu-snapshot-2006-03-27_23/slirp/ip.h	2006-04-06 00:28:49.000000000 -0700
@@ -79,6 +79,11 @@
  * We declare ip_len and ip_off to be short, rather than u_short
  * pragmatically since otherwise unsigned comparisons can result
  * against negative integers quite easily, and fail in subtle ways.
+ *
+ * The only problem with the above theory is that these quantities
+ * are in fact unsigned, and sorting fragments by a signed version
+ * of ip_off doesn't work very well, nor does length checks on
+ * ip packets with a signed version of their length!
  */
 struct ip {
 #ifdef WORDS_BIGENDIAN
@@ -101,6 +106,9 @@
 	struct	in_addr ip_src,ip_dst;	/* source and dest address */
 };
 
+#define IP_OFF(ip) (*(u_int16_t *)&((ip)->ip_off))
+#define IP_LEN(ip) (*(u_int16_t *)&((ip)->ip_len))
+
 #define	IP_MAXPACKET	65535		/* maximum packet size */
 
 /*
diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c qemu-snapshot-2006-03-27_23/slirp/ip_input.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/ip_input.c	2004-04-21 17:10:47.000000000 -0700
+++ qemu-snapshot-2006-03-27_23/slirp/ip_input.c	2006-04-06 00:32:19.000000000 -0700
@@ -111,7 +111,7 @@
 	 * Convert fields to host representation.
 	 */
 	NTOHS(ip->ip_len);
-	if (ip->ip_len < hlen) {
+	if (IP_LEN(ip) < hlen) {
 		ipstat.ips_badlen++;
 		goto bad;
 	}
@@ -124,13 +124,13 @@
 	 * Trim mbufs if longer than we expect.
 	 * Drop packet if shorter than we expect.
 	 */
-	if (m->m_len < ip->ip_len) {
+	if (m->m_len < IP_LEN(ip)) {
 		ipstat.ips_tooshort++;
 		goto bad;
 	}
 	/* Should drop packet if mbuf too long? hmmm... */
-	if (m->m_len > ip->ip_len)
-	   m_adj(m, ip->ip_len - m->m_len);
+	if (m->m_len > IP_LEN(ip))
+	   m_adj(m, IP_LEN(ip) - m->m_len);
 
 	/* check ip_ttl for a correct ICMP reply */
 	if(ip->ip_ttl==0 || ip->ip_ttl==1) {
@@ -191,7 +191,7 @@
 		 * or if this is not the first fragment,
 		 * attempt reassembly; if it succeeds, proceed.
 		 */
-		if (((struct ipasfrag *)ip)->ipf_mff & 1 || ip->ip_off) {
+		if (((struct ipasfrag *)ip)->ipf_mff & 1 || IP_OFF(ip)) {
 			ipstat.ips_fragments++;
 			ip = ip_reass((struct ipasfrag *)ip, fp);
 			if (ip == 0)
@@ -281,7 +281,7 @@
 	 */
 	for (q = (struct ipasfrag *)fp->ipq_next; q != (struct ipasfrag *)fp;
 	    q = (struct ipasfrag *)q->ipf_next)
-		if (q->ip_off > ip->ip_off)
+		if (IP_OFF(q) > IP_OFF(ip))
 			break;
 
 	/*
@@ -290,10 +290,10 @@
 	 * segment.  If it provides all of our data, drop us.
 	 */
 	if (q->ipf_prev != (ipasfragp_32)fp) {
-		i = ((struct ipasfrag *)(q->ipf_prev))->ip_off +
-		  ((struct ipasfrag *)(q->ipf_prev))->ip_len - ip->ip_off;
+		i = IP_OFF((struct ipasfrag *)(q->ipf_prev)) +
+		  IP_LEN((struct ipasfrag *)(q->ipf_prev)) - IP_OFF(ip);
 		if (i > 0) {
-			if (i >= ip->ip_len)
+			if (i >= IP_LEN(ip))
 				goto dropfrag;
 			m_adj(dtom(ip), i);
 			ip->ip_off += i;
@@ -305,9 +305,9 @@
 	 * While we overlap succeeding segments trim them or,
 	 * if they are completely covered, dequeue them.
 	 */
-	while (q != (struct ipasfrag *)fp && ip->ip_off + ip->ip_len > q->ip_off) {
-		i = (ip->ip_off + ip->ip_len) - q->ip_off;
-		if (i < q->ip_len) {
+	while (q != (struct ipasfrag *)fp && IP_OFF(ip) + IP_LEN(ip) > IP_OFF(q)) {
+		i = (IP_OFF(ip) + IP_LEN(ip)) - IP_OFF(q);
+		if (i < IP_LEN(q)) {
 			q->ip_len -= i;
 			q->ip_off += i;
 			m_adj(dtom(q), i);
@@ -327,9 +327,9 @@
 	next = 0;
 	for (q = (struct ipasfrag *) fp->ipq_next; q != (struct ipasfrag *)fp;
 	     q = (struct ipasfrag *) q->ipf_next) {
-		if (q->ip_off != next)
+		if (IP_OFF(q) != next)
 			return (0);
-		next += q->ip_len;
+		next += IP_LEN(q);
 	}
 	if (((struct ipasfrag *)(q->ipf_prev))->ipf_mff & 1)
 		return (0);
diff -burN qemu-snapshot-2006-03-27_23.orig/slirp/udp.c qemu-snapshot-2006-03-27_23/slirp/udp.c
--- qemu-snapshot-2006-03-27_23.orig/slirp/udp.c	2006-04-06 00:24:30.000000000 -0700
+++ qemu-snapshot-2006-03-27_23/slirp/udp.c	2006-04-06 00:32:55.000000000 -0700
@@ -111,12 +111,12 @@
 	 */
 	len = ntohs((u_int16_t)uh->uh_ulen);
 
-	if (ip->ip_len != len) {
-		if (len > ip->ip_len) {
+	if (IP_LEN(ip) != len) {
+		if (len > IP_LEN(ip)) {
 			udpstat.udps_badlen++;
 			goto bad;
 		}
-		m_adj(m, len - ip->ip_len);
+		m_adj(m, len - IP_LEN(ip));
 		ip->ip_len = len;
 	}
 	

^ permalink raw reply	[flat|nested] 5+ messages in thread