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 *);