Re: fix for IPSEC-IPV4 breakage

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

: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@xxxxxxxxxxxxx>