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 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.747 diff -u -p -r1.747 pf.c --- net/pf.c 2 Jun 2011 22:08:40 -0000 1.747 +++ net/pf.c 10 Jun 2011 08:22:36 -0000 @@ -5466,10 +5466,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"); @@ -5479,13 +5481,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; @@ -5500,9 +5512,41 @@ 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_DROP) + 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 has been reassembled, 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: { @@ -5551,7 +5595,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 has been reassembled, 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; @@ -5756,7 +5832,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; @@ -5788,13 +5863,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); @@ -5806,40 +5874,22 @@ 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) { + m = *m0; if (action != PF_PASS) 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, @@ -5921,6 +5971,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; @@ -5953,7 +6004,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 */ @@ -6037,7 +6088,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; @@ -6070,13 +6120,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); @@ -6088,44 +6131,22 @@ 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); - 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 (pf_setup_pdesc(AF_INET6, dir, &pd, m0, &action, &reason, kif, &a, + &r, &ruleset, &off, &hdrlen) == -1) { + m = *m0; 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, @@ -6239,7 +6260,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 *);