From nobody Fri Apr 18 15:12:18 2025 X-Original-To: dev-commits-src-all@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 4ZfJCR3QNnz5sxKL; Fri, 18 Apr 2025 15:12:19 +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 "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4ZfJCR0zHZz3nF9; Fri, 18 Apr 2025 15:12:19 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1744989139; 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=WFSzmkbqYb7rjZW8T1e7M7WauOp3Yt1WR+0TgajFetI=; b=ojQAG8fUArDhnDFYDXRSpktlbw6nwqM45UM7tAo/5JQEc1k31bvWdpU2+XguQ1ZH3vf1kk XnAG2u5nXBr1DIGRx8rc2In9X4hQz9K/RWeR8gLrmjLKH3kT0L0EYL+i+TwlMl2YK5Ikh1 RC25UvnhtV2Oydy7zwMHAQXEuC6AJ5jIJeVbXpUVRRniUqlLrWXXe2RDaN0kGbnkLbqE4l DJ83/e9qdpuD4W2unsz58cwBBEqK7Z4auNyd4+Y03dQeoQiW9Jp5qgGHaXLJr67PMyzNKD Cntpp3g5JSCwFV+tGpe+106mwi+yxbyvawzPlvyngrcP1EmZyh5Mwnfs4YjA+A== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1744989139; a=rsa-sha256; cv=none; b=WwP3UxOCUcIXjQMhAeBLdb9lblIRvG7oQdDA8etO1tm470GUet2YrrMhUyzH0u8ilwmXVR g+IVST5IBZDa5nziq0UyKzG2Uezf1PWKKeC/3l2RZjaoQjGFMtMsCrAxB/CxLgPaF9Lm1x oaCJ1DHv7Y5UjARhvfIzQv/J5jL8+m1vlHWzTeyoRL6juiZ+m25CGhB2zml2VlQoDoJ1Cj zk6zQUcv0q02nE0oVh077iQde6KYmlB0CZ97GnjrkkYB5xmUM1RcLhfzy6PxTZAf6wZ+J7 H0+0wFZciuZ1W4iVtLrUZxMbyMDOpwMeTo20VbzYutG5KegJcAScHq5Pekw3nA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1744989139; 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=WFSzmkbqYb7rjZW8T1e7M7WauOp3Yt1WR+0TgajFetI=; b=rYlYKZuCRvuwUaJqXo8MuLCsSfj35XaH37xs0M2eqAIjLcwoCkuZ5Q41W0qH8l5l+VWD61 w/GddGcm9u0Y3GNXPWeAGYqedFPwVHcoKPeE2ec4IqGMSGb6wwdRQ2+pAkyfdT0sd3n/+4 xDBMd5Sx8TXvSVuLTuJQ567dE7xpsSpb5lL3lb/wcwZYeN6szr6YjTyeLwG7eBFoxBmRiy crBHUh6nUVwSNtUhrOWzMHVOPn0Td00Wj9MCsf2r0k+7Yo9yzxfy+E0UQATJYjspsuOwpU 8vNzliuWUpXFV0FNmMA5JOKbXQu3c60J8NUZtw8dUxirVk41zIx6NxtFrYPRSA== 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 4ZfJCR0ZGWzfy4; Fri, 18 Apr 2025 15:12:19 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 53IFCIYg065620; Fri, 18 Apr 2025 15:12:18 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 53IFCIIF065617; Fri, 18 Apr 2025 15:12:18 GMT (envelope-from git) Date: Fri, 18 Apr 2025 15:12:18 GMT Message-Id: <202504181512.53IFCIIF065617@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: 0015baeb19eb - main - pf: Avoid logging state creation failures unless requested List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@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/main X-Git-Reftype: branch X-Git-Commit: 0015baeb19eb500130d905a162c8f16a17d85b7c Auto-Submitted: auto-generated The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=0015baeb19eb500130d905a162c8f16a17d85b7c commit 0015baeb19eb500130d905a162c8f16a17d85b7c Author: Mark Johnston AuthorDate: 2025-04-18 13:35:58 +0000 Commit: Mark Johnston CommitDate: 2025-04-18 15:11:50 +0000 pf: Avoid logging state creation failures unless requested pd.act.log is applied unconditionally, but the intent in commit 886396f1b1a7 was to log only if the rule specifically requested it. Thus, check the rule and associated NAT rule before setting PF_LOG_FORCE. For consistency with other handling of memory allocation failures, we also want to log if state creation failed for that reason. Thus, modify pf_create_state() to return the drop reason. Extend the regression test added in commit 886396f1b1a7 to check that we don't log anything if a state creation failure occurs for a rule without logging configured. Fixes: 886396f1b1a7 ("pf: Force logging if pf_create_state() fails") Reviewed by: kp MFC after: 2 weeks Sponsored by: Klara, Inc. Sponsored by: OPNsense Differential Revision: https://reviews.freebsd.org/D49352 --- sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- tests/sys/netpfil/pf/pflog.sh | 14 +++++++++++++- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 5d16af6c6d4b..88e9ef78f07c 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -348,7 +348,8 @@ static int pf_create_state(struct pf_krule *, struct pf_krule *, struct pf_krule *, struct pf_pdesc *, struct pf_state_key *, struct pf_state_key *, int *, struct pf_kstate **, int, u_int16_t, u_int16_t, - struct pf_krule_slist *, struct pf_udp_mapping *); + struct pf_krule_slist *, struct pf_udp_mapping *, + u_short *); static int pf_state_key_addr_setup(struct pf_pdesc *, struct pf_state_key_cmp *, int); static int pf_tcp_track_full(struct pf_kstate *, @@ -5972,11 +5973,13 @@ nextrule: action = pf_create_state(r, nr, a, pd, nk, sk, &rewrite, sm, tag, bproto_sum, bip_sum, - &match_rules, udp_mapping); + &match_rules, udp_mapping, reason); sk = nk = NULL; if (action != PF_PASS) { pf_udp_mapping_release(udp_mapping); - pd->act.log |= PF_LOG_FORCE; + if (r->log || (nr != NULL && nr->log) || + *reason == PFRES_MEMORY) + pd->act.log |= PF_LOG_FORCE; if (action == PF_DROP && (r->rule_flag & PFRULE_RETURN)) pf_return(r, nr, pd, th, @@ -6061,7 +6064,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, struct pf_pdesc *pd, struct pf_state_key *nk, struct pf_state_key *sk, int *rewrite, struct pf_kstate **sm, int tag, u_int16_t bproto_sum, u_int16_t bip_sum, struct pf_krule_slist *match_rules, - struct pf_udp_mapping *udp_mapping) + struct pf_udp_mapping *udp_mapping, u_short *reason) { struct pf_kstate *s = NULL; struct pf_ksrc_node *sns[PF_SN_MAX] = { NULL }; @@ -6073,7 +6076,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, struct pf_srchash *snhs[PF_SN_MAX] = { NULL }; struct tcphdr *th = &pd->hdr.tcp; u_int16_t mss = V_tcp_mssdflt; - u_short reason, sn_reason; + u_short sn_reason; struct pf_krule_item *ri; struct pf_kpool *pool_route = &r->route; @@ -6081,14 +6084,14 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, if (r->max_states && (counter_u64_fetch(r->states_cur) >= r->max_states)) { counter_u64_add(V_pf_status.lcounters[LCNT_STATES], 1); - REASON_SET(&reason, PFRES_MAXSTATES); + REASON_SET(reason, PFRES_MAXSTATES); goto csfailed; } /* src node for limits */ if ((r->rule_flag & PFRULE_SRCTRACK) && (sn_reason = pf_insert_src_node(sns, snhs, r, pd->src, pd->af, NULL, NULL, PF_SN_LIMIT)) != 0) { - REASON_SET(&reason, sn_reason); + REASON_SET(reason, sn_reason); goto csfailed; } /* src node for route-to rule */ @@ -6097,19 +6100,19 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, if ((pool_route->opts & PF_POOL_STICKYADDR) && (sn_reason = pf_insert_src_node(sns, snhs, r, pd->src, pd->af, &pd->act.rt_addr, pd->act.rt_kif, PF_SN_ROUTE)) != 0) { - REASON_SET(&reason, sn_reason); + REASON_SET(reason, sn_reason); goto csfailed; } /* src node for translation rule */ if (nr != NULL && (nr->rdr.opts & PF_POOL_STICKYADDR) && (sn_reason = pf_insert_src_node(sns, snhs, nr, &sk->addr[pd->sidx], pd->af, &nk->addr[1], NULL, PF_SN_NAT)) != 0 ) { - REASON_SET(&reason, sn_reason); + REASON_SET(reason, sn_reason); goto csfailed; } s = pf_alloc_state(M_NOWAIT); if (s == NULL) { - REASON_SET(&reason, PFRES_MEMORY); + REASON_SET(reason, PFRES_MEMORY); goto csfailed; } s->rule = r; @@ -6197,11 +6200,11 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, if (pd->proto == IPPROTO_TCP) { if (s->state_flags & PFSTATE_SCRUB_TCP && pf_normalize_tcp_init(pd, th, &s->src)) { - REASON_SET(&reason, PFRES_MEMORY); + REASON_SET(reason, PFRES_MEMORY); goto csfailed; } if (s->state_flags & PFSTATE_SCRUB_TCP && s->src.scrub && - pf_normalize_tcp_stateful(pd, &reason, th, s, + pf_normalize_tcp_stateful(pd, reason, th, s, &s->src, &s->dst, rewrite)) { /* This really shouldn't happen!!! */ DPFPRINTF(PF_DEBUG_URGENT, @@ -6236,7 +6239,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, if (pf_state_insert(BOUND_IFACE(s, pd), pd->kif, (pd->dir == PF_IN) ? sk : nk, (pd->dir == PF_IN) ? nk : sk, s)) { - REASON_SET(&reason, PFRES_STATEINS); + REASON_SET(reason, PFRES_STATEINS); goto drop; } else *sm = s; @@ -6271,7 +6274,7 @@ pf_create_state(struct pf_krule *r, struct pf_krule *nr, struct pf_krule *a, th->th_sport, s->src.seqhi, ntohl(th->th_seq) + 1, TH_SYN|TH_ACK, 0, s->src.mss, 0, M_SKIP_FIREWALL, 0, 0, pd->act.rtableid); - REASON_SET(&reason, PFRES_SYNPROXY); + REASON_SET(reason, PFRES_SYNPROXY); return (PF_SYNPROXY_DROP); } diff --git a/tests/sys/netpfil/pf/pflog.sh b/tests/sys/netpfil/pf/pflog.sh index fdd9af6316d0..a34ec893a75c 100644 --- a/tests/sys/netpfil/pf/pflog.sh +++ b/tests/sys/netpfil/pf/pflog.sh @@ -238,7 +238,7 @@ state_max_body() cat pflog.txt # Second ping is blocked due to the state limit. - atf_check -o match:".*rule 0/0\(match\): block in on ${epair}a: 192.0.2.2 > 192.0.2.1: ICMP echo request.*" \ + atf_check -o match:".*rule 0/12\(state-limit\): block in on ${epair}a: 192.0.2.2 > 192.0.2.1: ICMP echo request.*" \ cat pflog.txt # At most three lines should be written: one for the first ping, and @@ -246,6 +246,18 @@ state_max_body() # then a drop because of the state limit. Ideally only the drop would # be logged; if this is fixed, the count will be 2 instead of 3. atf_check -o match:3 grep -c . pflog.txt + + # If the rule doesn't specify logging, we shouldn't log drops + # due to state limits. + pft_set_rules alcatraz "pass inet keep state (max 1)" + + atf_check -s exit:0 -o ignore \ + ping -c 1 192.0.2.1 + + atf_check -s exit:2 -o ignore \ + ping -c 1 192.0.2.1 + + atf_check -o match:3 grep -c . pflog.txt } state_max_cleanup()