- patching use-after-free and innocent memory leak in pfctl_optimzie.c

OK bluhm@

2 files changed, 40 insertions, 31 deletions

diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c
index f6f8b259531..15e26d7002e 100644
--- a/sbin/pfctl/pfctl_optimize.c
+++ b/sbin/pfctl/pfctl_optimize.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_optimize.c,v 1.37 2017/11/25 22:16:47 sashan Exp $ */
+/*	$OpenBSD: pfctl_optimize.c,v 1.38 2017/11/25 22:20:06 sashan Exp $ */
 
 /*
  * Copyright (c) 2004 Mike Frantzen <frantzen@openbsd.org>
@@ -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,11 @@ 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);
+			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);
@@ -325,16 +329,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 +498,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 +521,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 +1216,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 +1616,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 +1625,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..08539b1f9ed 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_parser.h,v 1.106 2017/08/11 22:30:38 benno Exp $ */
+/*	$OpenBSD: pfctl_parser.h,v 1.107 2017/11/25 22:20:06 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -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;
 };