Date: Wed, 25 Jun 2025 20:04:44 GMT From: Kristof Provost <kp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: a7d631f69d3f - main - pfctl: fix use-after-free and memory leak in pfctl_optimzie.c Message-ID: <202506252004.55PK4igF080704@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=a7d631f69d3fa993c701d681850d42a750886298 commit a7d631f69d3fa993c701d681850d42a750886298 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2025-06-17 13:55:12 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2025-06-25 20:04:15 +0000 pfctl: fix use-after-free and memory leak in pfctl_optimzie.c OK bluhm@ Obtained from: OpenBSD, sashan <sashan@openbsd.org>, 43d70b8338 Sponsored by: Rubicon Communications, LLC ("Netgate") --- sbin/pfctl/pfctl_optimize.c | 64 ++++++++++++++++++++++++++------------------- sbin/pfctl/pfctl_parser.h | 1 + 2 files changed, 38 insertions(+), 27 deletions(-) diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c index 530f1c241661..d6417e8e73a1 100644 --- a/sbin/pfctl/pfctl_optimize.c +++ b/sbin/pfctl/pfctl_optimize.c @@ -241,6 +241,8 @@ int skip_cmp_src_addr(struct pfctl_rule *, struct pfctl_rule *); int skip_cmp_src_port(struct pfctl_rule *, struct pfctl_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 *); static int (*skip_comparitors[PF_SKIP_COUNT])(struct pfctl_rule *, @@ -345,9 +347,11 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pfctl_ruleset *rs) TAILQ_INSERT_TAIL( rs->rules[PF_RULESET_FILTER].active.ptr, r, entries); + pf_opt_table_unref(por->por_src_tbl); + pf_opt_table_unref(por->por_dst_tbl); free(por); } - free(block); + superblock_free(pf, block); } return (0); @@ -355,16 +359,8 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pfctl_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))) { @@ -537,12 +533,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)) 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 && @@ -559,12 +557,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)) 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); } } } @@ -1249,6 +1249,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); @@ -1657,20 +1658,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) @@ -1678,3 +1667,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 718c05b306b2..45d9ebc45bc9 100644 --- a/sbin/pfctl/pfctl_parser.h +++ b/sbin/pfctl/pfctl_parser.h @@ -258,6 +258,7 @@ struct pf_opt_tbl { char pt_name[PF_TABLE_NAME_SIZE]; int pt_rulecount; int pt_generated; + uint32_t pt_refcnt; struct node_tinithead pt_nodes; struct pfr_buffer *pt_buf; };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202506252004.55PK4igF080704>