On Mon, Oct 07, 2019 at 01:26:28PM -0700, Cong Wang wrote: > Marcelo noticed a backward compatibility issue of TCA_KIND > after we move from NLA_STRING to NLA_NUL_STRING, so it is probably > too late to change it. > > Instead, to make everyone happy, we can just insert a NUL to > terminate the string with nla_strlcpy() like we do for TC actions. > > Fixes: 62794fc4fbf5 ("net_sched: add max len check for TCA_KIND") > Reported-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> > Cc: Jamal Hadi Salim <j...@mojatatu.com> > Cc: Jiri Pirko <j...@resnulli.us> > Signed-off-by: Cong Wang <xiyou.wangc...@gmail.com> > --- > net/sched/cls_api.c | 36 +++++++++++++++++++++++++++++++++--- > net/sched/sch_api.c | 3 +-- > 2 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 64584a1df425..8717c0b26c90 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -162,11 +162,22 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp) > return TC_H_MAJ(first); > } > > +static bool tcf_proto_check_kind(struct nlattr *kind, char *name) > +{ > + if (kind) > + return nla_strlcpy(name, kind, IFNAMSIZ) >= IFNAMSIZ; > + memset(name, 0, IFNAMSIZ); > + return false; > +} > + > static bool tcf_proto_is_unlocked(const char *kind) > { > const struct tcf_proto_ops *ops; > bool ret; > > + if (strlen(kind) == 0) > + return false; > + > ops = tcf_proto_lookup_ops(kind, false, NULL); > /* On error return false to take rtnl lock. Proto lookup/create > * functions will perform lookup again and properly handle errors. > @@ -1843,6 +1854,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > { > struct net *net = sock_net(skb->sk); > struct nlattr *tca[TCA_MAX + 1]; > + char name[IFNAMSIZ]; > struct tcmsg *t; > u32 protocol; > u32 prio; > @@ -1899,13 +1911,19 @@ static int tc_new_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > if (err) > return err; > > + if (tcf_proto_check_kind(tca[TCA_KIND], name)) { > + NL_SET_ERR_MSG(extack, "Specified TC filter name too long"); > + err = -EINVAL; > + goto errout; > + } > + > /* Take rtnl mutex if rtnl_held was set to true on previous iteration, > * block is shared (no qdisc found), qdisc is not unlocked, classifier > * type is not specified, classifier is not unlocked. > */ > if (rtnl_held || > (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) || > - !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) { > + !tcf_proto_is_unlocked(name)) { > rtnl_held = true; > rtnl_lock(); > } > @@ -2063,6 +2081,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > { > struct net *net = sock_net(skb->sk); > struct nlattr *tca[TCA_MAX + 1]; > + char name[IFNAMSIZ]; > struct tcmsg *t; > u32 protocol; > u32 prio; > @@ -2102,13 +2121,18 @@ static int tc_del_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > if (err) > return err; > > + if (tcf_proto_check_kind(tca[TCA_KIND], name)) { > + NL_SET_ERR_MSG(extack, "Specified TC filter name too long"); > + err = -EINVAL; > + goto errout; > + } > /* Take rtnl mutex if flushing whole chain, block is shared (no qdisc > * found), qdisc is not unlocked, classifier type is not specified, > * classifier is not unlocked. > */ > if (!prio || > (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) || > - !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) { > + !tcf_proto_is_unlocked(name)) { > rtnl_held = true; > rtnl_lock(); > } > @@ -2216,6 +2240,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > { > struct net *net = sock_net(skb->sk); > struct nlattr *tca[TCA_MAX + 1]; > + char name[IFNAMSIZ]; > struct tcmsg *t; > u32 protocol; > u32 prio; > @@ -2252,12 +2277,17 @@ static int tc_get_tfilter(struct sk_buff *skb, struct > nlmsghdr *n, > if (err) > return err; > > + if (tcf_proto_check_kind(tca[TCA_KIND], name)) { > + NL_SET_ERR_MSG(extack, "Specified TC filter name too long"); > + err = -EINVAL; > + goto errout; > + } > /* Take rtnl mutex if block is shared (no qdisc found), qdisc is not > * unlocked, classifier type is not specified, classifier is not > * unlocked. > */ > if ((q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) || > - !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) { > + !tcf_proto_is_unlocked(name)) { > rtnl_held = true; > rtnl_lock(); > } > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index 81d58b280612..1047825d9f48 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -1390,8 +1390,7 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct > qdisc_walker *w) > } > > const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = { > - [TCA_KIND] = { .type = NLA_NUL_STRING, > - .len = IFNAMSIZ - 1 }, > + [TCA_KIND] = { .type = NLA_STRING }, > [TCA_RATE] = { .type = NLA_BINARY, > .len = sizeof(struct tc_estimator) }, > [TCA_STAB] = { .type = NLA_NESTED }, > -- > 2.21.0 >