Hi, Hrvoje managed to crash the kernel in pf fragment reassembly.
> r620-1# pfctl -e > pf enabled > r620-1# pfctl -f /etc/pf.conf > uvm_fault(0xffffffff824b9278, 0xb7, 0, 2) -> e > kernel: page fault trap, code=0 > Stopped at pf_free_fragment+0x77: movq %rax,0xb8(%rcx) > TID PID UID PRFLAGS PFLAGS CPU COMMAND > 350110 79921 0 0x100003 0 2K tcpbench > *301306 98239 0 0x14000 0x200 4 softnet > 55791 2358 0 0x14000 0x200 3 softnet > 176238 13130 0 0x14000 0x200 1 softnet > 66977 54316 0 0x14000 0x200 5 systq > 165986 42679 0 0x14000 0x40000200 0 softclock > pf_free_fragment(fffffd83a5022010) at pf_free_fragment+0x77 > pf_create_fragment(ffff800022d717ce) at pf_create_fragment+0xc8 > pf_reassemble6(ffff800022d71708,ffff800022d71648,30,0,1,ffff800022d717ce) at > pf_reassemble6+0x51 > pf_normalize_ip6(ffff800022d716c8,ffff800022d717ce) at pf_normalize_ip6+0x8a > pf_test(18,1,ffff800000095048,ffff800022d71978) at pf_test+0x30d > ip6_input_if(ffff800022d71978,ffff800022d71984,29,0,ffff800000095048) at > ip6_input_if+0x1ae > ipv6_input(ffff800000095048,fffffd80a3f1dc00) at ipv6_input+0x39 > ether_input(ffff800000095048,fffffd80a3f1dc00) at ether_input+0x3b1 > if_input_process(ffff800000095048,ffff800022d71a68) at if_input_process+0x6f > ifiq_process(ffff800000099900) at ifiq_process+0x69 > taskq_thread(ffff800000032200) at taskq_thread+0x11a > end trace frame: 0x0, count: 4 > https://www.openbsd.org/ddb.html describes the minimum info required in > bug reports. Insufficient info makes it difficult to find and fix bugs. > ddb{4}> It crashes here in pf_free_fragment() 266 TAILQ_REMOVE(&pf_fragqueue, frag, frag_next); Putting a pf frag lock into pf_create_fragment() around pf_flush_fragments() does not look sufficient. The pf_nfrents++ also needs protection. So I moved the lock around pf_reassemble(). ok? bluhm Index: net/pf_norm.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v retrieving revision 1.223 diff -u -p -r1.223 pf_norm.c --- net/pf_norm.c 10 Mar 2021 10:21:48 -0000 1.223 +++ net/pf_norm.c 22 Aug 2022 18:35:40 -0000 @@ -801,12 +801,9 @@ pf_reassemble(struct mbuf **m0, int dir, key.fn_proto = ip->ip_p; key.fn_direction = dir; - PF_FRAG_LOCK(); if ((frag = pf_fillup_fragment(&key, ip->ip_id, frent, reason)) - == NULL) { - PF_FRAG_UNLOCK(); + == NULL) return (PF_DROP); - } /* The mbuf is part of the fragment entry, no direct free or access */ m = *m0 = NULL; @@ -814,7 +811,6 @@ pf_reassemble(struct mbuf **m0, int dir, if (frag->fr_holes) { DPFPRINTF(LOG_DEBUG, "frag %d, holes %d", frag->fr_id, frag->fr_holes); - PF_FRAG_UNLOCK(); return (PF_PASS); /* drop because *m0 is NULL, no error */ } @@ -833,7 +829,6 @@ pf_reassemble(struct mbuf **m0, int dir, ip->ip_off &= ~(IP_MF|IP_OFFMASK); if (hdrlen + total > IP_MAXPACKET) { - PF_FRAG_UNLOCK(); DPFPRINTF(LOG_NOTICE, "drop: too big: %d", total); ip->ip_len = 0; REASON_SET(reason, PFRES_SHORT); @@ -841,7 +836,6 @@ pf_reassemble(struct mbuf **m0, int dir, return (PF_DROP); } - PF_FRAG_UNLOCK(); DPFPRINTF(LOG_INFO, "complete: %p(%d)", m, ntohs(ip->ip_len)); return (PF_PASS); } @@ -880,12 +874,9 @@ pf_reassemble6(struct mbuf **m0, struct key.fn_proto = 0; key.fn_direction = dir; - PF_FRAG_LOCK(); if ((frag = pf_fillup_fragment(&key, fraghdr->ip6f_ident, frent, - reason)) == NULL) { - PF_FRAG_UNLOCK(); + reason)) == NULL) return (PF_DROP); - } /* The mbuf is part of the fragment entry, no direct free or access */ m = *m0 = NULL; @@ -893,7 +884,6 @@ pf_reassemble6(struct mbuf **m0, struct if (frag->fr_holes) { DPFPRINTF(LOG_DEBUG, "frag %#08x, holes %d", frag->fr_id, frag->fr_holes); - PF_FRAG_UNLOCK(); return (PF_PASS); /* drop because *m0 is NULL, no error */ } @@ -943,20 +933,17 @@ pf_reassemble6(struct mbuf **m0, struct ip6->ip6_nxt = proto; if (hdrlen - sizeof(struct ip6_hdr) + total > IPV6_MAXPACKET) { - PF_FRAG_UNLOCK(); DPFPRINTF(LOG_NOTICE, "drop: too big: %d", total); ip6->ip6_plen = 0; REASON_SET(reason, PFRES_SHORT); /* PF_DROP requires a valid mbuf *m0 in pf_test6() */ return (PF_DROP); } - PF_FRAG_UNLOCK(); DPFPRINTF(LOG_INFO, "complete: %p(%d)", m, ntohs(ip6->ip6_plen)); return (PF_PASS); fail: - PF_FRAG_UNLOCK(); REASON_SET(reason, PFRES_MEMORY); /* PF_DROP requires a valid mbuf *m0 in pf_test6(), will free later */ return (PF_DROP); @@ -1060,8 +1047,12 @@ pf_normalize_ip(struct pf_pdesc *pd, u_s return (PF_PASS); /* no reassembly */ /* Returns PF_DROP or m is NULL or completely reassembled mbuf */ - if (pf_reassemble(&pd->m, pd->dir, reason) != PF_PASS) + PF_FRAG_LOCK(); + if (pf_reassemble(&pd->m, pd->dir, reason) != PF_PASS) { + PF_FRAG_UNLOCK(); return (PF_DROP); + } + PF_FRAG_UNLOCK(); if (pd->m == NULL) return (PF_PASS); /* packet has been reassembled, no error */ @@ -1092,9 +1083,13 @@ pf_normalize_ip6(struct pf_pdesc *pd, u_ return (PF_PASS); /* no reassembly */ /* Returns PF_DROP or m is NULL or completely reassembled mbuf */ + PF_FRAG_LOCK(); if (pf_reassemble6(&pd->m, &frag, pd->fragoff + sizeof(frag), - pd->extoff, pd->dir, reason) != PF_PASS) + pd->extoff, pd->dir, reason) != PF_PASS) { + PF_FRAG_UNLOCK(); return (PF_DROP); + } + PF_FRAG_UNLOCK(); if (pd->m == NULL) return (PF_PASS); /* packet has been reassembled, no error */
