Date: Tue, 5 Jun 2012 10:25:00 +0200 From: Daniel Hartmeier <daniel@benzedrine.cx> To: freebsd-net@freebsd.org Cc: Darren Reed <darrenr@freebsd.org> Subject: pfil invariant proposal: mbuf begins with contiguous IP header Message-ID: <20120605082500.GD13069@insomnia.benzedrine.cx>
next in thread | raw e-mail | index | archive | help
Quoting from pfil(9)
When a filter is invoked, the packet appears just as if it ``came off the
wire''. That is, all protocol fields are in network byte order. [...]
This should be extended to include the guarantee that the mbuf begins
with a contiguous IP header, i.e. mtod(*mp, struct ip *) may be used to
access all IP header fields.
And the hooks should be required to guarantee this for the mbufs they
return.
All pfil hooks rely on this to be true for input mbufs, and callers of
pfil_run_hooks() expect it to be true for resulting mbufs.
Currently, ipfilter violates this in certain cases (see kern/168190 for
details), which causes errors and crashes in other pfil hooks.
For example, ipfilter might run as the first hook, and return an mbuf
with a leading segment of m_len == 0. Next, ipfw as the second hook
will swap byte order of the IP header, but a subsequent m_pullup()
overwrites the header. Finally, pf as the third hook will cause a call
to m_copym() with much-too-large length (due to reversed byte order).
The following patch both demonstrates and fixes the problem:
--- sys/contrib/ipfilter/netinet/ip_fil_freebsd.c 23 Sep 2011 00:51:37 -0000 1.20.4.1
+++ sys/contrib/ipfilter/netinet/ip_fil_freebsd.c 5 Jun 2012 08:05:33 -0000
@@ -183,7 +183,14 @@
fr_check_wrapper(void *arg, struct mbuf **mp, struct ifnet *ifp, int dir)
{
struct ip *ip = mtod(*mp, struct ip *);
- return fr_check(ip, ip->ip_hl << 2, ifp, (dir == PFIL_OUT), mp);
+ int r = fr_check(ip, ip->ip_hl << 2, ifp, (dir == PFIL_OUT), mp);
+ if (*mp != NULL && (*mp)->m_pkthdr.len >= sizeof(struct ip) &&
+ (*mp)->m_len < sizeof(struct ip)) {
+ printf("fr_check_wrapper: m_len %d fixed\n",
+ (int)(*mp)->m_len);
+ *mp = m_pullup(*mp, sizeof(struct ip));
+ }
+ return r;
}
# ifdef USE_INET6
Maybe someone can provide a more efficient check closer to the origin
of the empty leading segment, possibly fr_pullup() calling m_pulldown()?
Or simply commit the above without the printf(), if you agree. :)
Kind regards,
Daniel
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120605082500.GD13069>