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;
};
