From owner-freebsd-net@FreeBSD.ORG Tue Jun 5 08:25:09 2012 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 74BDE106566B; Tue, 5 Jun 2012 08:25:09 +0000 (UTC) (envelope-from dhartmei@insomnia.benzedrine.cx) Received: from insomnia.benzedrine.cx (106-30.3-213.fix.bluewin.ch [213.3.30.106]) by mx1.freebsd.org (Postfix) with ESMTP id D69778FC16; Tue, 5 Jun 2012 08:25:07 +0000 (UTC) Received: from insomnia.benzedrine.cx (localhost.benzedrine.cx [127.0.0.1]) by insomnia.benzedrine.cx (8.14.1/8.13.4) with ESMTP id q558P0u7028587 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES256-SHA bits=256 verify=NO); Tue, 5 Jun 2012 10:25:00 +0200 (MEST) Received: (from dhartmei@localhost) by insomnia.benzedrine.cx (8.14.1/8.12.10/Submit) id q558P0Jb016927; Tue, 5 Jun 2012 10:25:00 +0200 (MEST) Date: Tue, 5 Jun 2012 10:25:00 +0200 From: Daniel Hartmeier To: freebsd-net@freebsd.org Message-ID: <20120605082500.GD13069@insomnia.benzedrine.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.12-2006-07-14 Cc: Darren Reed Subject: pfil invariant proposal: mbuf begins with contiguous IP header X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jun 2012 08:25:09 -0000 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