Re: IPFW2 layer2 filtering broken - PATCH
:On Mon, Jan 24, 2005 at 11:12:11AM -0800, Jeffrey Hsu wrote:
:> This is an interface problem. When ether_ipfw_chk() does not modify the
:> mbuf, the recomputed eh pointer is incorrect because the mbuf has already
:> been adjusted. An ugly workaround is something like
:>
:> if (IPFW_LOADED && ether_ipfw != 0) {
:> + struct mbuf *n = m;
:> +
:> if (!ether_ipfw_chk(&m, NULL, &rule, eh, FALSE)) {
:> m_freem(m);
:> return;
:> }
:> - eh = mtod(m, struct ether_header *);
:> + if (m != n)
:> + eh = mtod(m, struct ether_header *);
:> }
:>
:> Alternatively, we could change the 4th parameter to ether_ipfw_chk()
:> to &eh and update it inside ether_ipfw_chk().
:
:I'm not even sure if that is enough. The problem is that
:ehter_ipfw_chk does an m_pullup. That can return a new mbuf.
:So far so bad. The first problem is that the ether header might
:be part of the old mbuf, but is not copied because of the m_adj
:done earlier. This is a race condition. The second problem is that
:ether header might really be outside the mbuf, in which case the
:modifiction is just lost.
:
:As a solution, ether_ipfw_chk has to update the eh pointer itself
:and deal with the case of eh being part of the header. Do we care
:enough about a few cycles? If not, we could just copy the ether header
:into a temporary buffer, do the m_pullup and copy it into the new
:mbuf if necessary.
:
:Joerg
What about the code around line 395 of if_ethersubr.c? That looks
like a horror as well:
no_bridge:
s = splimp();
if (IPFW_LOADED && ether_ipfw != 0) {
struct ether_header save_eh, *eh;
eh = mtod(m, struct ether_header *);
save_eh = *eh;
m_adj(m, ETHER_HDR_LEN);
if (!ether_ipfw_chk(&m, ifp, &rule, eh, FALSE)) {
if (m != NULL) {
m_freem(m);
return ENOBUFS; /* pkt dropped */
} else
return 0; /* consumed e.g. in a pipe */
}
eh = mtod(m, struct ether_header *);
/* packet was ok, restore the ethernet header */
if ((void *)(eh + 1) == (void *)m->m_data) {
m->m_data -= ETHER_HDR_LEN ;
m->m_len += ETHER_HDR_LEN ;
m->m_pkthdr.len += ETHER_HDR_LEN ;
} else {
M_PREPEND(m, ETHER_HDR_LEN, MB_DONTWAIT);
if (m == NULL) /* nope... */
return ENOBUFS;
bcopy(&save_eh, mtod(m, struct ether_header *),
ETHER_HDR_LEN);
}
}
The above code has clearly been hacked (badly!) to deal with header
messes. It has to be rewritten as well.
Maybe the solution is to do the m_adj() after the ether_ipfw_chk()
call instead of before, and pass a 'skip' offset into ether_ipfw_chk().
ether_ipfw_chk() would be responsible for retaining the header across
operations.
so e.g.
eh = mtod(m, struct ether_header *);
if (!ether_ipfw_chk(&m, ETHER_HDR_LEN, ifp, &rule, eh, FALSE)) {
...
}
eh = mtod(m, struct ether_header *);
m_adj(m, ETHER_HDR_LEN);
...
-Matt
Matthew Dillon
<dillon@backplane.com>
討論串 (同標題文章)
完整討論串 (本文為第 7 之 8 篇)：