git: 773e3a71b2f1 - main - pf: Initialize pf_kpool mutexes earlier

From: Mark Johnston <markj_at_FreeBSD.org>
Date: Mon, 31 Jan 2022 21:14:49 UTC
The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=773e3a71b2f11d422694495aca988d4c7143601b

commit 773e3a71b2f11d422694495aca988d4c7143601b
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2022-01-31 21:14:00 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-01-31 21:14:00 +0000

    pf: Initialize pf_kpool mutexes earlier
    
    There are some error paths in ioctl handlers that will call
    pf_krule_free() before the rule's rpool.mtx field is initialized,
    causing a panic with INVARIANTS enabled.
    
    Fix the problem by introducing pf_krule_alloc() and initializing the
    mutex there.  This does mean that the rule->krule and pool->kpool
    conversion functions need to stop zeroing the input structure, but I
    don't see a nicer way to handle this except perhaps by guarding the
    mtx_destroy() with a mtx_initialized() check.
    
    Constify some related functions while here and add a regression test
    based on a syzkaller reproducer.
    
    Reported by:    syzbot+77cd12872691d219c158@syzkaller.appspotmail.com
    Reviewed by:    kp
    MFC after:      1 week
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D34115
---
 sys/net/pfvar.h                         |  5 +++--
 sys/netpfil/pf/pf_ioctl.c               | 34 ++++++++++++++++-----------------
 sys/netpfil/pf/pf_nv.c                  |  2 --
 tests/sys/netpfil/pf/ioctl/validation.c | 27 ++++++++++++++++++++++++++
 4 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 7794b8f849fe..ba1f0a2fd9b3 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -169,7 +169,7 @@ pf_counter_u64_periodic(struct pf_counter_u64 *pfcu64)
 }
 
 static inline u_int64_t
-pf_counter_u64_fetch(struct pf_counter_u64 *pfcu64)
+pf_counter_u64_fetch(const struct pf_counter_u64 *pfcu64)
 {
 	struct pf_counter_u64_pcpu *pcpu;
 	u_int64_t sum;
@@ -263,7 +263,7 @@ pf_counter_u64_add(struct pf_counter_u64 *pfcu64, uint32_t n)
 }
 
 static inline u_int64_t
-pf_counter_u64_fetch(struct pf_counter_u64 *pfcu64)
+pf_counter_u64_fetch(const struct pf_counter_u64 *pfcu64)
 {
 
 	return (counter_u64_fetch(pfcu64->counter));
@@ -2155,6 +2155,7 @@ struct pf_kruleset	*pf_find_kruleset(const char *);
 struct pf_kruleset	*pf_find_or_create_kruleset(const char *);
 void			 pf_rs_initialize(void);
 
+struct pf_krule		*pf_krule_alloc(void);
 void			 pf_krule_free(struct pf_krule *);
 #endif
 
diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
index d66b13f61b79..5bc78ab60e74 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -1512,6 +1512,16 @@ pf_altq_get_nth_active(u_int32_t n)
 }
 #endif /* ALTQ */
 
+struct pf_krule *
+pf_krule_alloc(void)
+{
+	struct pf_krule *rule;
+
+	rule = malloc(sizeof(struct pf_krule), M_PFRULE, M_WAITOK | M_ZERO);
+	mtx_init(&rule->rpool.mtx, "pf_krule_pool", NULL, MTX_DEF);
+	return (rule);
+}
+
 void
 pf_krule_free(struct pf_krule *rule)
 {
@@ -1584,14 +1594,12 @@ pf_kpool_to_pool(const struct pf_kpool *kpool, struct pf_pool *pool)
 	pool->opts = kpool->opts;
 }
 
-static int
+static void
 pf_pool_to_kpool(const struct pf_pool *pool, struct pf_kpool *kpool)
 {
 	_Static_assert(sizeof(pool->key) == sizeof(kpool->key), "");
 	_Static_assert(sizeof(pool->counter) == sizeof(kpool->counter), "");
 
-	bzero(kpool, sizeof(*kpool));
-
 	bcopy(&pool->key, &kpool->key, sizeof(kpool->key));
 	bcopy(&pool->counter, &kpool->counter, sizeof(kpool->counter));
 
@@ -1599,12 +1607,10 @@ pf_pool_to_kpool(const struct pf_pool *pool, struct pf_kpool *kpool)
 	kpool->proxy_port[0] = pool->proxy_port[0];
 	kpool->proxy_port[1] = pool->proxy_port[1];
 	kpool->opts = pool->opts;
-
-	return (0);
 }
 
 static void
-pf_krule_to_rule(struct pf_krule *krule, struct pf_rule *rule)
+pf_krule_to_rule(const struct pf_krule *krule, struct pf_rule *rule)
 {
 
 	bzero(rule, sizeof(*rule));
@@ -1727,8 +1733,6 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
 	if (ret != 0)
 		return (ret);
 
-	bzero(krule, sizeof(*krule));
-
 	bcopy(&rule->src, &krule->src, sizeof(rule->src));
 	bcopy(&rule->dst, &krule->dst, sizeof(rule->dst));
 
@@ -1757,9 +1761,7 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
 	if (ret != 0)
 		return (ret);
 
-	ret = pf_pool_to_kpool(&rule->rpool, &krule->rpool);
-	if (ret != 0)
-		return (ret);
+	pf_pool_to_kpool(&rule->rpool, &krule->rpool);
 
 	/* Don't allow userspace to set evaulations, packets or bytes. */
 	/* kif, anchor, overload_tbl are not copied over. */
@@ -1862,8 +1864,6 @@ pf_ioctl_addrule(struct pf_krule *rule, uint32_t ticket,
 	int			 rs_num;
 	int			 error = 0;
 
-	mtx_init(&rule->rpool.mtx, "pf_krule_pool", NULL, MTX_DEF);
-
 	if ((rule->return_icmp >> 8) > ICMP_MAXTYPE) {
 		error = EINVAL;
 		goto errout_unlocked;
@@ -2357,7 +2357,7 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
 		if (! nvlist_exists_nvlist(nvl, "rule"))
 			ERROUT(EINVAL);
 
-		rule = malloc(sizeof(*rule), M_PFRULE, M_WAITOK | M_ZERO);
+		rule = pf_krule_alloc();
 		error = pf_nvrule_to_krule(nvlist_get_nvlist(nvl, "rule"),
 		    rule);
 		if (error)
@@ -2390,10 +2390,10 @@ DIOCADDRULENV_error:
 		struct pfioc_rule	*pr = (struct pfioc_rule *)addr;
 		struct pf_krule		*rule;
 
-		rule = malloc(sizeof(*rule), M_PFRULE, M_WAITOK | M_ZERO);
+		rule = pf_krule_alloc();
 		error = pf_rule_to_krule(&pr->rule, rule);
 		if (error != 0) {
-			free(rule, M_PFRULE);
+			pf_krule_free(rule);
 			break;
 		}
 
@@ -2647,7 +2647,7 @@ DIOCGETRULENV_error:
 		}
 
 		if (pcr->action != PF_CHANGE_REMOVE) {
-			newrule = malloc(sizeof(*newrule), M_PFRULE, M_WAITOK | M_ZERO);
+			newrule = pf_krule_alloc();
 			error = pf_rule_to_krule(&pcr->rule, newrule);
 			if (error != 0) {
 				free(newrule, M_PFRULE);
diff --git a/sys/netpfil/pf/pf_nv.c b/sys/netpfil/pf/pf_nv.c
index 69ef09c704c7..b4b844c4edd5 100644
--- a/sys/netpfil/pf/pf_nv.c
+++ b/sys/netpfil/pf/pf_nv.c
@@ -223,8 +223,6 @@ pf_nvpool_to_pool(const nvlist_t *nvl, struct pf_kpool *kpool)
 {
 	int error = 0;
 
-	bzero(kpool, sizeof(*kpool));
-
 	PFNV_CHK(pf_nvbinary(nvl, "key", &kpool->key, sizeof(kpool->key)));
 
 	if (nvlist_exists_nvlist(nvl, "counter")) {
diff --git a/tests/sys/netpfil/pf/ioctl/validation.c b/tests/sys/netpfil/pf/ioctl/validation.c
index 947193599b31..80fa4773c8de 100644
--- a/tests/sys/netpfil/pf/ioctl/validation.c
+++ b/tests/sys/netpfil/pf/ioctl/validation.c
@@ -869,6 +869,32 @@ ATF_TC_CLEANUP(rpool_mtx, tc)
 	COMMON_CLEANUP();
 }
 
+ATF_TC_WITH_CLEANUP(rpool_mtx2);
+ATF_TC_HEAD(rpool_mtx2, tc)
+{
+	atf_tc_set_md_var(tc, "require.user", "root");
+}
+
+ATF_TC_BODY(rpool_mtx2, tc)
+{
+	struct pfioc_rule rule;
+
+	COMMON_HEAD();
+
+	memset(&rule, 0, sizeof(rule));
+
+	rule.pool_ticket = 1000000;
+	rule.action = PF_CHANGE_ADD_HEAD;
+	rule.rule.af = AF_INET;
+
+	ioctl(dev, DIOCCHANGERULE, &rule);
+}
+
+ATF_TC_CLEANUP(rpool_mtx2, tc)
+{
+	COMMON_CLEANUP();
+}
+
 
 ATF_TP_ADD_TCS(tp)
 {
@@ -893,6 +919,7 @@ ATF_TP_ADD_TCS(tp)
 	ATF_TP_ADD_TC(tp, getsrcnodes);
 	ATF_TP_ADD_TC(tp, tag);
 	ATF_TP_ADD_TC(tp, rpool_mtx);
+	ATF_TP_ADD_TC(tp, rpool_mtx2);
 
 	return (atf_no_error());
 }