Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 25 May 2012 11:16:27 +0200
From:      Daniel Hartmeier <daniel@benzedrine.cx>
To:        Joerg Pulz <Joerg.Pulz@frm2.tum.de>
Cc:        freebsd-pf@freebsd.org
Subject:   Re: kern/168190: [pf] panic when using pf and route-to (maybe: bad fragment handling?)
Message-ID:  <20120525091627.GA27514@insomnia.benzedrine.cx>
In-Reply-To: <201205250730.q4P7UGu0006036@freefall.freebsd.org>
References:  <201205250730.q4P7UGu0006036@freefall.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, May 25, 2012 at 07:30:16AM +0000, Joerg Pulz wrote:

>  the system is still running without panic, but i found the following log 
>  entries from last night:
>  
>  May 24 23:28:57 charon kernel: pf_route: m0->m_len < sizeof(struct ip)
>  May 24 23:28:57 charon kernel: pf_route: m0->m_len < sizeof(struct ip)
>  
>  Do you think that this may be related to the panics?
>  I've found this error message two times in contrib/pf/net/pf.c.
>  I can't say which of them or both have printed the message.

Yes, this could possibly explain it.

All pfil consumers assume that the IP header is one continuous memory
region in the mbuf, without verifying this or correcting it (with
m_pullup() or such) if wrong.

pf: pf_check_in() in sys/contrib/pf/net/pf_ioctl.c
	h = mtod(*m, struct ip *);
	access h->ip_len

ipfw: ipfw_check_hook() in sys/netinet/ipfw/ip_fw_pfil.c
	SET_NET_IPLEN(mtod(*m0, struct ip *));

ipfilter: fr_check_wrapper() in sys/contrib/ipfilter/netinet/ip_fil_freebsd.c
	struct ip *ip = mtod(*mp, struct ip *);
	access ip->ip_hl

Hence, the caller of pfil_run_hooks() must ensure this before the call.
ip_input() does, but there are operations that might violate the
condition again.

If the IP header is not continuous in the first mbuf, doing

	struct ip *ip = mtod(m, struct ip *);
	ip->ip_len = ntohs(ip->ip_len);

will read (and write) unrelated memory.

If, later, something does call m_pullup(), ip_len will get 'replaced'
again. This could lead to some byte order swaps getting undone.

I'm not sure what the proper action is here, i.e. should we be surprised
that an mbuf with such a small m_len is found, and track down how it
was produced, or should the pfil code simply expect this?

I'd probably add a sanity check to pfil_run_hooks(), like

--- sys/net/pfil.c      23 Sep 2011 00:51:37 -0000      1.19.2.1
+++ sys/net/pfil.c      25 May 2012 09:10:27 -0000
@@ -46,6 +46,8 @@

 #include <net/if.h>
 #include <net/pfil.h>
+#include <netinet/in.h>
+#include <netinet/ip.h>

 static struct mtx pfil_global_lock;

@@ -74,15 +76,21 @@
        struct mbuf *m = *mp;
        int rv = 0;

+       if (m->m_pkthdr.len < sizeof(struct ip) ||
+           m->m_len < sizeof(struct ip))
+               panic("pfil_run_hooks: m->m_pkthdr.len %d, m->m_len %d",
+                   (int)m->m_pkthdr.len, (int)m->m_len);
        PFIL_RLOCK(ph, &rmpt);
        KASSERT(ph->ph_nhooks >= 0, ("Pfil hook count dropped < 0"));
        for (pfh = pfil_hook_get(dir, ph); pfh != NULL;
             pfh = TAILQ_NEXT(pfh, pfil_link)) {
                if (pfh->pfil_func != NULL) {
+                       ASSERT_HOST_BYTE_ORDER(m);
                        rv = (*pfh->pfil_func)(pfh->pfil_arg, &m, ifp, dir,
                            inp);
                        if (rv != 0 || m == NULL)
                                break;
+                       ASSERT_HOST_BYTE_ORDER(m);
                }
        }
        PFIL_RUNLOCK(ph, &rmpt);

Then when it will panic (instead of just the pf_route() message), the
stack trace could help.

This might require several iterations, adding such checks all around the
place. The cause might be some mbuf operation done in ipsec, for some
edge case, explaining why it occurs so rarely...

If I could easily reproduce it locally, I'd probably do it, but it's
your machine that crashes all the time, so it's your call :)

Daniel



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