| Message ID | 20230608140246.15190-2-fw@strlen.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/sched: act_ipt bug fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 49 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, Jun 8, 2023 at 10:03 AM Florian Westphal <fw@strlen.de> wrote: > > Looks like "tc" hard-codes "mangle" as the only supported table > name, but on kernel side there are no checks. > > This is wrong. Not all xtables targets are safe to call from tc. > E.g. "nat" targets assume skb has a conntrack object assigned to it. > Normally those get called from netfilter nat core which consults the > nat table to obtain the address mapping. > > "tc" userspace either sets PRE or POSTROUTING as hook number, but there > is no validation of this on kernel side, so update netlink policy to > reject bogus numbers. Some targets may assume skb_dst is set for > input/forward hooks, so prevent those from being used. > > act_ipt uses the hook number in two places: > 1. the state hook number, this is fine as-is > 2. to set par.hook_mask > > The latter is a bit mask, so update the assignment to make > xt_check_target() to the right thing. > > Followup patch adds required checks for the skb/packet headers before > calling the targets evaluation function. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Florian Westphal <fw@strlen.de> > Reviewed-by: Simon Horman <simon.horman@corigine.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal > --- > v2: add Fixes tag, diff unchanged. > > net/sched/act_ipt.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c > index 5d96ffebd40f..ea7f151e7dd2 100644 > --- a/net/sched/act_ipt.c > +++ b/net/sched/act_ipt.c > @@ -48,7 +48,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t, > par.entryinfo = &e; > par.target = target; > par.targinfo = t->data; > - par.hook_mask = hook; > + par.hook_mask = 1 << hook; > par.family = NFPROTO_IPV4; > > ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false); > @@ -85,7 +85,8 @@ static void tcf_ipt_release(struct tc_action *a) > > static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = { > [TCA_IPT_TABLE] = { .type = NLA_STRING, .len = IFNAMSIZ }, > - [TCA_IPT_HOOK] = { .type = NLA_U32 }, > + [TCA_IPT_HOOK] = NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING, > + NF_INET_NUMHOOKS), > [TCA_IPT_INDEX] = { .type = NLA_U32 }, > [TCA_IPT_TARG] = { .len = sizeof(struct xt_entry_target) }, > }; > @@ -158,15 +159,27 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla, > return -EEXIST; > } > } > + > + err = -EINVAL; > hook = nla_get_u32(tb[TCA_IPT_HOOK]); > + switch (hook) { > + case NF_INET_PRE_ROUTING: > + break; > + case NF_INET_POST_ROUTING: > + break; > + default: > + goto err1; > + } > + > + if (tb[TCA_IPT_TABLE]) { > + /* mangle only for now */ > + if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle")) > + goto err1; > + } > > - err = -ENOMEM; > - tname = kmalloc(IFNAMSIZ, GFP_KERNEL); > + tname = kstrdup("mangle", GFP_KERNEL); > if (unlikely(!tname)) > goto err1; > - if (tb[TCA_IPT_TABLE] == NULL || > - nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ) > - strcpy(tname, "mangle"); > > t = kmemdup(td, td->u.target_size, GFP_KERNEL); > if (unlikely(!t)) > -- > 2.40.1 >
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c index 5d96ffebd40f..ea7f151e7dd2 100644 --- a/net/sched/act_ipt.c +++ b/net/sched/act_ipt.c @@ -48,7 +48,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t, par.entryinfo = &e; par.target = target; par.targinfo = t->data; - par.hook_mask = hook; + par.hook_mask = 1 << hook; par.family = NFPROTO_IPV4; ret = xt_check_target(&par, t->u.target_size - sizeof(*t), 0, false); @@ -85,7 +85,8 @@ static void tcf_ipt_release(struct tc_action *a) static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = { [TCA_IPT_TABLE] = { .type = NLA_STRING, .len = IFNAMSIZ }, - [TCA_IPT_HOOK] = { .type = NLA_U32 }, + [TCA_IPT_HOOK] = NLA_POLICY_RANGE(NLA_U32, NF_INET_PRE_ROUTING, + NF_INET_NUMHOOKS), [TCA_IPT_INDEX] = { .type = NLA_U32 }, [TCA_IPT_TARG] = { .len = sizeof(struct xt_entry_target) }, }; @@ -158,15 +159,27 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla, return -EEXIST; } } + + err = -EINVAL; hook = nla_get_u32(tb[TCA_IPT_HOOK]); + switch (hook) { + case NF_INET_PRE_ROUTING: + break; + case NF_INET_POST_ROUTING: + break; + default: + goto err1; + } + + if (tb[TCA_IPT_TABLE]) { + /* mangle only for now */ + if (nla_strcmp(tb[TCA_IPT_TABLE], "mangle")) + goto err1; + } - err = -ENOMEM; - tname = kmalloc(IFNAMSIZ, GFP_KERNEL); + tname = kstrdup("mangle", GFP_KERNEL); if (unlikely(!tname)) goto err1; - if (tb[TCA_IPT_TABLE] == NULL || - nla_strscpy(tname, tb[TCA_IPT_TABLE], IFNAMSIZ) >= IFNAMSIZ) - strcpy(tname, "mangle"); t = kmemdup(td, td->u.target_size, GFP_KERNEL); if (unlikely(!t))
Looks like "tc" hard-codes "mangle" as the only supported table name, but on kernel side there are no checks. This is wrong. Not all xtables targets are safe to call from tc. E.g. "nat" targets assume skb has a conntrack object assigned to it. Normally those get called from netfilter nat core which consults the nat table to obtain the address mapping. "tc" userspace either sets PRE or POSTROUTING as hook number, but there is no validation of this on kernel side, so update netlink policy to reject bogus numbers. Some targets may assume skb_dst is set for input/forward hooks, so prevent those from being used. act_ipt uses the hook number in two places: 1. the state hook number, this is fine as-is 2. to set par.hook_mask The latter is a bit mask, so update the assignment to make xt_check_target() to the right thing. Followup patch adds required checks for the skb/packet headers before calling the targets evaluation function. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Florian Westphal <fw@strlen.de> Reviewed-by: Simon Horman <simon.horman@corigine.com> --- v2: add Fixes tag, diff unchanged. net/sched/act_ipt.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)