On Wed, Jun 15, 2011 at 11:06:16PM -0400, Kenneth R Westerback wrote:
> On Wed, Jun 15, 2011 at 09:50:51PM -0400, Kenneth R Westerback wrote:
> > On Tue, Jun 14, 2011 at 10:23:36PM +0200, Claudio Jeker wrote:
> > > On Wed, Jun 08, 2011 at 09:53:24AM +0200, Claudio Jeker wrote:
> > > > Next step on my quest to unify pf_test and pf_test6.
> > > > Move the fragment handling and some other protocol specific tests into
> > > > pf_setup_pdesc(). IPv6 already does this mostly but only IPv4 did not.
> > > > So
> > > > this diff brings that more into sync. It also includes some additional
> > > > cleanups.
> > > >
> > > > Works for me but needs some more testing.
> > >
> > > New version with fixes found by bluhm@ (mostly make sure that the right
> > > header and mbufs are used in all cases).
> > >
> > > --
> > > :wq Claudio
> >
> > Blows up REAL good on my 6xamd64 box. Within a minute or two.
> >
> > I put some debug in and pf_scrub() is being passed a NULL 'm' parameter
> > from pf_test(). Hand transcribed trace:
> >
> > pf_scrub()+0x12
> > pf_test()+0x93b
> > ipv4_input()+0x22a
> > ipintr()+0x51
> > netintr()+0xda
> > softintr_dispatch()+0x5d
> > Xsoftnet()+0x2d
> >
> > This box is my nfs server and is using the default pf.conf file as far
> > as I know. Also blew up on my firewall as soon as traffic started
> > transitting it. Same trace as far as I can see.
> >
> > .... Ken
> >
>
> panic'ing my way along the stack I find that the problem starts in
> pf_reassemble(), when the 'if (!pf_isfull_fragment(frag))' test
> leaves m0 as NULL and returns PF_PASS to pf_normalize_ip(). *m0 is
> tested and found to be NULL, so pf_normalize_ip() returns PF_PASS to
> pf_setup_pdesc(). Where *m0 is tested for NULL and causes *action to
> be set to PF_PASS and pf_setup_pdesc() to return -1 to pf_test. m is
> set to *m0 (i.e. NULL) and then pf_test goto's done:. action is PF_PASS
> and 'if (s)' fails so control passes to the first statement of the else,
> which is a pf_scrub() call with m being NULL. Boom.
>
Yes, very good analysis. I came to the same conclusion. I do not
understand why we do the goto done; case for PF_PASS in case of a
pf_setup_pdesc() failure. Since in that case *m0 is NULL and so we can
just return directly from there. I changed my diff to do exactly that. On
further inspection I think there is no other case where pf_setup_pdesc()
returns -1 but action is set to PF_PASS. IMO this is a lot saver then
trying to execute all that code after the done: label.
New diff below
--
:wq Claudio
Index: net/if_pflog.c
===================================================================
RCS file: /cvs/src/sys/net/if_pflog.c,v
retrieving revision 1.34
diff -u -p -r1.34 if_pflog.c
--- net/if_pflog.c 22 May 2011 13:21:24 -0000 1.34
+++ net/if_pflog.c 24 May 2011 15:56:53 -0000
@@ -334,7 +334,7 @@ pflog_bpfcopy(const void *src_arg, void
/* rewrite addresses if needed */
memset(&pd, 0, sizeof(pd));
pd.hdr.any = &pf_hdrs;
- if (pf_setup_pdesc(pfloghdr->af, pfloghdr->dir, &pd, mfake, &action,
+ if (pf_setup_pdesc(pfloghdr->af, pfloghdr->dir, &pd, &mfake, &action,
&reason, NULL, NULL, NULL, NULL, &off, &hdrlen) == -1)
return;
Index: net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.748
diff -u -p -r1.748 pf.c
--- net/pf.c 14 Jun 2011 10:14:01 -0000 1.748
+++ net/pf.c 16 Jun 2011 21:10:32 -0000
@@ -5467,10 +5467,12 @@ pf_get_divert(struct mbuf *m)
}
int
-pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf *m,
+pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0,
u_short *action, u_short *reason, struct pfi_kif *kif, struct pf_rule **a,
struct pf_rule **r, struct pf_ruleset **ruleset, int *off, int *hdrlen)
{
+ struct mbuf *m = *m0;
+
if (pd->hdr.any == NULL)
panic("pf_setup_pdesc: no storage for headers provided");
@@ -5480,13 +5482,23 @@ pf_setup_pdesc(sa_family_t af, int dir,
case AF_INET: {
struct ip *h;
+ /* Check for illegal packets */
+ if (m->m_pkthdr.len < (int)sizeof(struct ip)) {
+ *action = PF_DROP;
+ REASON_SET(reason, PFRES_SHORT);
+ return (-1);
+ }
+
h = mtod(m, struct ip *);
*off = h->ip_hl << 2;
- if (*off < (int)sizeof(*h)) {
+
+ if (*off < (int)sizeof(struct ip) ||
+ *off > ntohs(h->ip_len)) {
*action = PF_DROP;
REASON_SET(reason, PFRES_SHORT);
return (-1);
}
+
pd->src = (struct pf_addr *)&h->ip_src;
pd->dst = (struct pf_addr *)&h->ip_dst;
pd->sport = pd->dport = NULL;
@@ -5501,9 +5513,43 @@ pf_setup_pdesc(sa_family_t af, int dir,
pd->tot_len = ntohs(h->ip_len);
pd->rdomain = rtable_l2(m->m_pkthdr.rdomain);
- /* fragments not reassembled handled later */
- if (h->ip_off & htons(IP_MF | IP_OFFMASK))
- return (0);
+ if (h->ip_off & htons(IP_MF | IP_OFFMASK)) {
+ if (!pf_status.reass) {
+ /*
+ * handle fragments that aren't reassembled by
+ * normalization
+ */
+ if (kif == NULL || r == NULL) /* pflog */
+ *action = PF_DROP;
+ else
+ *action = pf_test_fragment(r, dir, kif,
+ m, pd, a, ruleset);
+ if (*action == PF_PASS)
+ /* m is still valid, return success */
+ return (0);
+ REASON_SET(reason, PFRES_FRAG);
+ return (-1);
+ }
+ /* packet reassembly */
+ if (pf_normalize_ip(m0, dir, reason, pd) != PF_PASS) {
+ *action = PF_DROP;
+ return (-1);
+ }
+ m = *m0;
+ if (m == NULL) {
+ /* packet sits in reassembly queue, no error */
+ *action = PF_PASS;
+ return (-1);
+ }
+ /* refetch header, recalc offset and update pd */
+ h = mtod(m, struct ip *);
+ *off = h->ip_hl << 2;
+ pd->src = (struct pf_addr *)&h->ip_src;
+ pd->dst = (struct pf_addr *)&h->ip_dst;
+ pd->ip_sum = &h->ip_sum;
+ pd->tot_len = ntohs(h->ip_len);
+ pd->tos = h->ip_tos;
+ }
switch (h->ip_p) {
case IPPROTO_TCP: {
@@ -5552,7 +5598,39 @@ pf_setup_pdesc(sa_family_t af, int dir,
struct ip6_hdr *h;
int terminal = 0;
+ /* Check for illegal packets */
+ if (m->m_pkthdr.len < (int)sizeof(struct ip6_hdr)) {
+ *action = PF_DROP;
+ REASON_SET(reason, PFRES_SHORT);
+ return (-1);
+ }
+
+ /* packet reassembly */
+ if (pf_status.reass &&
+ pf_normalize_ip6(m0, dir, reason, pd) != PF_PASS) {
+ *action = PF_DROP;
+ return (-1);
+ }
+ m = *m0;
+ if (m == NULL) {
+ /* packet sits in reassembly queue, no error */
+ *action = PF_PASS;
+ return (-1);
+ }
+
h = mtod(m, struct ip6_hdr *);
+#if 1
+ /*
+ * we do not support jumbogram yet. if we keep going, zero
+ * ip6_plen will do something bad, so drop the packet for now.
+ */
+ if (htons(h->ip6_plen) == 0) {
+ *action = PF_DROP;
+ REASON_SET(reason, PFRES_NORM);
+ return (-1);
+ }
+#endif
+
pd->src = (struct pf_addr *)&h->ip6_src;
pd->dst = (struct pf_addr *)&h->ip6_dst;
pd->sport = pd->dport = NULL;
@@ -5757,7 +5835,6 @@ pf_test(int dir, struct ifnet *ifp, stru
struct pfi_kif *kif;
u_short action, reason = 0;
struct mbuf *m = *m0;
- struct ip *h;
struct pf_rule *a = NULL, *r = &pf_default_rule;
struct pf_state *s = NULL;
struct pf_ruleset *ruleset = NULL;
@@ -5789,13 +5866,6 @@ pf_test(int dir, struct ifnet *ifp, stru
panic("non-M_PKTHDR is passed to pf_test");
#endif /* DIAGNOSTIC */
- if (m->m_pkthdr.len < (int)sizeof(*h)) {
- action = PF_DROP;
- REASON_SET(&reason, PFRES_SHORT);
- pd.pflog |= PF_LOG_FORCE;
- goto done;
- }
-
if (m->m_pkthdr.pf.flags & PF_TAG_GENERATED)
return (PF_PASS);
@@ -5807,40 +5877,23 @@ pf_test(int dir, struct ifnet *ifp, stru
return (PF_PASS);
}
- /* packet reassembly here if 1) enabled 2) we deal with a fragment */
- h = mtod(m, struct ip *);
- if (pf_status.reass &&
- (h->ip_off & htons(IP_MF | IP_OFFMASK)) &&
- pf_normalize_ip(m0, dir, kif, &reason, &pd) != PF_PASS) {
- action = PF_DROP;
- goto done;
- }
- m = *m0; /* pf_normalize messes with m0 */
- if (m == NULL)
- return (PF_PASS);
- h = mtod(m, struct ip *);
-
- if (pf_setup_pdesc(AF_INET, dir, &pd, m, &action, &reason, kif, &a, &r,
+ if (pf_setup_pdesc(AF_INET, dir, &pd, m0, &action, &reason, kif, &a, &r,
&ruleset, &off, &hdrlen) == -1) {
- if (action != PF_PASS)
- pd.pflog |= PF_LOG_FORCE;
+ if (action == PF_PASS)
+ return (PF_PASS);
+ m = *m0;
+ pd.pflog |= PF_LOG_FORCE;
goto done;
}
pd.eh = eh;
-
- /* handle fragments that didn't get reassembled by normalization */
- if (h->ip_off & htons(IP_MF | IP_OFFMASK)) {
- action = pf_test_fragment(&r, dir, kif, m,
- &pd, &a, &ruleset);
- goto done;
- }
+ m = *m0; /* pf_setup_pdesc -> pf_normalize messes with m0 */
switch (pd.proto) {
case IPPROTO_TCP: {
if ((pd.hdr.tcp->th_flags & TH_ACK) && pd.p_len == 0)
pqid = 1;
- action = pf_normalize_tcp(dir, kif, m, 0, off, h, &pd);
+ action = pf_normalize_tcp(dir, m, off, &pd);
if (action == PF_DROP)
goto done;
action = pf_test_state_tcp(&s, dir, kif, m, off, &pd,
@@ -5922,6 +5975,7 @@ done:
if (action != PF_DROP) {
if (s) {
/* The non-state case is handled in pf_test_rule() */
+ struct ip *h = mtod(m, struct ip *);
if (action == PF_PASS && h->ip_hl > 5 &&
!(s->state_flags & PFSTATE_ALLOWOPTS)) {
action = PF_DROP;
@@ -5954,7 +6008,7 @@ done:
#ifdef ALTQ
if (action == PF_PASS && qid) {
m->m_pkthdr.pf.qid = qid;
- m->m_pkthdr.pf.hdr = h; /* hints for ecn */
+ m->m_pkthdr.pf.hdr = mtod(m, caddr_t); /* hints for ecn */
}
#endif /* ALTQ */
@@ -6038,7 +6092,6 @@ pf_test6(int fwdir, struct ifnet *ifp, s
u_short action, reason = 0;
struct mbuf *m = *m0;
struct m_tag *mtag;
- struct ip6_hdr *h;
struct pf_rule *a = NULL, *r = &pf_default_rule;
struct pf_state *s = NULL;
struct pf_ruleset *ruleset = NULL;
@@ -6071,13 +6124,6 @@ pf_test6(int fwdir, struct ifnet *ifp, s
panic("non-M_PKTHDR is passed to pf_test6");
#endif /* DIAGNOSTIC */
- if (m->m_pkthdr.len < (int)sizeof(*h)) {
- action = PF_DROP;
- REASON_SET(&reason, PFRES_SHORT);
- pd.pflog |= PF_LOG_FORCE;
- goto done;
- }
-
if (m->m_pkthdr.pf.flags & PF_TAG_GENERATED)
return (PF_PASS);
@@ -6089,44 +6135,23 @@ pf_test6(int fwdir, struct ifnet *ifp, s
return (PF_PASS);
}
- /* packet reassembly */
- if (pf_status.reass &&
- pf_normalize_ip6(m0, fwdir, kif, &reason, &pd) != PF_PASS) {
- action = PF_DROP;
- goto done;
- }
- m = *m0; /* pf_normalize messes with m0 */
- if (m == NULL)
- return (PF_PASS);
- h = mtod(m, struct ip6_hdr *);
-
-#if 1
- /*
- * we do not support jumbogram yet. if we keep going, zero ip6_plen
- * will do something bad, so drop the packet for now.
- */
- if (htons(h->ip6_plen) == 0) {
- action = PF_DROP;
- REASON_SET(&reason, PFRES_NORM);
+ if (pf_setup_pdesc(AF_INET6, dir, &pd, m0, &action, &reason, kif, &a,
+ &r, &ruleset, &off, &hdrlen) == -1) {
+ if (action == PF_PASS)
+ return (PF_PASS);
+ m = *m0;
pd.pflog |= PF_LOG_FORCE;
goto done;
}
-#endif
-
- if (pf_setup_pdesc(AF_INET6, dir, &pd, m, &action, &reason, kif, &a, &r,
- &ruleset, &off, &hdrlen) == -1) {
- if (action != PF_PASS)
- pd.pflog |= PF_LOG_FORCE;
- goto done;
- }
pd.eh = eh;
+ m = *m0; /* pf_setup_pdesc -> pf_normalize messes with m0 */
switch (pd.proto) {
case IPPROTO_TCP: {
if ((pd.hdr.tcp->th_flags & TH_ACK) && pd.p_len == 0)
pqid = 1;
- action = pf_normalize_tcp(dir, kif, m, 0, off, h, &pd);
+ action = pf_normalize_tcp(dir, m, off, &pd);
if (action == PF_DROP)
goto done;
action = pf_test_state_tcp(&s, dir, kif, m, off, &pd,
@@ -6240,7 +6265,7 @@ done:
#ifdef ALTQ
if (action == PF_PASS && qid) {
m->m_pkthdr.pf.qid = qid;
- m->m_pkthdr.pf.hdr = h; /* add hints for ecn */
+ m->m_pkthdr.pf.hdr = mtod(m, caddr_t); /* add hints for ecn */
}
#endif /* ALTQ */
Index: net/pf_norm.c
===================================================================
RCS file: /cvs/src/sys/net/pf_norm.c,v
retrieving revision 1.133
diff -u -p -r1.133 pf_norm.c
--- net/pf_norm.c 24 May 2011 14:01:52 -0000 1.133
+++ net/pf_norm.c 24 May 2011 16:57:05 -0000
@@ -739,30 +739,22 @@ pf_refragment6(struct mbuf **m0, struct
#endif /* INET6 */
int
-pf_normalize_ip(struct mbuf **m0, int dir, struct pfi_kif *kif,
- u_short *reason, struct pf_pdesc *pd)
+pf_normalize_ip(struct mbuf **m0, int dir, u_short *reason,
+ struct pf_pdesc *pd)
{
struct mbuf *m = *m0;
struct ip *h = mtod(m, struct ip *);
- int hlen = h->ip_hl << 2;
u_int16_t fragoff = (ntohs(h->ip_off) & IP_OFFMASK) << 3;
u_int16_t mff = (ntohs(h->ip_off) & IP_MF);
- /* Check for illegal packets */
- if (hlen < (int)sizeof(struct ip))
- goto drop;
-
- if (hlen > ntohs(h->ip_len))
- goto drop;
+ /* We will need other tests here */
+ if (!fragoff && !mff)
+ goto no_fragment;
/* Clear IP_DF if we're in no-df mode */
if (pf_status.reass & PF_REASS_NODF && h->ip_off & htons(IP_DF))
h->ip_off &= htons(~IP_DF);
- /* We will need other tests here */
- if (!fragoff && !mff)
- goto no_fragment;
-
/* We're dealing with a fragment now. Don't allow fragments
* with IP_DF to enter the cache. If the flag was cleared by
* no-df above, fine. Otherwise drop it.
@@ -789,16 +781,12 @@ pf_normalize_ip(struct mbuf **m0, int di
pd->flags |= PFDESC_IP_REAS;
return (PF_PASS);
-
- drop:
- REASON_SET(reason, PFRES_NORM);
- return (PF_DROP);
}
#ifdef INET6
int
-pf_normalize_ip6(struct mbuf **m0, int dir, struct pfi_kif *kif,
- u_short *reason, struct pf_pdesc *pd)
+pf_normalize_ip6(struct mbuf **m0, int dir, u_short *reason,
+ struct pf_pdesc *pd)
{
struct mbuf *m = *m0;
struct ip6_hdr *h = mtod(m, struct ip6_hdr *);
@@ -826,7 +814,6 @@ pf_normalize_ip6(struct mbuf **m0, int d
switch (proto) {
case IPPROTO_FRAGMENT:
goto fragment;
- break;
case IPPROTO_AH:
case IPPROTO_ROUTING:
case IPPROTO_DSTOPTS:
@@ -938,8 +925,7 @@ pf_normalize_ip6(struct mbuf **m0, int d
#endif /* INET6 */
int
-pf_normalize_tcp(int dir, struct pfi_kif *kif, struct mbuf *m, int ipoff,
- int off, void *h, struct pf_pdesc *pd)
+pf_normalize_tcp(int dir, struct mbuf *m, int off, struct pf_pdesc *pd)
{
struct tcphdr *th = pd->hdr.tcp;
u_short reason;
Index: net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.332
diff -u -p -r1.332 pfvar.h
--- net/pfvar.h 24 May 2011 14:01:52 -0000 1.332
+++ net/pfvar.h 24 May 2011 16:57:22 -0000
@@ -1743,7 +1743,7 @@ void pf_rm_rule(struct
pf_rulequeue
struct pf_rule *);
struct pf_divert *pf_find_divert(struct mbuf *);
int pf_setup_pdesc(sa_family_t, int,
- struct pf_pdesc *, struct mbuf *,
+ struct pf_pdesc *, struct mbuf **,
u_short *, u_short *, struct pfi_kif *,
struct pf_rule **, struct pf_rule **,
struct pf_ruleset **, int *, int *);
@@ -1777,12 +1777,9 @@ int pf_match_gid(u_int8_t, gid_t, gid_t,
int pf_refragment6(struct mbuf **, struct m_tag *mtag, int);
void pf_normalize_init(void);
-int pf_normalize_ip(struct mbuf **, int, struct pfi_kif *, u_short *,
- struct pf_pdesc *);
-int pf_normalize_ip6(struct mbuf **, int, struct pfi_kif *, u_short *,
- struct pf_pdesc *);
-int pf_normalize_tcp(int, struct pfi_kif *, struct mbuf *, int, int, void *,
- struct pf_pdesc *);
+int pf_normalize_ip(struct mbuf **, int, u_short *, struct pf_pdesc *);
+int pf_normalize_ip6(struct mbuf **, int, u_short *, struct pf_pdesc *);
+int pf_normalize_tcp(int, struct mbuf *, int, struct pf_pdesc *);
void pf_normalize_tcp_cleanup(struct pf_state *);
int pf_normalize_tcp_init(struct mbuf *, int, struct pf_pdesc *,
struct tcphdr *, struct pf_state_peer *, struct pf_state_peer *);
