Hello,
It seems to me PF might leak rule items when pf_create_state() fails to create
state for matching packet.
The scenario is as follows:
packet matches couple of 'match' rules before it hits a 'pass' rule
match rules are kept in `rules` single list, which is a local variable
of pf_test_rule()
3238 if (r->action == PF_MATCH) {
3239 if ((ri = pool_get(&pf_rule_item_pl,
3240 PR_NOWAIT)) == NULL) {
3241 REASON_SET(&reason, PFRES_MEMORY);
3242 goto cleanup;
3243 }
3244 ri->r = r;
3245 /* order is irrelevant */
3246 SLIST_INSERT_HEAD(&rules, ri, entry);
3247 pf_rule_to_actions(r, &act);
as soon as pf_test_rule() is done with rules it proceeds to state creation,
we are passing the `rules` as arg9
3373
3374 action = pf_create_state(pd, r, a, nr, &skw, &sks,
&rewrite,
3375 sm, tag, &rules, &act, sns);
3376
3377 if (action != PF_PASS)
3378 return (action);
3379 if (sks != skw) {
note we are doing return at line 3378, when pf_create_state() fails to
create
state and does not return PF_PASS. pf_create_state() assumes `rules` are
either
bound to state or released by pf_create_state().
So let's see what happens in pf_create_state():
3451 s = pool_get(&pf_state_pl, PR_NOWAIT | PR_ZERO);
3452 if (s == NULL) {
3453 REASON_SET(&reason, PFRES_MEMORY);
3454 goto csfailed;
3455 }
3456 s->rule.ptr = r;
3457 s->anchor.ptr = a;
3458 s->natrule.ptr = nr;
3459 memcpy(&s->match_rules, rules, sizeof(s->match_rules));
if we fail to allocate state we jump right to csfailed returning PF_DROP
3614 csfailed:
3615 if (s) {
3616 pf_normalize_tcp_cleanup(s); /* safe even w/o init */
3617 pf_src_tree_remove_state(s);
3618 pool_put(&pf_state_pl, s);
3619 }
3620
3621 for (i = 0; i < PF_SN_MAX; i++)
3622 if (sns[i] != NULL)
3623 pf_remove_src_node(sns[i]);
3624
3625 return (PF_DROP);
without releasing `rules` (list of pf_rule_item)
The easiest way to fix it is to make sure pf_test_rule() releases `rules`
whenever
pf_create_state() fails. Patches (-p, -u) are attached.
regards
sasha
? create_state.diff
? meleak.diff-p
? meleak.diff-u
? pbr-ipv6-reass.diff-p
? pbr-ipv6-reass.diff-u
? pf.c.diff
? pf.c.patch
? sa_family_t.diff
? sa_family_t.diff-p
Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.924
diff -p -r1.924 pf.c
*** pf.c 16 Jul 2015 21:14:21 -0000 1.924
--- pf.c 16 Jul 2015 22:56:17 -0000
*************** pf_test_rule(struct pf_pdesc *pd, struct
*** 3055,3060 ****
--- 3055,3061 ----
int state_icmp = 0, icmp_dir = 0;
u_int16_t virtual_type, virtual_id;
u_int8_t icmptype = 0, icmpcode = 0;
+ int action = PF_DROP;
bzero(&act, sizeof(act));
bzero(sns, sizeof(sns));
*************** pf_test_rule(struct pf_pdesc *pd, struct
*** 3330,3336 ****
if (pd->virtual_proto != PF_VPROTO_FRAGMENT
&& !state_icmp && r->keep_state) {
- int action;
if (r->rule_flag & PFRULE_SRCTRACK &&
pf_insert_src_node(&sns[PF_SN_NONE], r, PF_SN_NONE, pd->af,
--- 3331,3336 ----
*************** pf_test_rule(struct pf_pdesc *pd, struct
*** 3349,3355 ****
sm, tag, &rules, &act, sns);
if (action != PF_PASS)
! return (action);
if (sks != skw) {
struct pf_state_key *sk;
--- 3349,3355 ----
sm, tag, &rules, &act, sns);
if (action != PF_PASS)
! goto cleanup;
if (sks != skw) {
struct pf_state_key *sk;
*************** cleanup:
*** 3407,3413 ****
pool_put(&pf_rule_item_pl, ri);
}
! return (PF_DROP);
}
static __inline int
--- 3407,3413 ----
pool_put(&pf_rule_item_pl, ri);
}
! return (action);
}
static __inline int
*************** pf_create_state(struct pf_pdesc *pd, str
*** 3430,3436 ****
s->rule.ptr = r;
s->anchor.ptr = a;
s->natrule.ptr = nr;
- memcpy(&s->match_rules, rules, sizeof(s->match_rules));
if (r->allow_opts)
s->state_flags |= PFSTATE_ALLOWOPTS;
if (r->rule_flag & PFRULE_STATESLOPPY)
--- 3430,3435 ----
*************** pf_create_state(struct pf_pdesc *pd, str
*** 3581,3586 ****
--- 3580,3587 ----
REASON_SET(&reason, PFRES_SYNPROXY);
return (PF_SYNPROXY_DROP);
}
+ memcpy(&s->match_rules, rules, sizeof(s->match_rules));
+ bzero(rules, sizeof(*rules));
return (PF_PASS);
? create_state.diff
? meleak.diff-p
? meleak.diff-u
? pbr-ipv6-reass.diff-p
? pbr-ipv6-reass.diff-u
? pf.c.diff
? pf.c.patch
? sa_family_t.diff
? sa_family_t.diff-p
Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.924
diff -u -r1.924 pf.c
--- pf.c 16 Jul 2015 21:14:21 -0000 1.924
+++ pf.c 16 Jul 2015 22:56:10 -0000
@@ -3055,6 +3055,7 @@
int state_icmp = 0, icmp_dir = 0;
u_int16_t virtual_type, virtual_id;
u_int8_t icmptype = 0, icmpcode = 0;
+ int action = PF_DROP;
bzero(&act, sizeof(act));
bzero(sns, sizeof(sns));
@@ -3330,7 +3331,6 @@
if (pd->virtual_proto != PF_VPROTO_FRAGMENT
&& !state_icmp && r->keep_state) {
- int action;
if (r->rule_flag & PFRULE_SRCTRACK &&
pf_insert_src_node(&sns[PF_SN_NONE], r, PF_SN_NONE, pd->af,
@@ -3349,7 +3349,7 @@
sm, tag, &rules, &act, sns);
if (action != PF_PASS)
- return (action);
+ goto cleanup;
if (sks != skw) {
struct pf_state_key *sk;
@@ -3407,7 +3407,7 @@
pool_put(&pf_rule_item_pl, ri);
}
- return (PF_DROP);
+ return (action);
}
static __inline int
@@ -3430,7 +3430,6 @@
s->rule.ptr = r;
s->anchor.ptr = a;
s->natrule.ptr = nr;
- memcpy(&s->match_rules, rules, sizeof(s->match_rules));
if (r->allow_opts)
s->state_flags |= PFSTATE_ALLOWOPTS;
if (r->rule_flag & PFRULE_STATESLOPPY)
@@ -3581,6 +3580,8 @@
REASON_SET(&reason, PFRES_SYNPROXY);
return (PF_SYNPROXY_DROP);
}
+ memcpy(&s->match_rules, rules, sizeof(s->match_rules));
+ bzero(rules, sizeof(*rules));
return (PF_PASS);
