From nobody Fri Mar 04 22:12:01 2022 X-Original-To: dev-commits-src-branches@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 654D619E51A7; Fri, 4 Mar 2022 22:12:01 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4K9MXK2SZfz3kvy; Fri, 4 Mar 2022 22:12:01 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1646431921; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=vKoHWWFcviuuf9cYZjuI1161FDs+qlysr8HLpLKax5A=; b=NCHd0yKsVxZqHKzw3TF1xDxw9LgIhM1G33AT0JC1SAnPeEKFfDe6idX4T20e+VNjQ0tqrD II3sGWQNHWNPkyp4oMTMZkGOd7hKu/PJTFv/DpHnfcFvl85Zg3TaXCCsCFAneCkXY33xKr 4ZlL0s0mFAc37nh7HO2Siq+1xhy6sR7dbdWEzxkgNmZ93ZMmFgKz+ECS1tzbkszNr/XxQ9 KTHxcs4YVIg+dShHGWxOz4nbn/Wf+Avf0TdMC1Zd/0CFJiQUZD5uKir4CD2aGnPwYBhUvM 1kBWD6r0lT/ZsSQ34zEVlcpImoJGluR4dOYrLOVMnMrJ7wIcPPZjqWQ1eja1Ng== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 32EC414997; Fri, 4 Mar 2022 22:12:01 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 224MC1TW071112; Fri, 4 Mar 2022 22:12:01 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 224MC12L071111; Fri, 4 Mar 2022 22:12:01 GMT (envelope-from git) Date: Fri, 4 Mar 2022 22:12:01 GMT Message-Id: <202203042212.224MC12L071111@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: a41b6be8fcfb - stable/12 - pf: Initialize pf_kpool mutexes earlier List-Id: Commits to the stable branches of the FreeBSD src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-branches List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-branches@freebsd.org X-BeenThere: dev-commits-src-branches@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/stable/12 X-Git-Reftype: branch X-Git-Commit: a41b6be8fcfbae49f0a950c75b36b5d1ca47ee46 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1646431921; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=vKoHWWFcviuuf9cYZjuI1161FDs+qlysr8HLpLKax5A=; b=RyYCuqZcbfRv+EKSeltR2Ph1Zi7DKUofJ6myVvs0N8Fb+rrz+9kKw72uZiTZroBuVHqHK2 bRtldT8jsX5E0KceRRV0PmS2gy08/WFKuEOWScDKFbmRwHgDNAuUliiZgHnpicP7q9AluM bb/EDORu+oEqCdJmXxNyG6McsBjEHtcTbMgD0oUxCpJk5d+SsRT+UXHw7Kwuh/G7p+A8eY oxXkF07A0qaiPtm89GWk3mvlbAiHAXBLV4SrAz3saqpmIq9avonHXGCUt1zl53WBcLGFgj A1PgRJoS1tNCMReATuKM2GccwCzneNnrcKbGrwCBsQ1lN/fFsSNlpDEILuH6Tg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1646431921; a=rsa-sha256; cv=none; b=LYSJcuc0hirkLEJMp1ChqT3LL+2djoKvYdnPCxsbGvKdIWzPNYudqOiU5NL0reV5/x8150 FDRRVPPxvm/xNt8r9WNFIr+szJovIdMe2sMwRg5qecp1VqJTf3JsG8mzhGV1imtZkvUcDE hNzOfrft1Ghz1MGwak29bu/5RLyvemRALEGS+IWCJzOO+kbiGJE2isPeP544E8fQuVzF9Z pdlpNpeMfMpqFvE9oMVeDFo09REeIrHoJiI4W6hAtQRzFymL67lC05POQEAgz/tSn7OKaC GJiaY4WHYBIZiZSsIfQLzg7DaXCGpuRCyjkcyJvGA78E03Aps1JXf868Os2tlw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch stable/12 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=a41b6be8fcfbae49f0a950c75b36b5d1ca47ee46 commit a41b6be8fcfbae49f0a950c75b36b5d1ca47ee46 Author: Mark Johnston AuthorDate: 2022-01-31 21:14:00 +0000 Commit: Mark Johnston 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()); }