Re: [Patch net] net_sched: add max len check for TCA_KIND

On Thu, 3 Oct 2019 16:45:25 -0300, Marcelo Ricardo Leitner wrote:

On Sat, Sep 21, 2019 at 07:24:34PM -0700, Jakub Kicinski wrote:

quoted

Applied, queued for 4.14+, thanks!  

Ahm, this breaks some user applications.

I'm getting "Attribute failed policy validation" extack error while
adding ingress qdisc on an app using libmnl, because it just doesn't
pack the null byte there if it uses mnl_attr_put_str():
https://git.netfilter.org/libmnl/tree/src/attr.c#n481
Unless it uses mnl_attr_put_strz() instead.

Though not sure who's to blame here, as one could argue that the
app should have been using the latter in the first place, but well..
it worked and produced the right results.

Ditto for 199ce850ce11 ("net_sched: add policy validation for action
attributes") on TCA_ACT_KIND.

Thanks for the report Marcelo! This netlink validation stuff is always
super risky I figured better find out if something breaks sooner than
later, hence the backport.

So if I'm understanding this would be the fix?

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 2558f00f6b3e..bcc1178ce50d 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c

@@ -832,8 +832,7 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 }
 
 static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
-       [TCA_ACT_KIND]          = { .type = NLA_NUL_STRING,
-                                   .len = IFNAMSIZ - 1 },
+       [TCA_ACT_KIND]          = { .type = NLA_STRING },
        [TCA_ACT_INDEX]         = { .type = NLA_U32 },
        [TCA_ACT_COOKIE]        = { .type = NLA_BINARY,
                                    .len = TC_COOKIE_MAX_SIZE },

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 },

Cong, are you planning to take a look? With this we have to find a
different way to deal with the KMSAN report you mentioned :(