Hello, the patch below is part of larger diff [1] I've sent earlier. Leonardo seen a pfctl.core, when pfctl_optimize failed to create a radix table. The use after free happens in superblock_free() at 1621:
1618 while ((por = TAILQ_FIRST(&block->sb_rules))) { 1619 TAILQ_REMOVE(&block->sb_rules, por, por_entry); 1620 if (por->por_src_tbl) { 1621 if (por->por_src_tbl->pt_buf) { 1622 pfr_buf_clear(por->por_src_tbl->pt_buf); 1623 free(por->por_src_tbl->pt_buf); 1624 } 1625 free(por->por_src_tbl); in this case the superblock_free() is invoked from pfctl_optimize_ruleset(): 337 } 338 free(por); 339 } 340 while ((block = TAILQ_FIRST(&superblocks))) { 341 TAILQ_REMOVE(&superblocks, block, sb_entry); 342 superblock_free(pf, block); 343 } 344 return (1); 345 } 346 the buffer (por->por_src_tbl) contains pattern 0xdfdfdf, which is a clear indication we deal with freed memory. Fortunately it was not hard to discover culprit in function(combine_rules(): 526 return (1); 527 p2->por_src_tbl = p1->por_src_tbl; 528 if (p1->por_src_tbl->pt_rulecount >= 529 TABLE_THRESHOLD) { 530 TAILQ_REMOVE(&block->sb_rules, p2, 531 por_entry); 532 free(p2); 533 } we share por_src_tbl between two different instances (p1, p2) of pf_opt_rule objects. The least invasive fix I could come up with is to introduce simple reference count. One more question remains to be answered: how does it come there are no panics on OK path? (when optimizer succeeds to create table) Let's check pfctl_optimize_ruleset(): 307 rs->anchor->refcnt = 0; 308 while ((block = TAILQ_FIRST(&superblocks))) { 309 TAILQ_REMOVE(&superblocks, block, sb_entry); 310 311 while ((por = TAILQ_FIRST(&block->sb_rules))) { 312 TAILQ_REMOVE(&block->sb_rules, por, por_entry); 313 por->por_rule.nr = rs->anchor->refcnt++; 314 if ((r = calloc(1, sizeof(*r))) == NULL) 315 err(1, "calloc"); 316 memcpy(r, &por->por_rule, sizeof(*r)); 317 TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries); 318 free(por); 319 } 320 free(block); 321 } line 318 frees pf_opt_rule object, without taking care of por_*_tbl members. also line 320 is better to call superblock_free(). All those glitches are fixed in patch below. OK? thanks and regards sasha [1] https://marc.info/?l=openbsd-tech&m=151148479226177&w=2 --------8<---------------8<---------------8<------------------8<-------- diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c index a1b19781756..cea34c241de 100644 --- a/sbin/pfctl/pfctl_optimize.c +++ b/sbin/pfctl/pfctl_optimize.c @@ -238,6 +238,8 @@ int skip_cmp_src_addr(struct pf_rule *, struct pf_rule *); int skip_cmp_src_port(struct pf_rule *, struct pf_rule *); int superblock_inclusive(struct superblock *, struct pf_opt_rule *); void superblock_free(struct pfctl *, struct superblock *); +struct pf_opt_tbl *pf_opt_table_ref(struct pf_opt_tbl *); +void pf_opt_table_unref(struct pf_opt_tbl *); int (*skip_comparitors[PF_SKIP_COUNT])(struct pf_rule *, struct pf_rule *); @@ -315,9 +317,10 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pf_ruleset *rs) err(1, "calloc"); memcpy(r, &por->por_rule, sizeof(*r)); TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries); - free(por); + pf_opt_table_unref(por->por_src_tbl); + pf_opt_table_unref(por->por_dst_tbl); } - free(block); + superblock_free(pf, block); } return (0); @@ -325,16 +328,8 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pf_ruleset *rs) error: while ((por = TAILQ_FIRST(&opt_queue))) { TAILQ_REMOVE(&opt_queue, por, por_entry); - if (por->por_src_tbl) { - pfr_buf_clear(por->por_src_tbl->pt_buf); - free(por->por_src_tbl->pt_buf); - free(por->por_src_tbl); - } - if (por->por_dst_tbl) { - pfr_buf_clear(por->por_dst_tbl->pt_buf); - free(por->por_dst_tbl->pt_buf); - free(por->por_dst_tbl); - } + pf_opt_table_unref(por->por_src_tbl); + pf_opt_table_unref(por->por_dst_tbl); free(por); } while ((block = TAILQ_FIRST(&superblocks))) { @@ -502,13 +497,14 @@ combine_rules(struct pfctl *pf, struct superblock *block) if (add_opt_table(pf, &p1->por_dst_tbl, p1->por_rule.af, &p2->por_rule.dst, NULL)) return (1); - p2->por_dst_tbl = p1->por_dst_tbl; if (p1->por_dst_tbl->pt_rulecount >= TABLE_THRESHOLD) { TAILQ_REMOVE(&block->sb_rules, p2, por_entry); free(p2); - } + } else + p2->por_dst_tbl = + pf_opt_table_ref(p1->por_dst_tbl); } else if (!src_eq && dst_eq && p1->por_dst_tbl == NULL && p2->por_src_tbl == NULL && p2->por_dst_tbl == NULL && @@ -524,13 +520,14 @@ combine_rules(struct pfctl *pf, struct superblock *block) if (add_opt_table(pf, &p1->por_src_tbl, p1->por_rule.af, &p2->por_rule.src, NULL)) return (1); - p2->por_src_tbl = p1->por_src_tbl; if (p1->por_src_tbl->pt_rulecount >= TABLE_THRESHOLD) { TAILQ_REMOVE(&block->sb_rules, p2, por_entry); free(p2); - } + } else + p2->por_src_tbl = + pf_opt_table_ref(p1->por_src_tbl); } } } @@ -1218,6 +1215,7 @@ add_opt_table(struct pfctl *pf, struct pf_opt_tbl **tbl, sa_family_t af, ((*tbl)->pt_buf = calloc(1, sizeof(*(*tbl)->pt_buf))) == NULL) err(1, "calloc"); + (*tbl)->pt_refcnt = 1; (*tbl)->pt_buf->pfrb_type = PFRB_ADDRS; SIMPLEQ_INIT(&(*tbl)->pt_nodes); @@ -1617,20 +1615,8 @@ superblock_free(struct pfctl *pf, struct superblock *block) struct pf_opt_rule *por; while ((por = TAILQ_FIRST(&block->sb_rules))) { TAILQ_REMOVE(&block->sb_rules, por, por_entry); - if (por->por_src_tbl) { - if (por->por_src_tbl->pt_buf) { - pfr_buf_clear(por->por_src_tbl->pt_buf); - free(por->por_src_tbl->pt_buf); - } - free(por->por_src_tbl); - } - if (por->por_dst_tbl) { - if (por->por_dst_tbl->pt_buf) { - pfr_buf_clear(por->por_dst_tbl->pt_buf); - free(por->por_dst_tbl->pt_buf); - } - free(por->por_dst_tbl); - } + pf_opt_table_unref(por->por_src_tbl); + pf_opt_table_unref(por->por_dst_tbl); free(por); } if (block->sb_profiled_block) @@ -1638,3 +1624,24 @@ superblock_free(struct pfctl *pf, struct superblock *block) free(block); } +struct pf_opt_tbl * +pf_opt_table_ref(struct pf_opt_tbl *pt) +{ + /* parser does not run concurrently, we don't need atomic ops. */ + if (pt != NULL) + pt->pt_refcnt++; + + return (pt); +} + +void +pf_opt_table_unref(struct pf_opt_tbl *pt) +{ + if ((pt != NULL) && ((--pt->pt_refcnt) == 0)) { + if (pt->pt_buf != NULL) { + pfr_buf_clear(pt->pt_buf); + free(pt->pt_buf); + } + free(pt); + } +} diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h index 7dacc34de1c..b83e2878c8a 100644 --- a/sbin/pfctl/pfctl_parser.h +++ b/sbin/pfctl/pfctl_parser.h @@ -175,6 +175,7 @@ struct pf_opt_tbl { int pt_rulecount; int pt_generated; u_int32_t pt_flags; + u_int32_t pt_refcnt; struct node_tinithead pt_nodes; struct pfr_buffer *pt_buf; };