Re: IPFW2 layer2 filtering broken - PATCH

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

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