ipsec4_splithdr invariant not right

[Greg Troxel]

I have a sparc64 running netbsd-4 that does tunnel-mode IPsec (v4 in
v4), and it's been hitting the 'mbuf too short' check in
ipsec4_splithdr. I added debugging code and found that the first mbuf
had zero bytes. I then added a conditional pullup, and that's been hit
with the machine surviving.

ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20

I don't see why it's reasonable for ipsec4_splithdr to assume that
struct ip fits in the first mbuf.



Here's my working diff:

Index: sys/netinet6/ipsec.c
===================================================================
RCS file: /cvsroot/src/sys/netinet6/ipsec.c,v
retrieving revision 1.110.2.1
diff -u -p -r1.110.2.1 ipsec.c
--- sys/netinet6/ipsec.c 20 Dec 2006 23:03:08 -0000 1.110.2.1
+++ sys/netinet6/ipsec.c 19 Apr 2007 16:28:46 -0000
@@ -3219,8 +3219,26 @@ ipsec4_splithdr(m)
struct ip *ip;
int hlen;

- if (m->m_len < sizeof(struct ip))
- panic("ipsec4_splithdr: first mbuf too short");
+ /*
+ * Previously the code checked for m_len too small and paniced.
+ */
+ if (m->m_len < sizeof(struct ip)) {
+ /* Rare, but happens with tunnel mode IPsec on sparc64 (at least). */
+ printf("ipsec4_splithdr: m->m_len %d m_length %d < %d\n",
+ m->m_len, m_length(m), (unsigned int) sizeof(struct ip));
+ m = m_pullup(m, sizeof(struct ip));
+ if (!m) {
+ printf("ipsec4_splithdr: pullup failed\n");
+ return NULL;
+ }
+ }
+ /* Recheck, and just complain/drop rather than panic. */
+ if (m->m_len < sizeof(struct ip)) {
+ printf("ipsec4_splithdr: STILL m_len %d m_length %d\n",
+ m->m_len, m_length(m));
+ m_freem(m);
+ return NULL;
+ }
ip = mtod(m, struct ip *);
hlen = ip->ip_hl << 2;
if (m->m_len > hlen) {

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

[Christos Zoulas]

Post by Greg Troxel
I have a sparc64 running netbsd-4 that does tunnel-mode IPsec (v4 in
v4), and it's been hitting the 'mbuf too short' check in
ipsec4_splithdr. I added debugging code and found that the first mbuf
had zero bytes. I then added a conditional pullup, and that's been hit
with the machine surviving.
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
I don't see why it's reasonable for ipsec4_splithdr to assume that
struct ip fits in the first mbuf.

Is the second if statement even possible to fire?

christos


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

[Greg Troxel]

Post by Christos Zoulas

Post by Greg Troxel
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
ipsec4_splithdr: m->m_len 0 m_length 176 < 20
I don't see why it's reasonable for ipsec4_splithdr to assume that
struct ip fits in the first mbuf.

Is the second if statement even possible to fire?

If you mean rechecking the results of pullup after we confirmed we
didn't get null back, then no, I don't think it's possible. Were I to
commit this I'd drop that, and drop the printfs.


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

[Christos Zoulas]

On Apr 20, 9:12am, ***@ir.bbn.com (Greg Troxel) wrote:
-- Subject: Re: ipsec4_splithdr invariant not right

| ***@astron.com (Christos Zoulas) writes:
|
| > In article <***@fnord.ir.bbn.com>,
| > Greg Troxel <***@ir.bbn.com> wrote:
| >
| >>ipsec4_splithdr: m->m_len 0 m_length 176 < 20
| >>ipsec4_splithdr: m->m_len 0 m_length 176 < 20
| >>ipsec4_splithdr: m->m_len 0 m_length 176 < 20
| >>
| >>I don't see why it's reasonable for ipsec4_splithdr to assume that
| >>struct ip fits in the first mbuf.
| >
| > Is the second if statement even possible to fire?
|
| If you mean rechecking the results of pullup after we confirmed we
| didn't get null back, then no, I don't think it's possible. Were I to
| commit this I'd drop that, and drop the printfs.

I'd say do that. Although I don't understand why I never see it on my
x86 box.

christos


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