Subject: netipsec m_makespace() overrun
To: None <tech-net@netbsd.org>
From: Sean Boudreau <seanb@qnx.com>
List: tech-net
Date: 12/14/2007 10:38:03
Hi:

It's pretty easy to tickle the
IPSEC_ASSERT(remain < MLEN, ("m_makespace: remainder too big: %u", remain));
in m_makespace().  If not running DIAGNOSTIC an memcpy()
past a buffer occurs.  The following is more generic and
handles this case.  Any comments before I commit?

BTW what happened to sys/arch/i386/conf/GENERIC.FAST_IPSEC
on the HEAD?  Is one supposed to use another method to
build FAST_IPSEC?

Thanks,

-seanb


Index: ipsec_mbuf.c
===================================================================
RCS file: /cvsroot/src/sys/netipsec/ipsec_mbuf.c,v
retrieving revision 1.9
diff -U 10 -r1.9 ipsec_mbuf.c
--- ipsec_mbuf.c	4 Mar 2007 19:54:48 -0000	1.9
+++ ipsec_mbuf.c	14 Dec 2007 15:12:49 -0000
@@ -231,81 +231,82 @@
 	/*
 	 * At this point skip is the offset into the mbuf m
 	 * where the new header should be placed.  Figure out
 	 * if there's space to insert the new header.  If so,
 	 * and copying the remainder makese sense then do so.
 	 * Otherwise insert a new mbuf in the chain, splitting
 	 * the contents of m as needed.
 	 */
 	remain = m->m_len - skip;		/* data to move */
 	if (hlen > M_TRAILINGSPACE(m)) {
-		struct mbuf *n;
+		struct mbuf *n0, *n, **np;
+		int todo, len, done, alloc;
+
+		n0 = NULL;
+		np = &n0;
+		alloc = 0;
+		done = 0;
+		todo = remain;
+		while (todo > 0) {
+			if (todo > MHLEN) {
+				n = m_getcl(M_DONTWAIT, m->m_type, 0);
+				len = MCLBYTES;
+			}
+			else {
+				n = m_get(M_DONTWAIT, m->m_type);
+				len = MHLEN;
+			}
+			if (n == NULL) {
+				m_freem(n0);
+				return NULL;
+			}
+			*np = n;
+			np = &n->m_next;
+			alloc++;
+			len = min(todo, len);
+			memcpy(n->m_data, mtod(m, char *) + skip + done, len);
+			n->m_len = len;
+			done += len;
+			todo -= len;
+		}
 
-		/* XXX code doesn't handle clusters XXX */
-		IPSEC_ASSERT(remain < MLEN,
-			("m_makespace: remainder too big: %u", remain));
-		/*
-		 * Not enough space in m, split the contents
-		 * of m, inserting new mbufs as required.
-		 *
-		 * NB: this ignores mbuf types.
-		 */
-		MGET(n, M_DONTWAIT, MT_DATA);
-		if (n == NULL)
-			return (NULL);
-		n->m_next = m->m_next;		/* splice new mbuf */
-		m->m_next = n;
-		newipsecstat.ips_mbinserted++;
 		if (hlen <= M_TRAILINGSPACE(m) + remain) {
-			/*
-			 * New header fits in the old mbuf if we copy
-			 * the remainder; just do the copy to the new
-			 * mbuf and we're good to go.
-			 */
-			memcpy(mtod(n, char *),
-			       mtod(m, char *) + skip, remain);
-			n->m_len = remain;
 			m->m_len = skip + hlen;
 			*off = skip;
-		} else {
-			/*
-			 * No space in the old mbuf for the new header.
-			 * Make space in the new mbuf and check the
-			 * remainder'd data fits too.  If not then we
-			 * must allocate an additional mbuf (yech).
-			 */
-			n->m_len = 0;
-			if (remain + hlen > M_TRAILINGSPACE(n)) {
-				struct mbuf *n2;
-
-				MGET(n2, M_DONTWAIT, MT_DATA);
-				/* NB: new mbuf is on chain, let caller free */
-				if (n2 == NULL)
-					return (NULL);
-				n2->m_len = 0;
-				memcpy(mtod(n2, char *),
-				       mtod(m, char *) + skip, remain);
-				n2->m_len = remain;
-				/* splice in second mbuf */
-				n2->m_next = n->m_next;
-				n->m_next = n2;
-				newipsecstat.ips_mbinserted++;
-			} else {
-				memcpy(mtod(n, char *) + hlen,
-				       mtod(m, char *) + skip, remain);
-				n->m_len += remain;
+			if (n0 != NULL) {
+				*np = m->m_next;
+				m->m_next = n0;
 			}
-			m->m_len -= remain;
-			n->m_len += hlen;
+		}
+		else {
+			n = m_get(M_DONTWAIT, m->m_type);
+			if (n == NULL) {
+				m_freem(n0);
+				return NULL;
+			}
+			alloc++;
+
+			if ((n->m_next = n0) == NULL)
+				np = &n->m_next;
+			n0 = n;
+
+			*np = m->m_next;
+			m->m_next = n0;
+
+			n->m_len = hlen;
+			m->m_len = skip;
+
 			m = n;			/* header is at front ... */
 			*off = 0;		/* ... of new mbuf */
 		}
+
+		newipsecstat.ips_mbinserted += alloc;
 	} else {
 		/*
 		 * Copy the remainder to the back of the mbuf
 		 * so there's space to write the new header.
 		 */
 		/* XXX can this be memcpy? does it handle overlap? */
 		ovbcopy(mtod(m, char *) + skip,
 			mtod(m, char *) + skip + hlen, remain);
 		m->m_len += hlen;
 		*off = skip;