netipsec m_makespace() overrun

[Sean Boudreau]

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;


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Arnaud Degroote]

Post by Sean Boudreau
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?

The patch seems ok. Maybe we can be a bit smarter in case where
hlen > M_TRAILINGSPACE(m) + remain. As we already need to allocate at
least one mbuf for remain, we may try to preserve some space for hlen if
we can't put in m after that. It can save one mbuf allocation in some cases. Not
sure it is really important in fact.

It would be nice if the patch can be pulled-up in NetBSD-4 (or must we
wait for 4.1 ?).

Post by Sean Boudreau
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;

[Sean Boudreau]

Post by Sean Boudreau

Post by Sean Boudreau
It's pretty easy to tickle the
IPSEC_ASSERT(remain < MLEN, ("m_makespace: remainder too big: %u",

remain));

Post by Sean Boudreau
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?

The patch seems ok. Maybe we can be a bit smarter in case where
hlen > M_TRAILINGSPACE(m) + remain. As we already need to allocate at
least one mbuf for remain, we may try to preserve some space for hlen if
we can't put in m after that. It can save one mbuf allocation in some cases. Not
sure it is really important in fact.
It would be nice if the patch can be pulled-up in NetBSD-4 (or must we
wait for 4.1 ?).

Checked in. I sent the pullup request off. We'll see...

BTW looks like this is PR 30124.

-seanb

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de