Date: Tue, 29 May 2012 08:49:10 +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: <20120529064910.GA12508@insomnia.benzedrine.cx> In-Reply-To: <201205271830.q4RIU9fA039893@freefall.freebsd.org> References: <201205271830.q4RIU9fA039893@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--vkogqOf2sHV7VnPd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
On Sun, May 27, 2012 at 06:30:09PM +0000, Joerg Pulz wrote:
> i've seen 12 more "pf_route: m0->m_len < sizeof(struct ip)" messages since the system is running after adding your patch, but no panic.
> Is there another place in the code where i can add an additional check?
Yes, the following patch adds more checks to pf.
Kind regards,
Daniel
--vkogqOf2sHV7VnPd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="len.diff"
Index: sys/contrib/pf/net/pf.c
===================================================================
RCS file: /home/ncvs/src/sys/contrib/pf/net/pf.c,v
retrieving revision 1.78.2.6
diff -u -r1.78.2.6 pf.c
--- sys/contrib/pf/net/pf.c 29 Feb 2012 09:47:26 -0000 1.78.2.6
+++ sys/contrib/pf/net/pf.c 29 May 2012 06:39:54 -0000
@@ -2560,6 +2560,7 @@
case AF_INET:
#ifdef __FreeBSD__
/* icmp_error() expects host byte ordering */
+ ASSERT_NET_BYTE_ORDER(m0);
ip = mtod(m0, struct ip *);
NTOHS(ip->ip_len);
NTOHS(ip->ip_off);
@@ -5894,6 +5895,13 @@
(dir != PF_IN && dir != PF_OUT) || oifp == NULL)
panic("pf_route: invalid parameters");
+ ASSERT_NET_BYTE_ORDER(*m);
+
+ if ((*m)->m_pkthdr.len < sizeof(struct ip) ||
+ (*m)->m_len < sizeof(struct ip))
+ panic("pf_route: 1: (*m)->m_pkthdr.len %d, (*m)->m_len %d",
+ (int)(*m)->m_pkthdr.len, (int)(*m)->m_len);
+
#ifdef __FreeBSD__
if (pd->pf_mtag->routed++ > 3) {
#else
@@ -5917,9 +5925,14 @@
m0 = *m;
}
+ if (m0->m_pkthdr.len < sizeof(struct ip) ||
+ m0->m_len < sizeof(struct ip))
+ panic("pf_route: 2: m0->m_pkthdr.len %d, m0->m_len %d",
+ (int)m0->m_pkthdr.len, (int)m0->m_len);
+
if (m0->m_len < sizeof(struct ip)) {
DPFPRINTF(PF_DEBUG_URGENT,
- ("pf_route: m0->m_len < sizeof(struct ip)\n"));
+ ("pf_route: a: m0->m_len < sizeof(struct ip)\n"));
goto bad;
}
@@ -5975,8 +5988,14 @@
if (ifp == NULL)
goto bad;
+ if (m0->m_pkthdr.len < sizeof(struct ip) ||
+ m0->m_len < sizeof(struct ip))
+ panic("pf_route: 2: m0->m_pkthdr.len %d, m0->m_len %d",
+ (int)m0->m_pkthdr.len, (int)m0->m_len);
+
if (oifp != ifp) {
#ifdef __FreeBSD__
+ ASSERT_NET_BYTE_ORDER(m0);
PF_UNLOCK();
if (pf_test(PF_OUT, ifp, &m0, NULL, NULL) != PF_PASS) {
PF_LOCK();
@@ -5992,12 +6011,18 @@
else if (m0 == NULL)
goto done;
#endif
+ if (m0->m_pkthdr.len < sizeof(struct ip) ||
+ m0->m_len < sizeof(struct ip))
+ panic("pf_route: 3: m0->m_pkthdr.len %d, m0->m_len %d",
+ (int)m0->m_pkthdr.len, (int)m0->m_len);
+
if (m0->m_len < sizeof(struct ip)) {
DPFPRINTF(PF_DEBUG_URGENT,
- ("pf_route: m0->m_len < sizeof(struct ip)\n"));
+ ("pf_route: b: m0->m_len < sizeof(struct ip)\n"));
goto bad;
}
ip = mtod(m0, struct ip *);
+ ASSERT_NET_BYTE_ORDER(m0);
}
#ifdef __FreeBSD__
@@ -6008,6 +6033,7 @@
/*
* XXX: in_delayed_cksum assumes HBO for ip->ip_len (at least)
*/
+ ASSERT_NET_BYTE_ORDER(m0);
NTOHS(ip->ip_len);
NTOHS(ip->ip_off); /* XXX: needed? */
in_delayed_cksum(m0);
@@ -6017,6 +6043,8 @@
}
m0->m_pkthdr.csum_flags &= ifp->if_hwassist;
+ ASSERT_NET_BYTE_ORDER(m0);
+
if (ntohs(ip->ip_len) <= ifp->if_mtu ||
(ifp->if_hwassist & CSUM_FRAGMENT &&
((ip->ip_off & htons(IP_DF)) == 0))) {
@@ -6104,6 +6132,7 @@
if (r->rt != PF_DUPTO) {
#ifdef __FreeBSD__
/* icmp_error() expects host byte ordering */
+ ASSERT_NET_BYTE_ORDER(m0);
NTOHS(ip->ip_len);
NTOHS(ip->ip_off);
PF_UNLOCK();
@@ -6124,6 +6153,7 @@
/*
* XXX: is cheaper + less error prone than own function
*/
+ ASSERT_NET_BYTE_ORDER(m0);
NTOHS(ip->ip_len);
NTOHS(ip->ip_off);
error = ip_fragment(ip, &m0, ifp->if_mtu, ifp->if_hwassist, sw_csum);
@@ -6672,6 +6702,8 @@
#endif /* DIAGNOSTIC */
#endif
+ ASSERT_NET_BYTE_ORDER(m);
+
if (m->m_pkthdr.len < (int)sizeof(*h)) {
action = PF_DROP;
REASON_SET(&reason, PFRES_SHORT);
@@ -6679,6 +6711,11 @@
goto done;
}
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 1: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
#ifdef __FreeBSD__
if (m->m_flags & M_SKIP_FIREWALL) {
PF_UNLOCK();
@@ -6711,6 +6748,11 @@
m = *m0; /* pf_normalize messes with m0 */
h = mtod(m, struct ip *);
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 2: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
off = h->ip_hl << 2;
if (off < (int)sizeof(*h)) {
action = PF_DROP;
@@ -6740,6 +6782,11 @@
goto done;
}
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 3: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
switch (h->ip_p) {
case IPPROTO_TCP: {
@@ -6891,6 +6938,11 @@
}
done:
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 4: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
if (action == PF_PASS && h->ip_hl > 5 &&
!((s && s->state_flags & PFSTATE_ALLOWOPTS) || r->allow_opts)) {
action = PF_DROP;
@@ -6935,6 +6987,11 @@
}
#endif /* ALTQ */
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 5: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
/*
* connections redirected to loopback should not match sockets
* bound specifically to loopback due to security implications,
@@ -6996,6 +7053,11 @@
}
#endif
+ if (m->m_pkthdr.len < sizeof(struct ip) ||
+ m->m_len < sizeof(struct ip))
+ panic("pf_test: 6: m->m_pkthdr.len %d, m->m_len %d",
+ (int)m->m_pkthdr.len, (int)m->m_len);
+
if (log) {
struct pf_rule *lr;
@@ -7069,8 +7131,14 @@
break;
default:
/* pf_route can free the mbuf causing *m0 to become NULL */
- if (r->rt)
+ if (r->rt) {
+ if ((*m0)->m_pkthdr.len < sizeof(struct ip) ||
+ (*m0)->m_len < sizeof(struct ip))
+ panic("pf_test: 7: m0->m_pkthdr.len %d, "
+ "m0->m_len %d", (int)(*m0)->m_pkthdr.len,
+ (int)(*m0)->m_len);
pf_route(m0, r, dir, kif->pfik_ifp, s, &pd);
+ }
break;
}
#ifdef __FreeBSD__
--vkogqOf2sHV7VnPd--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120529064910.GA12508>