From 94a2fe4c6b2f5fd4dfc07e816c13c9e34289aade Mon Sep 17 00:00:00 2001 From: Kristof Provost Date: Mon, 10 Jan 2022 18:08:18 +0100 Subject: [PATCH] pf: protect the rpool from races The roundrobin pool stores its state in the rule, which could potentially lead to invalid addresses being returned. For example, thread A just executed PF_AINC(&rpool->counter) and immediately afterwards thread B executes PF_ACPY(naddr, &rpool->counter) (i.e. after the pf_match_addr() check of rpool->counter). Lock the rpool with its own mutex to prevent these races. The performance impact of this is expected to be low, as each rule has its own lock, and the lock is also only relevant when state is being created (so only for the initial packets of a connection, not for all traffic). --- sys/net/pfvar.h | 1 + sys/netpfil/pf/pf_ioctl.c | 5 +++++ sys/netpfil/pf/pf_lb.c | 25 +++++++++++++++++++------ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 933bb8bff4d5..319e22efd674 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -547,6 +547,7 @@ struct pf_kpooladdr { TAILQ_HEAD(pf_kpalist, pf_kpooladdr); struct pf_kpool { + struct mtx mtx; struct pf_kpalist list; struct pf_kpooladdr *cur; struct pf_poolhashkey key; diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 9fcfcecb8879..c4bf2ec84e5f 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -1538,6 +1538,9 @@ pf_krule_free(struct pf_krule *rule) counter_u64_free(rule->states_cur); counter_u64_free(rule->states_tot); counter_u64_free(rule->src_nodes); + + + mtx_destroy(&rule->rpool.mtx); free(rule, M_PFRULE); } @@ -2113,6 +2116,8 @@ pf_ioctl_addrule(struct pf_krule *rule, uint32_t ticket, TAILQ_INSERT_TAIL(ruleset->rules[rs_num].inactive.ptr, rule, entries); ruleset->rules[rs_num].inactive.rcount++; + + mtx_init(&rule->rpool.mtx, "pf_krule_pool", NULL, MTX_DEF); PF_RULES_WUNLOCK(); return (0); diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c index 24ff907cdc61..06f2029b9c7d 100644 --- a/sys/netpfil/pf/pf_lb.c +++ b/sys/netpfil/pf/pf_lb.c @@ -370,36 +370,45 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, return (0); } + mtx_lock(&rpool->mtx); /* Find the route using chosen algorithm. Store the found route in src_node if it was given or found. */ - if (rpool->cur->addr.type == PF_ADDR_NOROUTE) + if (rpool->cur->addr.type == PF_ADDR_NOROUTE) { + mtx_unlock(&rpool->mtx); return (1); + } if (rpool->cur->addr.type == PF_ADDR_DYNIFTL) { switch (af) { #ifdef INET case AF_INET: if (rpool->cur->addr.p.dyn->pfid_acnt4 < 1 && (rpool->opts & PF_POOL_TYPEMASK) != - PF_POOL_ROUNDROBIN) + PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); - raddr = &rpool->cur->addr.p.dyn->pfid_addr4; - rmask = &rpool->cur->addr.p.dyn->pfid_mask4; + } + raddr = &rpool->cur->addr.p.dyn->pfid_addr4; + rmask = &rpool->cur->addr.p.dyn->pfid_mask4; break; #endif /* INET */ #ifdef INET6 case AF_INET6: if (rpool->cur->addr.p.dyn->pfid_acnt6 < 1 && (rpool->opts & PF_POOL_TYPEMASK) != - PF_POOL_ROUNDROBIN) + PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); + } raddr = &rpool->cur->addr.p.dyn->pfid_addr6; rmask = &rpool->cur->addr.p.dyn->pfid_mask6; break; #endif /* INET6 */ } } else if (rpool->cur->addr.type == PF_ADDR_TABLE) { - if ((rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_ROUNDROBIN) + if ((rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); /* unsupported */ + } } else { raddr = &rpool->cur->addr.v.a.addr; rmask = &rpool->cur->addr.v.a.mask; @@ -507,6 +516,7 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, /* table contains no address of type 'af' */ if (rpool->cur != acur) goto try_next; + mtx_unlock(&rpool->mtx); return (1); } } else if (rpool->cur->addr.type == PF_ADDR_DYNIFTL) { @@ -516,6 +526,7 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, /* table contains no address of type 'af' */ if (rpool->cur != acur) goto try_next; + mtx_unlock(&rpool->mtx); return (1); } } else { @@ -535,6 +546,8 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, if (*sn != NULL) PF_ACPY(&(*sn)->raddr, naddr, af); + mtx_unlock(&rpool->mtx); + if (V_pf_status.debug >= PF_DEBUG_MISC && (rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) { printf("pf_map_addr: selected address "); -- 2.34.1