Re: [Patch net] net_sched: fix backward compatibility for TCA_KIND

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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.leitner@gmail.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@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
>