On Monday, January 31, 2011 9:40:09 pm Lawrence Stewart wrote:
> On 02/01/11 04:17, John Baldwin wrote:
> > Somewhat related fallout to the bug reported on security@ recently, I think 
> > this KASSERT() in tcp_output() is bogus:
> > 
> > 
> >     KASSERT(len + hdrlen + ipoptlen == m_length(m, NULL),
> >         ("%s: mbuf chain shorter than expected", __func__));
> > 
> > Specifically, just a few lines earlier in tcp_output() we set the packet 
> > header length to just 'len + hdrlen':
> > 
> >     /*
> >      * Put TCP length in extended header, and then
> >      * checksum extended header and data.
> >      */
> >     m->m_pkthdr.len = hdrlen + len; /* in6_cksum() need this */
> > 
> > Also, the ipoptions are stored in a separate mbuf chain in the in pcb 
> > (inp_options) that is passed as a separate argument to ip_output().  Given 
> > that, I would think that m_length() should not reflect ipoptlen since it 
> > should not include IP options in that chain?
> > 
> 
> There is some relevant prior discussion on src-committers@ for r212803
> between Andre and Bjoern.

I still don't see where ipoptlen bytes are reserved in the mbuf chain.  After
this block where 'm' is allocated and initialized:

        /*
         * Grab a header mbuf, attaching a copy of data to
         * be transmitted, and initialize the header from
         * the template for sends on this connection.
         */
        if (len) {
                ...
                m->m_len = hdrlen;
                ...
                if (len <= MHLEN - hdrlen - max_linkhdr) {
                        ...
                        m->m_len += len;
                } else {
                        m->m_next = m_copy(mb, moff, (int)len);
                        ...
                }
                ...
        } else {
                ...
                m->m_len = hdrlen;
        }

The length of the mbuf chain headed by 'm' is clearly hdrlen + len.

At no point anywhere do we do any sort of m_prepend() or other operation to
allocate space in the mbuf chain for the IP options.  They are merged in in
ip_output().  I think the only reason this KASSERT() isn't firing in HEAD is
that IP options are rarely used?

Is there an easy way to test a connection with IP options enabled with this
KASSERT() enabled?

-- 
John Baldwin
_______________________________________________
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"

Reply via email to