Hello,
On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
> On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > > > - if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
> > > > - pf_state_key_detach(s, PF_SK_STACK);
> > > > - pf_state_key_detach(s, PF_SK_WIRE);
> > >
> > > This bug is not yours, but doing two pf_state_key_detach is wrong
> > > and results in all kinds of protection fault fireworks. The right
> > > thing is to do pf_detach_state that would handle the case where
> > > PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
> > good catch.
> >
> > >
> > > In csfailed case we do pf_remove_src_node and then call
> > > pf_src_tree_remove_state. With your diff this means that
> > > pf_src_tree_remove_state will be dereferencing sni->sn that
> > > might be pool_put by the pf_remove_src_node. Therefore their
> > > order needs to be reversed.
> >
> > I see. Another option to fix it would be to do:
> >
> > sni->sn = sns[i];
> > sns[i] = NULL;
> >
>
> I'm not sure I follow. Where do you want to do it?
> If it's when we pool_get sni's, then this would mean
> we won't run pf_remove_src_node and cleanup source
> nodes that might have been created.
>
sorry I was too brief. The snippet below comes from
pf_create_state() with your patch applied:
3560 for (i = 0; i < PF_SN_MAX; i++)
3561 if (sns[i] != NULL) {
3562 struct pf_sn_item *sni;
3563
3564 sni = pool_get(&pf_sn_item_pl, PR_NOWAIT);
3565 if (sni == NULL) {
3566 REASON_SET(&reason, PFRES_MEMORY);
3567 goto csfailed;
3568 }
3569 sni->sn = sns[i];
3570 sns[i] = NULL;
3571 SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
3572 sni->sn->states++;
3573 }
3574
the point of my suggestion is to transfer ownership of source node from
local variable sns[] to state. so the check performed later in csfailed
at line 3617, will find NULL pointer in sns[i] field and won't attempt
to call pf_remove_src_node().
3610 csfailed:
3611 if (s) {
3612 pf_normalize_tcp_cleanup(s); /* safe even w/o init */
3613 pf_src_tree_remove_state(s);
3614 }
3615
3616 for (i = 0; i < PF_SN_MAX; i++)
3617 if (sns[i] != NULL)
3618 pf_remove_src_node(sns[i]);
3619
your patch updated by my suggestion is further below.
regards
sasha
? pf.c.diff
? pf.c.patch
Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.916
diff -u -r1.916 pf.c
--- pf.c 26 May 2015 16:17:51 -0000 1.916
+++ pf.c 27 May 2015 22:59:32 -0000
@@ -3557,16 +3557,6 @@
goto csfailed;
}
- if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
- *sks = *skw = NULL;
- REASON_SET(&reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
- /* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i < PF_SN_MAX; i++)
if (sns[i] != NULL) {
struct pf_sn_item *sni;
@@ -3574,16 +3564,22 @@
sni = pool_get(&pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(&reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(&pf_state_pl, s);
- return (PF_DROP);
+ goto csfailed;
}
sni->sn = sns[i];
+ sns[i] = NULL;
SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
sni->sn->states++;
}
+ if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
+ pf_detach_state(s);
+ *sks = *skw = NULL;
+ REASON_SET(&reason, PFRES_STATEINS);
+ goto csfailed;
+ } else
+ *sm = s;
+
pf_set_rt_ifp(s, pd->src); /* needs s->state_key set */
if (tag > 0) {
pf_tag_ref(tag);
@@ -3612,12 +3608,16 @@
return (PF_PASS);
csfailed:
+ if (s) {
+ pf_normalize_tcp_cleanup(s); /* safe even w/o init */
+ pf_src_tree_remove_state(s);
+ }
+
for (i = 0; i < PF_SN_MAX; i++)
if (sns[i] != NULL)
pf_remove_src_node(sns[i]);
+
if (s) {
- pf_normalize_tcp_cleanup(s); /* safe even w/o init */
- pf_src_tree_remove_state(s);
STATE_DEC_COUNTERS(s);
pool_put(&pf_state_pl, s);
}
