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>