git: fe9e4eb6f38a - main - pf: fix use of uninitialised variable
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 03 Feb 2026 22:51:54 UTC
The branch main has been updated by kp:
URL: https://cgit.FreeBSD.org/src/commit/?id=fe9e4eb6f38ae004efb576bf44aded08852f9e6b
commit fe9e4eb6f38ae004efb576bf44aded08852f9e6b
Author: Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2026-02-03 12:17:08 +0000
Commit: Kristof Provost <kp@FreeBSD.org>
CommitDate: 2026-02-03 22:51:28 +0000
pf: fix use of uninitialised variable
In pf_match_rule() we attempt to append matching rules to the end of
'match_rules'. We want to preserve the order to make the multiple
pflog entries easier to understand. So we keep track of the last added
rule item in 'rt'. However, that assumed that 'match_rules' was only
ever added to in that one call to pf_match_rules(). This isn't always
the case, for example if we have match rules in different anchors.
In that case we'd end up using the uninitialised 'rt' variable in the
SLIST_INSERT_AFTER call.
Instead track the match rules and the last matching rule (to enable
easy appending) in the struct pf_test_ctx.
This also allows us to reduce the number of arguments for some
functions, because we passed a ctx to most functions that needed
'match_rules'.
While here also make pf_match_rules() static, because it's only ever
used in pf.c
Add a test case to exercise the relevant code path.
MFC after: 2 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")
---
sys/net/pfvar.h | 7 +++---
sys/netpfil/pf/pf.c | 41 ++++++++++++++----------------
tests/sys/netpfil/pf/match.sh | 58 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 80 insertions(+), 26 deletions(-)
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index eb17c4ff5ef0..68616a7de5e4 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1442,6 +1442,8 @@ struct pf_test_ctx {
int limiter_drop;
u_short reason;
struct pf_src_node *sns[PF_SN_MAX];
+ struct pf_krule_slist *match_rules;
+ struct pf_krule_item *last_match_rule;
struct pf_krule *nr;
struct pf_krule *tr;
struct pf_krule **rm;
@@ -3128,10 +3130,7 @@ int pf_osfp_match(struct pf_osfp_enlist *, pf_osfp_t);
#ifdef _KERNEL
void pf_print_host(struct pf_addr *, u_int16_t, sa_family_t);
-enum pf_test_status pf_step_into_anchor(struct pf_test_ctx *, struct pf_krule *,
- struct pf_krule_slist *match_rules);
-enum pf_test_status pf_match_rule(struct pf_test_ctx *, struct pf_kruleset *,
- struct pf_krule_slist *);
+enum pf_test_status pf_step_into_anchor(struct pf_test_ctx *, struct pf_krule *);
void pf_step_into_keth_anchor(struct pf_keth_anchor_stackframe *,
int *, struct pf_keth_ruleset **,
struct pf_keth_rule **, struct pf_keth_rule **,
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index 3ccf1344fd7d..b7c79437584e 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -342,14 +342,14 @@ static int pf_dummynet_route(struct pf_pdesc *,
struct ifnet *, const struct sockaddr *, struct mbuf **);
static int pf_test_eth_rule(int, struct pfi_kkif *,
struct mbuf **);
+static enum pf_test_status pf_match_rule(struct pf_test_ctx *, struct pf_kruleset *);
static int pf_test_rule(struct pf_krule **, struct pf_kstate **,
struct pf_pdesc *, struct pf_krule **,
struct pf_kruleset **, u_short *, struct inpcb *,
struct pf_krule_slist *);
static int pf_create_state(struct pf_krule *,
struct pf_test_ctx *,
- struct pf_kstate **, u_int16_t, u_int16_t,
- struct pf_krule_slist *match_rules);
+ struct pf_kstate **, u_int16_t, u_int16_t);
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 *,
@@ -5108,8 +5108,7 @@ pf_tag_packet(struct pf_pdesc *pd, int tag)
} while (0)
enum pf_test_status
-pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r,
- struct pf_krule_slist *match_rules)
+pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r)
{
enum pf_test_status rv;
@@ -5127,7 +5126,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r,
struct pf_kanchor *child;
rv = PF_TEST_OK;
RB_FOREACH(child, pf_kanchor_node, &r->anchor->children) {
- rv = pf_match_rule(ctx, &child->ruleset, match_rules);
+ rv = pf_match_rule(ctx, &child->ruleset);
if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
/*
* we either hit a rule with quick action
@@ -5138,7 +5137,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r,
}
}
} else {
- rv = pf_match_rule(ctx, &r->anchor->ruleset, match_rules);
+ rv = pf_match_rule(ctx, &r->anchor->ruleset);
/*
* Unless errors occured, stop iff any rule matched
* within quick anchors.
@@ -5987,10 +5986,9 @@ pf_rule_apply_nat(struct pf_test_ctx *ctx, struct pf_krule *r)
}
enum pf_test_status
-pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset,
- struct pf_krule_slist *match_rules)
+pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset)
{
- struct pf_krule_item *ri, *rt;
+ struct pf_krule_item *ri;
struct pf_krule *r;
struct pf_krule *save_a;
struct pf_kruleset *save_aruleset;
@@ -6300,12 +6298,12 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset,
}
ri->r = r;
- if (SLIST_EMPTY(match_rules)) {
- SLIST_INSERT_HEAD(match_rules, ri, entry);
+ if (SLIST_EMPTY(ctx->match_rules)) {
+ SLIST_INSERT_HEAD(ctx->match_rules, ri, entry);
} else {
- SLIST_INSERT_AFTER(rt, ri, entry);
+ SLIST_INSERT_AFTER(ctx->last_match_rule, ri, entry);
}
- rt = ri;
+ ctx->last_match_rule = ri;
pf_rule_to_actions(r, &pd->act);
if (r->log)
@@ -6337,7 +6335,7 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset,
ctx->source = sr;
}
if (pd->act.log & PF_LOG_MATCHES)
- pf_log_matches(pd, r, ctx->a, ruleset, match_rules);
+ pf_log_matches(pd, r, ctx->a, ruleset, ctx->match_rules);
if (r->quick) {
ctx->test_status = PF_TEST_QUICK;
break;
@@ -6354,7 +6352,7 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset,
* Note: we don't need to restore if we are not going
* to continue with ruleset evaluation.
*/
- if (pf_step_into_anchor(ctx, r, match_rules) != PF_TEST_OK) {
+ if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) {
break;
}
ctx->a = save_a;
@@ -6391,6 +6389,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
ctx.rsm = rsm;
ctx.th = &pd->hdr.tcp;
ctx.reason = *reason;
+ ctx.match_rules = match_rules;
pf_addrcpy(&pd->nsaddr, pd->src, pd->af);
pf_addrcpy(&pd->ndaddr, pd->dst, pd->af);
@@ -6488,7 +6487,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
ruleset = *ctx.rsm;
} else {
ruleset = &pf_main_ruleset;
- rv = pf_match_rule(&ctx, ruleset, match_rules);
+ rv = pf_match_rule(&ctx, ruleset);
if (rv == PF_TEST_FAIL || ctx.limiter_drop == 1) {
REASON_SET(reason, ctx.reason);
goto cleanup;
@@ -6523,7 +6522,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
PFLOG_PACKET(r->action, ctx.reason, r, ctx.a, ruleset, pd, 1, NULL);
}
if (pd->act.log & PF_LOG_MATCHES)
- pf_log_matches(pd, r, ctx.a, ruleset, match_rules);
+ pf_log_matches(pd, r, ctx.a, ruleset, ctx.match_rules);
if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&
(r->action == PF_DROP) &&
((r->rule_flag & PFRULE_RETURNRST) ||
@@ -6568,8 +6567,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
(pd->flags & PFDESC_TCP_NORM)))) {
bool nat64;
- action = pf_create_state(r, &ctx, sm, bproto_sum, bip_sum,
- match_rules);
+ action = pf_create_state(r, &ctx, sm, bproto_sum, bip_sum);
ctx.sk = ctx.nk = NULL;
if (action != PF_PASS) {
pf_udp_mapping_release(ctx.udp_mapping);
@@ -6659,8 +6657,7 @@ cleanup:
static int
pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx,
- struct pf_kstate **sm, u_int16_t bproto_sum, u_int16_t bip_sum,
- struct pf_krule_slist *match_rules)
+ struct pf_kstate **sm, u_int16_t bproto_sum, u_int16_t bip_sum)
{
struct pf_pdesc *pd = ctx->pd;
struct pf_kstate *s = NULL;
@@ -6729,7 +6726,7 @@ pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx,
s->rule = r;
s->nat_rule = ctx->nr;
s->anchor = ctx->a;
- s->match_rules = *match_rules;
+ s->match_rules = *ctx->match_rules;
SLIST_INIT(&s->linkage);
memcpy(&s->act, &pd->act, sizeof(struct pf_rule_actions));
diff --git a/tests/sys/netpfil/pf/match.sh b/tests/sys/netpfil/pf/match.sh
index 58c1e021310a..992e32d9f887 100644
--- a/tests/sys/netpfil/pf/match.sh
+++ b/tests/sys/netpfil/pf/match.sh
@@ -177,9 +177,67 @@ allow_opts_cleanup()
pft_cleanup
}
+atf_test_case "double_match" "cleanup"
+double_match_head()
+{
+ atf_set descr 'Test two match statements in separate anchors'
+ atf_set require.user root
+}
+
+double_match_body()
+{
+ pft_init
+
+ epair_one=$(vnet_mkepair)
+ epair_two=$(vnet_mkepair)
+ vnet_mkjail rtr ${epair_one}a ${epair_two}a
+ vnet_mkjail srv ${epair_two}b
+
+ ifconfig ${epair_one}b 192.0.2.2/24 up
+
+ jexec rtr ifconfig ${epair_one}a 192.0.2.1/24 up
+ jexec rtr ifconfig ${epair_two}a 198.51.100.1/24 up
+ jexec rtr sysctl net.inet.ip.forwarding=1
+
+ jexec srv ifconfig ${epair_two}b 198.51.100.2/24 up
+
+ route add default 192.0.2.1
+
+ # Sanity check
+ atf_check -s exit:0 -o ignore \
+ ping -c 1 192.0.2.1
+
+ jexec rtr pfctl -e
+ pft_set_rules rtr \
+ "nat on ${epair_two}a from 192.0.2.0/24 to any -> (${epair_two}a)" \
+ "block all" \
+ "anchor \"userrules\" all {\n \
+ anchor \"one\" all { \n\
+ match in tag \"allow\"\n\
+ }\n\
+ anchor \"two\" all { \n\
+ match tag \"allow\"\n\
+ }\n\
+ }\n" \
+ "pass quick tagged \"allow\""
+
+ atf_check -s exit:0 -o ignore \
+ ping -c 1 198.51.100.2
+
+ jexec rtr pfctl -ss -vv
+ jexec rtr pfctl -sr -vv -a "*"
+ jexec rtr pfctl -sr -a "*"
+}
+
+double_match_cleanup()
+{
+ pft_cleanup
+}
+
atf_init_test_cases()
{
atf_add_test_case "dummynet"
atf_add_test_case "quick"
atf_add_test_case "allow_opts"
+ atf_add_test_case "double_match"
}