fix for IPSEC-IPV4 breakage
Matthew Dillon
dillon at apollo.backplane.com
Wed Oct 13 21:48:32 PDT 2004
:Hmm. cbc_decrypt() takes mbuf *m as an argument. Does m_pullup() guarantee
:that the root pointer isn't modified, otherwise I'll need to call it higher
:up in the chain. Maybe in esp4_input.
No, it can return a different mbuf.
:Actually I tried using m_pullup in both spots with different results. First
:I tried m_pullup as a straight replacement for my change in cbc_decrypt.
:This resulted in panics in mfreem called from esp4_input.
:The code I added looked like -
:..
: if ( d0 )
: scut = m_pullup( scut, scutoff + d0->m_len );
:..
Right, because you can't just ripout 'scut' like that, it's still
attached to the original mbuf chain starting at 'm'.
:Next I tried calling m_pullup in esp4_input, immediately following the call
:to cbc_decrypt. This didn't trap, but didn't seem to work either, I added
:a printf and could see the m_pullup being called, and the results from the
:pullup looked to have worked as the m_len field looked to have expanded.
:But those packets were just getting dropped. Not sure why yet.
:...
:
: /* pullup if the decryptor has modified the mbuf chain */
: if ( m->m_len < save_len && m->m_next ) {
: m = m_pullup(m, m->m_len + m->m_next->m_len);
: printf("doing a pullup %p %d %d\n", m, m->m_len, save_len);
: }
:
:This code is pretty new to me (I had no clue what m_pullup was for until
:you mentioned it, so I wouldn't be surprised if I've messed up the logic
:somewhere :P
:
:Andrew.
This looks better, but I don't think your assumption that save_len will
cover the TCP or UDP header is correct and I don't think you can safely
m_pullup() the entire mbuf length plus the following mbuf because that
might exceed the mbuf cluster size. Putting the m_pullup there is
not going to work, so rip out your m_pullup() code :-)
If you hop down to approximately line 357 in esp_input.c, around this
line:
/* was it transmitted over the IPsec tunnel SA? */
We have an interesting case here. The first part of the conditional
looks correct... it pulls up the IP header and then queues the packet
with netisr_queue(). The else section, start at:
* strip off ESP header and IV.
Does not look correct. The question is, which code path are your tests
using? We would need a DDB backtrace of the panic to see.
If it is the second code path then we probably have some work to do.
We obviously cannot just call the protocol routine directly. We might
be able to get away with requeueing the packet the same wawy the first
section does, via netisr_queue(NETISR_IP, m).
Jeffrey Hsu would know for sure.
You could try a quick hack... replace this code:
if (nxt != IPPROTO_DONE) {
if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0
&&
ipsec4_in_reject(m, NULL)) {
ipsecstat.in_polvio++;
goto bad;
}
(*inetsw[ip_protox[nxt]].pr_input)(m, off, nxt);
} else ...
With:
if (nxt != IPPROTO_DONE) {
if ((inetsw[ip_protox[nxt]].pr_flags & PR_LASTHDR) != 0
&&
ipsec4_in_reject(m, NULL)) {
ipsecstat.in_polvio++;
goto bad;
}
if (netisr_queue(NETISR_IP, m)) {
ipsecstat.in_inval++;
m = NULL;
goto bad;
}
} else ...
I'm not sure if this is correct, but it's worth a shot. I'm almost
positive that the direct protocol dispatch is illegal.
-Matt
Matthew Dillon
<dillon at xxxxxxxxxxxxx>
More information about the Submit
mailing list