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 *);
intskip_cmp_src_port(struct pf_rule *, struct pf_rule *);
intsuperblock_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);
-