On Thu, Nov 22, 2012 at 16:05 +0100, Mike Belopuhov wrote:
> re pf bug on bugs@:
>
> apparently the crash is caused by the stack corruption that happens
> in pf_map_addr as it expects to get an array of struct pf_src_node
> pointers, not just one pointer. the bug was introduced about four
> years ago, but somehow (stack layout?) went unnoticed.
>
> the proper fix is to pass an empty array of size PF_SN_MAX as done
> in other places.
>
> i've verified that this fixes the issue for me and arjan is going
> to verify as well.
>
> while here, i think it makes sense to initialize src_nodes list
> head and while it's essentially a noop, if we convert it to
> something else where it matters, proper initialization might get
> lost.
>
ok, two more places were found by florian@ in pf_route{,6}.
unfortunately i can't reproduce the crash there even by
assigning sn to something like 0xcf000000 or 0xdeadbeef
(on i386). as far as i can see it should be triggered by
the route-to with "no state" (as state pointer should be
NULL).
but in any case, we should treat it the same way.
OK?
diff --git sys/net/pf.c sys/net/pf.c
index 5177466..66837ca 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3204,15 +3204,16 @@ void
pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr)
{
struct pf_rule *r = s->rule.ptr;
- struct pf_src_node *sn = NULL;
+ struct pf_src_node *sns[PF_SN_MAX];
s->rt_kif = NULL;
if (!r->rt)
return;
+ bzero(sns, sizeof(sns));
switch (s->key[PF_SK_WIRE]->af) {
#ifdef INET
case AF_INET:
- pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, &sn,
+ pf_map_addr(AF_INET, r, saddr, &s->rt_addr, NULL, sns,
&r->route, PF_SN_ROUTE);
s->rt_kif = r->route.kif;
s->natrule.ptr = r;
@@ -3220,7 +3221,7 @@ pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr)
#endif /* INET */
#ifdef INET6
case AF_INET6:
- pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, &sn,
+ pf_map_addr(AF_INET6, r, saddr, &s->rt_addr, NULL, sns,
&r->route, PF_SN_ROUTE);
s->rt_kif = r->route.kif;
s->natrule.ptr = r;
@@ -3710,6 +3711,8 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
#endif
s->set_prio[0] = act->set_prio[0];
s->set_prio[1] = act->set_prio[1];
+ SLIST_INIT(&s->src_nodes);
+
switch (pd->proto) {
case IPPROTO_TCP:
s->src.seqlo = ntohl(th->th_seq);
@@ -5844,7 +5847,7 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir,
struct ifnet *oifp,
struct ip *ip;
struct ifnet *ifp = NULL;
struct pf_addr naddr;
- struct pf_src_node *sn = NULL;
+ struct pf_src_node *sns[PF_SN_MAX];
int error = 0;
#ifdef IPSEC
struct m_tag *mtag;
@@ -5901,9 +5904,10 @@ pf_route(struct mbuf **m, struct pf_rule *r, int dir,
struct ifnet *oifp,
m0->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
} else {
if (s == NULL) {
+ bzero(sns, sizeof(sns));
if (pf_map_addr(AF_INET, r,
(struct pf_addr *)&ip->ip_src,
- &naddr, NULL, &sn, &r->route, PF_SN_ROUTE)) {
+ &naddr, NULL, sns, &r->route, PF_SN_ROUTE)) {
DPFPRINTF(LOG_ERR,
"pf_route: pf_map_addr() failed.");
goto bad;
@@ -6027,7 +6031,7 @@ pf_route6(struct mbuf **m, struct pf_rule *r, int dir,
struct ifnet *oifp,
struct ip6_hdr *ip6;
struct ifnet *ifp = NULL;
struct pf_addr naddr;
- struct pf_src_node *sn = NULL;
+ struct pf_src_node *sns[PF_SN_MAX];
if (m == NULL || *m == NULL || r == NULL ||
(dir != PF_IN && dir != PF_OUT) || oifp == NULL)
@@ -6069,8 +6073,9 @@ pf_route6(struct mbuf **m, struct pf_rule *r, int dir,
struct ifnet *oifp,
}
if (s == NULL) {
+ bzero(sns, sizeof(sns));
if (pf_map_addr(AF_INET6, r, (struct pf_addr *)&ip6->ip6_src,
- &naddr, NULL, &sn, &r->route, PF_SN_ROUTE)) {
+ &naddr, NULL, sns, &r->route, PF_SN_ROUTE)) {
DPFPRINTF(LOG_ERR,
"pf_route6: pf_map_addr() failed.");
goto bad;
