git: a41b6be8fcfb - stable/12 - pf: Initialize pf_kpool mutexes earlier
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 04 Mar 2022 22:12:01 UTC
The branch stable/12 has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=a41b6be8fcfbae49f0a950c75b36b5d1ca47ee46
commit a41b6be8fcfbae49f0a950c75b36b5d1ca47ee46
Author: Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2022-01-31 21:14:00 +0000
Commit: Mark Johnston <markj@FreeBSD.org>
CommitDate: 2022-03-04 21:36:47 +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
Sponsored by: The FreeBSD Foundation
(cherry picked from commit 773e3a71b2f11d422694495aca988d4c7143601b)
---
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 469b7250b896..22ba2623a6c4 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -167,7 +167,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;
@@ -261,7 +261,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));
@@ -2145,6 +2145,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 4774d88be65c..5636ea93e99d 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -1508,6 +1508,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)
{
@@ -1577,14 +1587,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));
@@ -1592,12 +1600,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));
@@ -1720,8 +1726,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));
@@ -1735,9 +1739,7 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule)
strlcpy(krule->overload_tblname, rule->overload_tblname,
sizeof(rule->overload_tblname));
- 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. */
@@ -1970,8 +1972,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;
@@ -2336,7 +2336,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)
@@ -2369,10 +2369,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;
}
@@ -2622,7 +2622,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 3fda6831754d..e2af55af86e5 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());
}