Subject: patch for using pfil with bridging code
To: None <tech-net@netbsd.org>
From: Darren Reed <avalon@caligula.anu.edu.au>
List: tech-net
Date: 12/31/2003 03:46:53
--%--multipart-mixed-boundary-1.3639.1072802813--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Hi,
After Julian mentioned he was having problems with stability using
ipfilter with NetBSD as a bridge, I took a look at if_bridge.c and
found two somewhat serious problems.
First, there are no checks to see if m2 is not-NULL before calling
m_freem() on it after pfil_run_hooks() returns. This is an easy fix :)
Second, and most worringly, the code doesn't seem to deal well with
situations where m_split() hasn't been used and m_freem() called.
For example, I think there is a code path through, for non-SNAP
packets, where on a dropped packet I think it can end up doing
something like this:
m_freem(*mp->m_next)
m_freem(*mp)
I've attached a diff and would like others to review it, if not
test it, please. No guarantees, including that it'll compile :)
I've also a question on style - this file is all spaces, no tabs.
Should changes respect the existing format or introduce a mix of
tabs ?
Darren
--%--multipart-mixed-boundary-1.3639.1072802813--%
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Description: c program text
Content-Disposition: attachment; filename="if_bridge.c.diff"
*** if_bridge.c.orig Tue Dec 30 01:08:46 2003
--- if_bridge.c Tue Dec 30 02:29:05 2003
***************
*** 1125,1135 ****
#ifdef PFIL_HOOKS
if (runfilt) {
! if (pfil_run_hooks(&sc->sc_if.if_pfil, &m, dst_ifp, PFIL_OUT) !
! = 0) {
! m_freem(m);
return;
}
}
#endif /* PFIL_HOOKS */
--- 1125,1138 ----
#ifdef PFIL_HOOKS
if (runfilt) {
! if (pfil_run_hooks(&sc->sc_if.if_pfil, &m,
! dst_ifp, PFIL_OUT) != 0) {
! if (m != NULL)
! m_freem(m);
return;
}
+ if (m == NULL)
+ return;
}
#endif /* PFIL_HOOKS */
***************
*** 1377,1385 ****
}
#ifdef PFIL_HOOKS
! if (pfil_run_hooks(&sc->sc_if.if_pfil, &m, m->m_pkthdr.rcvif, PFIL_IN)
! != 0) {
! m_freem(m);
return;
}
if (m == NULL)
--- 1380,1389 ----
}
#ifdef PFIL_HOOKS
! if (pfil_run_hooks(&sc->sc_if.if_pfil, &m,
! m->m_pkthdr.rcvif, PFIL_IN) != 0) {
! if (m != NULL)
! m_freem(m);
return;
}
if (m == NULL)
***************
*** 1925,1934 ****
{
int snap, error, split1, split2, pktlen;
struct ether_header *eh;
! struct mbuf *m1, *m2;
u_int16_t ether_type;
snap = 0;
error = -1; /* Default error if not error == 0 */
eh = mtod(*mp, struct ether_header *);
ether_type = ntohs(eh->ether_type);
--- 1929,1941 ----
{
int snap, error, split1, split2, pktlen;
struct ether_header *eh;
! struct mbuf *m1, *m2, **mp2;
u_int16_t ether_type;
+ mp2 = NULL;
snap = 0;
+ split1 = 0;
+ split2 = 0;
error = -1; /* Default error if not error == 0 */
eh = mtod(*mp, struct ether_header *);
ether_type = ntohs(eh->ether_type);
***************
*** 1973,2019 ****
/* Strip off the Ethernet header---but keep a copy. */
if ((*mp)->m_len == sizeof(struct ether_header)) {
m1 = (*mp)->m_next;
! split1 = 0;
} else {
if ((m1 = m_split(*mp, sizeof(struct ether_header), M_NOWAIT))
== NULL)
goto bad;
split1 = 1;
}
/* Strip off snap header, if present */
if (snap) {
if (m1->m_len == sizeof(struct llc)) {
m2 = m1->m_next;
! split2 = 0;
} else {
if ((m2 = m_split(m1, sizeof(struct llc), M_NOWAIT)) ==
NULL)
goto bad2;
split2 = 1;
}
} else {
m2 = m1;
- split2 = 0; /* XXX: gcc */
}
/*
! * Check basic packet sanity, if the packet is outbound, and
! * run IPF filter.
*/
! if (ether_type == ETHERTYPE_IP &&
! (dir == PFIL_OUT || bridge_ip_checkbasic(&m2) == 0)) {
! error = pfil_run_hooks(&inet_pfil_hook, &m2, ifp, dir);
! if (error) goto bad2;
! }
# ifdef INET6
! if (ether_type == ETHERTYPE_IPV6 &&
! (dir == PFIL_OUT || bridge_ip6_checkbasic(&m2) == 0)) {
! error = pfil_run_hooks(&inet6_pfil_hook, &m2, ifp, dir);
! if (error) goto bad2;
! }
# endif
! if (m2 == NULL) goto bad2;
/*
* Finally, put everything back the way it was and return
*/
--- 1980,2035 ----
/* Strip off the Ethernet header---but keep a copy. */
if ((*mp)->m_len == sizeof(struct ether_header)) {
m1 = (*mp)->m_next;
! mp2 = &m1->m_next;
} else {
if ((m1 = m_split(*mp, sizeof(struct ether_header), M_NOWAIT))
== NULL)
goto bad;
split1 = 1;
+ mp2 = &m1;
}
/* Strip off snap header, if present */
if (snap) {
if (m1->m_len == sizeof(struct llc)) {
m2 = m1->m_next;
! mp2 = &m1->m_next;
} else {
if ((m2 = m_split(m1, sizeof(struct llc), M_NOWAIT)) ==
NULL)
goto bad2;
+ mp2 = &m2;
split2 = 1;
}
} else {
m2 = m1;
}
/*
! * Check basic packet sanity and run IPF through pfil.
*/
! switch (ether_type)
! {
! case ETHERTYPE_IP :
! error = (dir == PFIL_IN) ? bridge_ip_checkbasic(mp2) : 0;
! if (error == 0)
! error = pfil_run_hooks(&inet_pfil_hook, mp2, ifp, dir);
! break;
# ifdef INET6
! case ETHERTYPE_IPV6 :
! error = (dir == PFIL_IN) ? bridge_ip6_checkbasic(mp2) : 0;
! if (error == 0)
! error = pfil_run_hooks(&inet6_pfil_hook, mp2, ifp, dir);
! break;
# endif
! default :
! error = 0;
! break;
! }
+ if (error != 0 || *mp2 == NULL)
+ goto bad2;
+ error = -1;
+
/*
* Finally, put everything back the way it was and return
*/
***************
*** 2034,2042 ****
return 0;
bad2:
! if (snap)
! m_freem(m1);
! m_freem(m2);
bad:
m_freem(*mp);
*mp = NULL;
--- 2050,2061 ----
return 0;
bad2:
! if (*mp2 != NULL) {
! m_freem(*mp2);
! *mp2 = NULL;
! }
! if (split1 != 0 && m1 != NULL)
! m_freem(m1);
bad:
m_freem(*mp);
*mp = NULL;
--%--multipart-mixed-boundary-1.3639.1072802813--%--