From 6c51f7d89f3d76b4f8d49e9640ce1287fd1f6ce0 Mon Sep 17 00:00:00 2001 From: sashan Date: Tue, 11 Jan 2022 09:00:17 +0000 Subject: [PATCH] move allocations in DIOCSADDRULE and DIOCHANGERULE outside of locks. this diff lets pf_rule_copyin() to be called outside of PF_LOCK()/NET_LOCK(). OK bluhm@ --- sys/net/pf_ioctl.c | 300 ++++++++++++++++++++++++++++----------------- 1 file changed, 186 insertions(+), 114 deletions(-) diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c index a7e6641ae91..3766d749491 100644 --- a/sys/net/pf_ioctl.c +++ b/sys/net/pf_ioctl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pf_ioctl.c,v 1.369 2021/12/26 14:04:29 sashan Exp $ */ +/* $OpenBSD: pf_ioctl.c,v 1.370 2022/01/11 09:00:17 sashan Exp $ */ /* * Copyright (c) 2001 Daniel Hartmeier @@ -103,13 +103,12 @@ void pf_hash_rule_addr(MD5_CTX *, struct pf_rule_addr *); int pf_commit_rules(u_int32_t, char *); int pf_addr_setup(struct pf_ruleset *, struct pf_addr_wrap *, sa_family_t); -int pf_kif_setup(char *, struct pfi_kif **); +struct pfi_kif *pf_kif_setup(struct pfi_kif *); void pf_addr_copyout(struct pf_addr_wrap *); void pf_trans_set_commit(void); void pf_pool_copyin(struct pf_pool *, struct pf_pool *); int pf_validate_range(u_int8_t, u_int16_t[2]); -int pf_rule_copyin(struct pf_rule *, struct pf_rule *, - struct pf_ruleset *); +int pf_rule_copyin(struct pf_rule *, struct pf_rule *); u_int16_t pf_qname2qid(char *, int); void pf_qid2qname(u_int16_t, char *); void pf_qid_unref(u_int16_t); @@ -268,6 +267,21 @@ pfclose(dev_t dev, int flags, int fmt, struct proc *p) return (0); } +void +pf_rule_free(struct pf_rule *rule) +{ + if (rule == NULL) + return; + + pfi_kif_free(rule->kif); + pfi_kif_free(rule->rcv_kif); + pfi_kif_free(rule->rdr.kif); + pfi_kif_free(rule->nat.kif); + pfi_kif_free(rule->route.kif); + + pool_put(&pf_rule_pl, rule); +} + void pf_rm_rule(struct pf_rulequeue *rulequeue, struct pf_rule *rule) { @@ -880,19 +894,22 @@ pf_addr_setup(struct pf_ruleset *ruleset, struct pf_addr_wrap *addr, return (0); } -int -pf_kif_setup(char *ifname, struct pfi_kif **kif) +struct pfi_kif * +pf_kif_setup(struct pfi_kif *kif_buf) { - if (ifname[0]) { - *kif = pfi_kif_get(ifname, NULL); - if (*kif == NULL) - return (EINVAL); + struct pfi_kif *kif; - pfi_kif_ref(*kif, PFI_KIF_REF_RULE); - } else - *kif = NULL; + if (kif_buf == NULL) + return (NULL); - return (0); + KASSERT(kif_buf->pfik_name[0] != '\0'); + + kif = pfi_kif_get(kif_buf->pfik_name, &kif_buf); + if (kif_buf != NULL) + pfi_kif_free(kif_buf); + pfi_kif_ref(kif, PFI_KIF_REF_RULE); + + return (kif); } void @@ -1318,41 +1335,18 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) break; } - NET_LOCK(); - PF_LOCK(); - pr->anchor[sizeof(pr->anchor) - 1] = '\0'; - ruleset = pf_find_ruleset(pr->anchor); - if (ruleset == NULL) { - error = EINVAL; - PF_UNLOCK(); - NET_UNLOCK(); - pool_put(&pf_rule_pl, rule); + if ((error = pf_rule_copyin(&pr->rule, rule))) { + pf_rule_free(rule); + rule = NULL; break; } + if (pr->rule.return_icmp >> 8 > ICMP_MAXTYPE) { error = EINVAL; - PF_UNLOCK(); - NET_UNLOCK(); - pool_put(&pf_rule_pl, rule); - break; - } - if (pr->ticket != ruleset->rules.inactive.ticket) { - error = EBUSY; - PF_UNLOCK(); - NET_UNLOCK(); - pool_put(&pf_rule_pl, rule); - break; - } - if ((error = pf_rule_copyin(&pr->rule, rule, ruleset))) { - pf_rm_rule(NULL, rule); + pf_rule_free(rule); rule = NULL; - PF_UNLOCK(); - NET_UNLOCK(); break; } - rule->cuid = p->p_ucred->cr_ruid; - rule->cpid = p->p_p->ps_pid; - switch (rule->af) { case 0: break; @@ -1363,13 +1357,57 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) break; #endif /* INET6 */ default: - pf_rm_rule(NULL, rule); + pf_rule_free(rule); rule = NULL; error = EAFNOSUPPORT; + goto fail; + } + + if (rule->src.addr.type == PF_ADDR_NONE || + rule->dst.addr.type == PF_ADDR_NONE) { + error = EINVAL; + pf_rule_free(rule); + rule = NULL; + break; + } + + if (rule->rt && !rule->direction) { + error = EINVAL; + pf_rule_free(rule); + rule = NULL; + break; + } + + if (rule->scrub_flags & PFSTATE_SETPRIO && + (rule->set_prio[0] > IFQ_MAXPRIO || + rule->set_prio[1] > IFQ_MAXPRIO)) { + error = EINVAL; + pf_rule_free(rule); + rule = NULL; + break; + } + + NET_LOCK(); + PF_LOCK(); + pr->anchor[sizeof(pr->anchor) - 1] = '\0'; + ruleset = pf_find_ruleset(pr->anchor); + if (ruleset == NULL) { + error = EINVAL; PF_UNLOCK(); NET_UNLOCK(); - goto fail; + pf_rule_free(rule); + break; + } + if (pr->ticket != ruleset->rules.inactive.ticket) { + error = EBUSY; + PF_UNLOCK(); + NET_UNLOCK(); + pf_rule_free(rule); + break; } + rule->cuid = p->p_ucred->cr_ruid; + rule->cpid = p->p_p->ps_pid; + tail = TAILQ_LAST(ruleset->rules.inactive.ptr, pf_rulequeue); if (tail) @@ -1377,9 +1415,19 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) else rule->nr = 0; - if (rule->src.addr.type == PF_ADDR_NONE || - rule->dst.addr.type == PF_ADDR_NONE) - error = EINVAL; + rule->kif = pf_kif_setup(rule->kif); + rule->rcv_kif = pf_kif_setup(rule->rcv_kif); + rule->rdr.kif = pf_kif_setup(rule->rdr.kif); + rule->nat.kif = pf_kif_setup(rule->nat.kif); + rule->route.kif = pf_kif_setup(rule->route.kif); + + if (rule->overload_tblname[0]) { + if ((rule->overload_tbl = pfr_attach_table(ruleset, + rule->overload_tblname, 0)) == NULL) + error = EINVAL; + else + rule->overload_tbl->pfrkt_flags |= PFR_TFLAG_ACTIVE; + } if (pf_addr_setup(ruleset, &rule->src.addr, rule->af)) error = EINVAL; @@ -1393,12 +1441,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) error = EINVAL; if (pf_anchor_setup(rule, ruleset, pr->anchor_call)) error = EINVAL; - if (rule->rt && !rule->direction) - error = EINVAL; - if (rule->scrub_flags & PFSTATE_SETPRIO && - (rule->set_prio[0] > IFQ_MAXPRIO || - rule->set_prio[1] > IFQ_MAXPRIO)) - error = EINVAL; if (error) { pf_rm_rule(NULL, rule); @@ -1525,51 +1567,40 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) break; } - newrule = pool_get(&pf_rule_pl, PR_WAITOK|PR_LIMITFAIL|PR_ZERO); - if (newrule == NULL) { - error = ENOMEM; - break; - } + if (pcr->action == PF_CHANGE_GET_TICKET) { + NET_LOCK(); + PF_LOCK(); + + ruleset = pf_find_ruleset(pcr->anchor); + if (ruleset == NULL) + error = EINVAL; + else + pcr->ticket = ++ruleset->rules.active.ticket; - NET_LOCK(); - PF_LOCK(); - ruleset = pf_find_ruleset(pcr->anchor); - if (ruleset == NULL) { - error = EINVAL; PF_UNLOCK(); NET_UNLOCK(); - pool_put(&pf_rule_pl, newrule); break; } - if (pcr->action == PF_CHANGE_GET_TICKET) { - pcr->ticket = ++ruleset->rules.active.ticket; - PF_UNLOCK(); - NET_UNLOCK(); - pool_put(&pf_rule_pl, newrule); - break; - } else { - if (pcr->ticket != - ruleset->rules.active.ticket) { - error = EINVAL; - PF_UNLOCK(); - NET_UNLOCK(); - pool_put(&pf_rule_pl, newrule); + if (pcr->action != PF_CHANGE_REMOVE) { + newrule = pool_get(&pf_rule_pl, + PR_WAITOK|PR_LIMITFAIL|PR_ZERO); + if (newrule == NULL) { + error = ENOMEM; break; } + if (pcr->rule.return_icmp >> 8 > ICMP_MAXTYPE) { error = EINVAL; - PF_UNLOCK(); - NET_UNLOCK(); pool_put(&pf_rule_pl, newrule); break; } - } - - if (pcr->action != PF_CHANGE_REMOVE) { - pf_rule_copyin(&pcr->rule, newrule, ruleset); - newrule->cuid = p->p_ucred->cr_ruid; - newrule->cpid = p->p_p->ps_pid; + error = pf_rule_copyin(&pcr->rule, newrule); + if (error != 0) { + pf_rule_free(newrule); + newrule = NULL; + break; + } switch (newrule->af) { case 0: @@ -1581,24 +1612,74 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) break; #endif /* INET6 */ default: - pf_rm_rule(NULL, newrule); error = EAFNOSUPPORT; - PF_UNLOCK(); - NET_UNLOCK(); + pf_rule_free(newrule); + newrule = NULL; goto fail; } - if (newrule->rt && !newrule->direction) + if (newrule->rt && !newrule->direction) { + pf_rule_free(newrule); error = EINVAL; - if (pf_addr_setup(ruleset, &newrule->src.addr, newrule->af)) + newrule = NULL; + break; + } + } + + NET_LOCK(); + PF_LOCK(); + ruleset = pf_find_ruleset(pcr->anchor); + if (ruleset == NULL) { + error = EINVAL; + PF_UNLOCK(); + NET_UNLOCK(); + pf_rule_free(newrule); + break; + } + + if (pcr->ticket != ruleset->rules.active.ticket) { + error = EINVAL; + PF_UNLOCK(); + NET_UNLOCK(); + pf_rule_free(newrule); + break; + } + + if (pcr->action != PF_CHANGE_REMOVE) { + KASSERT(newrule != NULL); + newrule->cuid = p->p_ucred->cr_ruid; + newrule->cpid = p->p_p->ps_pid; + + newrule->kif = pf_kif_setup(newrule->kif); + newrule->rcv_kif = pf_kif_setup(newrule->rcv_kif); + newrule->rdr.kif = pf_kif_setup(newrule->rdr.kif); + newrule->nat.kif = pf_kif_setup(newrule->nat.kif); + newrule->route.kif = pf_kif_setup(newrule->route.kif); + + if (newrule->overload_tblname[0]) { + newrule->overload_tbl = pfr_attach_table( + ruleset, newrule->overload_tblname, 0); + if (newrule->overload_tbl == NULL) + error = EINVAL; + else + newrule->overload_tbl->pfrkt_flags |= + PFR_TFLAG_ACTIVE; + } + + if (pf_addr_setup(ruleset, &newrule->src.addr, + newrule->af)) error = EINVAL; - if (pf_addr_setup(ruleset, &newrule->dst.addr, newrule->af)) + if (pf_addr_setup(ruleset, &newrule->dst.addr, + newrule->af)) error = EINVAL; - if (pf_addr_setup(ruleset, &newrule->rdr.addr, newrule->af)) + if (pf_addr_setup(ruleset, &newrule->rdr.addr, + newrule->af)) error = EINVAL; - if (pf_addr_setup(ruleset, &newrule->nat.addr, newrule->af)) + if (pf_addr_setup(ruleset, &newrule->nat.addr, + newrule->af)) error = EINVAL; - if (pf_addr_setup(ruleset, &newrule->route.addr, newrule->af)) + if (pf_addr_setup(ruleset, &newrule->route.addr, + newrule->af)) error = EINVAL; if (pf_anchor_setup(newrule, ruleset, pcr->anchor_call)) error = EINVAL; @@ -3006,8 +3087,7 @@ pf_validate_range(u_int8_t op, u_int16_t port[2]) } int -pf_rule_copyin(struct pf_rule *from, struct pf_rule *to, - struct pf_ruleset *ruleset) +pf_rule_copyin(struct pf_rule *from, struct pf_rule *to) { int i; @@ -3041,24 +3121,16 @@ pf_rule_copyin(struct pf_rule *from, struct pf_rule *to, if (pf_validate_range(to->rdr.port_op, to->rdr.proxy_port)) return (EINVAL); - if (pf_kif_setup(to->ifname, &to->kif)) - return (EINVAL); - if (pf_kif_setup(to->rcv_ifname, &to->rcv_kif)) - return (EINVAL); - if (to->overload_tblname[0]) { - if ((to->overload_tbl = pfr_attach_table(ruleset, - to->overload_tblname, 0)) == NULL) - return (EINVAL); - else - to->overload_tbl->pfrkt_flags |= PFR_TFLAG_ACTIVE; - } - - if (pf_kif_setup(to->rdr.ifname, &to->rdr.kif)) - return (EINVAL); - if (pf_kif_setup(to->nat.ifname, &to->nat.kif)) - return (EINVAL); - if (pf_kif_setup(to->route.ifname, &to->route.kif)) - return (EINVAL); + to->kif = (to->ifname[0]) ? + pfi_kif_alloc(to->ifname, M_WAITOK) : NULL; + to->rcv_kif = (to->rcv_ifname[0]) ? + pfi_kif_alloc(to->rcv_ifname, M_WAITOK) : NULL; + to->rdr.kif = (to->rdr.ifname[0]) ? + pfi_kif_alloc(to->rdr.ifname, M_WAITOK) : NULL; + to->nat.kif = (to->nat.ifname[0]) ? + pfi_kif_alloc(to->nat.ifname, M_WAITOK) : NULL; + to->route.kif = (to->route.ifname[0]) ? + pfi_kif_alloc(to->route.ifname, M_WAITOK) : NULL; to->os_fingerprint = from->os_fingerprint; -- 2.20.1