Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 25 Oct 1998 23:15:00 -0800
From:      Don Lewis <Don.Lewis@tsc.tdk.com>
To:        Doug Rabson <dfr@nlsystems.com>, Don Lewis <Don.Lewis@tsc.tdk.com>
Cc:        Kris Kennaway <kkennawa@physics.adelaide.edu.au>, wollman@khavrinen.lcs.mit.edu, current@FreeBSD.ORG
Subject:   Re: nestea v2 against freebsd 3.0-Release (fwd)
Message-ID:  <199810260715.XAA26243@salsa.gv.tsc.tdk.com>
In-Reply-To: Doug Rabson <dfr@nlsystems.com> "Re: nestea v2 against freebsd 3.0-Release (fwd)" (Oct 25, 12:16pm)

next in thread | previous in thread | raw e-mail | index | archive | help
On Oct 25, 12:16pm, Doug Rabson wrote:
} Subject: Re: nestea v2 against freebsd 3.0-Release (fwd)
} On Sun, 25 Oct 1998, Don Lewis wrote:
} 
} > Ok, I figured out what's going on.  When I compiled the nestea2.c under
} > FreeBSD, it didn't run at all because rip_output() does some sanity
} > checking between ip_len in the packet and the actual packet length, so
} > it doesn't send the third fragment and causes sendto() to return
} > EINVAL.  The Linux emulation code in the kernel is kind enough to fix
} > ip_len, so the sanity check passes.  Even after I fixed this in
} > nestea2.c, running it still didn't cause the system to panic.  The
} > reason for this is some differences in byte swapping in the IP header
} > fields between Linux and FreeBSD that nestea2.c attempted to compensate
} > for, but didn't get right.  Once I fixed the byte swapping problem, I
} > got the same panic you did, except for the linux emulation which I was
} > not using.
} > 
} > The panic is caused by a bug in the new ip fragment reassembly code
} > that can be provoked into playing with an mbuf after it has been freed.
} > Here's a patch.
} > 
} > --- ip_input.c.orig	Fri Oct 23 02:17:19 1998
} > +++ ip_input.c	Sun Oct 25 01:50:20 1998
} > @@ -750,7 +750,7 @@
} >  	 * if they are completely covered, dequeue them.
} >  	 */
} >  	for (; q != NULL && ip->ip_off + ip->ip_len > GETIP(q)->ip_off;
} > -	     p = q, q = nq) {
} > +	     q = nq) {
} >  		i = (ip->ip_off + ip->ip_len) -
} >  		    GETIP(q)->ip_off;
} >  		if (i < GETIP(q)->ip_len) {
} > 
} 
} I don't understand how this patch works.  Won't it end up using the wrong
} value for 'p' later on in the loop?  Could you explain which mbuf is being
} used after its freed and in what circumstances.

Lets say we start off with fp->ipq_frags pointing to the list:

   fp->ipq_frags => frag1(offset 0) => frag2(offset 6) => NULL

and we receive a new fragment with offset 0 that overlaps both
frag1 and frag2.

        /*      
         * Find a segment which begins after this one does.
         */             
        for (p = NULL, q = fp->ipq_frags; q; p = q, q = q->m_nextpkt)
                if (GETIP(q)->ip_off > ip->ip_off)
                        break;
               
after the above loop, we'll have:

   fp->ipq_frags => frag1(offset 0) => frag2(offset 6) => NULL
                    ^                   ^
                    |                   |
                    p                   q

Next, we remove the beginning part of the new fragment that overlaps frag1 if
any (code not shown).

Next, we want to throw away any fragments starting at q that are completely
covered by the new fragment, and if we find a fragment in the list that
partially overlaps the new fragment, we want to remove data from its beginning
to remove the overlap.

        /*
         * While we overlap succeeding segments trim them or,
         * if they are completely covered, dequeue them.
         */      
        for (; q != NULL && ip->ip_off + ip->ip_len > GETIP(q)->ip_off;
             p = q, q = nq) {
                i = (ip->ip_off + ip->ip_len) -
                    GETIP(q)->ip_off;
                if (i < GETIP(q)->ip_len) {
                        GETIP(q)->ip_len -= i;
                        GETIP(q)->ip_off += i;
                        m_adj(q, i);
                        break;
                }       
                nq = q->m_nextpkt;
                if (p)
                        p->m_nextpkt = nq;
                else    
                        fp->ipq_frags = nq;
                m_freem(q);
        }    

The 'if (i < GETIP(q)->ip_len)' clause takes care of the partial overlap
case.

Using 'p = q, q = nq' in the for loop causes the following to be executed
at the end of each loop.

	m_freem(q);
	p = q;
	q = nq;

This means that 'p' references something on the free list.  Also, we
don't want to change 'p' because that is our insertion point for the
new fragment.

        if (p == NULL) {
                m->m_nextpkt = fp->ipq_frags;
                fp->ipq_frags = m;
        } else { 
                m->m_nextpkt = p->m_nextpkt;
                p->m_nextpkt = m;
        }

If 'p' is pointing to an mbuf on the free list, we have just linked the
new fragment in mbuf 'm' to the mbuf on the free list instead of linking
it into the fragment list.  No panic here, though.  What we wanted though
as a result was:
   fp->ipq_frags => frag1(offset 0) => newfrag => NULL


However, if the first packet on the original fragment list had an offset
greater than the incoming packet, the first loop will exit with 'p' == NULL.
When we execute the second loop to free any fragments that are totally
covered, we'll do:
                nq = q->m_nextpkt;  /* NULL */
                if (p)
                        p->m_nextpkt = nq;
                else    
                        fp->ipq_frags = nq;
                m_freem(q);
		p = q;
		q = nq;
and exit the loop because 'q' is now NULL.  Because 'p' was initially NULL,
we'll set fp->ipq_frags to NULL, and reset 'p' to the mbuf on the free list.
Because 'p' is no longer NULL, we'll link the the new fragment to the mbuf
on the free list again, instead of modifying fp->ipq_frags to point to it.

        if (p == NULL) {
                m->m_nextpkt = fp->ipq_frags;
                fp->ipq_frags = m;
        } else { 
                m->m_nextpkt = p->m_nextpkt;
                p->m_nextpkt = m;
        }

This leaves fp->ipq_frags set to NULL, so the following loop exits
immediately, with 'p' being NULL.

        for (p = NULL, q = fp->ipq_frags; q; p = q, q = q->m_nextpkt) {
                if (GETIP(q)->ip_off != next)
                        return (0);
                next += GETIP(q)->ip_len;
        }

And when we execute the following, we dereference a NULL pointer.

        /* Make sure the last packet didn't have the IP_MF flag */
        if (p->m_flags & M_FRAG)
                return (0);   

BOOM!

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199810260715.XAA26243>