* [PATCH net-next] netlink: add missing length check of rtnl msg
@ 2013-03-25 17:22 Hong Zhiguo
2013-03-25 17:54 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hong Zhiguo @ 2013-03-25 17:22 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, tgraf, Hong Zhiguo
When the legacy array rtm_min still exists, the length check within these
functions is covered by rtm_min[RTM_NEWTFILTER], rtm_min[RTM_NEWQDISC]
and rtm_min[RTM_NEWTCLASS].
But after Thomas Graf removed rtm_min several days ago, these checks are
missing.
Other doit functions should be OK.
Signed-off-by: Hong Zhiguo <honkiko@gmail.com>
---
net/sched/cls_api.c | 4 ++++
net/sched/sch_api.c | 22 ++++++++++++++++++----
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9a04b98..01bfa9c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -141,6 +141,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
if ((n->nlmsg_type != RTM_GETTFILTER) && !capable(CAP_NET_ADMIN))
return -EPERM;
+
+ if (nlmsg_len(n) < sizeof(*t))
+ return -EINVAL;
+
replay:
t = nlmsg_data(n);
protocol = TC_H_MIN(t->tcm_info);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 0bbce22..b30a988 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -977,7 +977,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n)
struct tcmsg *tcm = nlmsg_data(n);
struct nlattr *tca[TCA_MAX + 1];
struct net_device *dev;
- u32 clid = tcm->tcm_parent;
+ u32 clid;
struct Qdisc *q = NULL;
struct Qdisc *p = NULL;
int err;
@@ -985,6 +985,9 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n)
if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN))
return -EPERM;
+ if (nlmsg_len(n) < sizeof(*tcm))
+ return -EINVAL;
+
dev = __dev_get_by_index(net, tcm->tcm_ifindex);
if (!dev)
return -ENODEV;
@@ -993,6 +996,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n)
if (err < 0)
return err;
+ clid = tcm->tcm_parent;
if (clid) {
if (clid != TC_H_ROOT) {
if (TC_H_MAJ(clid) != TC_H_MAJ(TC_H_INGRESS)) {
@@ -1051,6 +1055,9 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n)
if (!capable(CAP_NET_ADMIN))
return -EPERM;
+ if (nlmsg_len(n) < sizeof(*tcm))
+ return -EINVAL;
+
replay:
/* Reinit, just in case something touches this. */
tcm = nlmsg_data(n);
@@ -1382,14 +1389,17 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n)
const struct Qdisc_class_ops *cops;
unsigned long cl = 0;
unsigned long new_cl;
- u32 portid = tcm->tcm_parent;
- u32 clid = tcm->tcm_handle;
- u32 qid = TC_H_MAJ(clid);
+ u32 portid;
+ u32 clid;
+ u32 qid;
int err;
if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN))
return -EPERM;
+ if (nlmsg_len(n) < sizeof(*tcm))
+ return -EINVAL;
+
dev = __dev_get_by_index(net, tcm->tcm_ifindex);
if (!dev)
return -ENODEV;
@@ -1413,6 +1423,10 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n)
/* Step 1. Determine qdisc handle X:0 */
+ portid = tcm->tcm_parent;
+ clid = tcm->tcm_handle;
+ qid = TC_H_MAJ(clid);
+
if (portid != TC_H_ROOT) {
u32 qid1 = TC_H_MAJ(portid);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] netlink: add missing length check of rtnl msg 2013-03-25 17:22 [PATCH net-next] netlink: add missing length check of rtnl msg Hong Zhiguo @ 2013-03-25 17:54 ` David Miller 2013-03-25 23:12 ` Thomas Graf 2013-03-26 3:36 ` [PATCH v2 net-next] netlink: have length check of rtnl msg before deref Hong Zhiguo 2 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2013-03-25 17:54 UTC (permalink / raw) To: honkiko; +Cc: netdev, stephen, tgraf From: Hong Zhiguo <honkiko@gmail.com> Date: Tue, 26 Mar 2013 01:22:23 +0800 > When the legacy array rtm_min still exists, the length check within these > functions is covered by rtm_min[RTM_NEWTFILTER], rtm_min[RTM_NEWQDISC] > and rtm_min[RTM_NEWTCLASS]. > > But after Thomas Graf removed rtm_min several days ago, these checks are > missing. > > Other doit functions should be OK. > > Signed-off-by: Hong Zhiguo <honkiko@gmail.com> This looks good, Thomas please review. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] netlink: add missing length check of rtnl msg 2013-03-25 17:22 [PATCH net-next] netlink: add missing length check of rtnl msg Hong Zhiguo 2013-03-25 17:54 ` David Miller @ 2013-03-25 23:12 ` Thomas Graf 2013-03-26 3:36 ` [PATCH v2 net-next] netlink: have length check of rtnl msg before deref Hong Zhiguo 2 siblings, 0 replies; 6+ messages in thread From: Thomas Graf @ 2013-03-25 23:12 UTC (permalink / raw) To: Hong Zhiguo; +Cc: netdev, davem, stephen On 03/26/13 at 01:22am, Hong Zhiguo wrote: > protocol = TC_H_MIN(t->tcm_info); > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c > index 0bbce22..b30a988 100644 > --- a/net/sched/sch_api.c > +++ b/net/sched/sch_api.c > @@ -977,7 +977,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) > struct tcmsg *tcm = nlmsg_data(n); > struct nlattr *tca[TCA_MAX + 1]; > struct net_device *dev; > - u32 clid = tcm->tcm_parent; > + u32 clid; > struct Qdisc *q = NULL; > struct Qdisc *p = NULL; > int err; > @@ -985,6 +985,9 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) > if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN)) > return -EPERM; > > + if (nlmsg_len(n) < sizeof(*tcm)) > + return -EINVAL; > + > dev = __dev_get_by_index(net, tcm->tcm_ifindex); > if (!dev) > return -ENODEV; What do you think about moving the nlmsg_parse() call from below here to before __dev_get_by_index()? It also verifies the header length and it's how other netlink code verifies the message type specific header as well. > if (err < 0) > @@ -1051,6 +1055,9 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n) > if (!capable(CAP_NET_ADMIN)) > return -EPERM; > > + if (nlmsg_len(n) < sizeof(*tcm)) > + return -EINVAL; > + Same here. > replay: > /* Reinit, just in case something touches this. */ > tcm = nlmsg_data(n); > @@ -1382,14 +1389,17 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) > const struct Qdisc_class_ops *cops; > unsigned long cl = 0; > unsigned long new_cl; > - u32 portid = tcm->tcm_parent; > - u32 clid = tcm->tcm_handle; > - u32 qid = TC_H_MAJ(clid); > + u32 portid; > + u32 clid; > + u32 qid; > int err; > > if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN)) > return -EPERM; > > + if (nlmsg_len(n) < sizeof(*tcm)) > + return -EINVAL; > + > dev = __dev_get_by_index(net, tcm->tcm_ifindex); > if (!dev) > return -ENODEV; And here. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net-next] netlink: have length check of rtnl msg before deref 2013-03-25 17:22 [PATCH net-next] netlink: add missing length check of rtnl msg Hong Zhiguo 2013-03-25 17:54 ` David Miller 2013-03-25 23:12 ` Thomas Graf @ 2013-03-26 3:36 ` Hong Zhiguo 2013-03-26 8:59 ` Thomas Graf 2 siblings, 1 reply; 6+ messages in thread From: Hong Zhiguo @ 2013-03-26 3:36 UTC (permalink / raw) To: netdev; +Cc: davem, stephen, tgraf When the legacy array rtm_min still exists, the length check within these functions is covered by rtm_min[RTM_NEWTFILTER], rtm_min[RTM_NEWQDISC] and rtm_min[RTM_NEWTCLASS]. But after Thomas Graf removed rtm_min several days ago, these checks are missing. Other doit functions should be OK. Signed-off-by: Hong Zhiguo <honkiko@gmail.com> --- net/sched/cls_api.c | 9 +++++---- net/sched/sch_api.c | 36 +++++++++++++++++++++--------------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 9a04b98..9d71d4d 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -141,7 +141,12 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n) if ((n->nlmsg_type != RTM_GETTFILTER) && !capable(CAP_NET_ADMIN)) return -EPERM; + replay: + err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL); + if (err < 0) + return err; + t = nlmsg_data(n); protocol = TC_H_MIN(t->tcm_info); prio = TC_H_MAJ(t->tcm_info); @@ -164,10 +169,6 @@ replay: if (dev == NULL) return -ENODEV; - err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, NULL); - if (err < 0) - return err; - /* Find qdisc */ if (!parent) { q = dev->qdisc; diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 0bbce22..d7468ba 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -977,7 +977,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct tcmsg *tcm = nlmsg_data(n); struct nlattr *tca[TCA_MAX + 1]; struct net_device *dev; - u32 clid = tcm->tcm_parent; + u32 clid; struct Qdisc *q = NULL; struct Qdisc *p = NULL; int err; @@ -985,14 +985,15 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN)) return -EPERM; - dev = __dev_get_by_index(net, tcm->tcm_ifindex); - if (!dev) - return -ENODEV; - err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); if (err < 0) return err; + dev = __dev_get_by_index(net, tcm->tcm_ifindex); + if (!dev) + return -ENODEV; + + clid = tcm->tcm_parent; if (clid) { if (clid != TC_H_ROOT) { if (TC_H_MAJ(clid) != TC_H_MAJ(TC_H_INGRESS)) { @@ -1053,6 +1054,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n) replay: /* Reinit, just in case something touches this. */ + err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); + if (err < 0) + return err; + tcm = nlmsg_data(n); clid = tcm->tcm_parent; q = p = NULL; @@ -1061,9 +1066,6 @@ replay: if (!dev) return -ENODEV; - err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); - if (err < 0) - return err; if (clid) { if (clid != TC_H_ROOT) { @@ -1382,22 +1384,22 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) const struct Qdisc_class_ops *cops; unsigned long cl = 0; unsigned long new_cl; - u32 portid = tcm->tcm_parent; - u32 clid = tcm->tcm_handle; - u32 qid = TC_H_MAJ(clid); + u32 portid; + u32 clid; + u32 qid; int err; if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN)) return -EPERM; - dev = __dev_get_by_index(net, tcm->tcm_ifindex); - if (!dev) - return -ENODEV; - err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); if (err < 0) return err; + dev = __dev_get_by_index(net, tcm->tcm_ifindex); + if (!dev) + return -ENODEV; + /* parent == TC_H_UNSPEC - unspecified parent. parent == TC_H_ROOT - class is root, which has no parent. @@ -1413,6 +1415,10 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) /* Step 1. Determine qdisc handle X:0 */ + portid = tcm->tcm_parent; + clid = tcm->tcm_handle; + qid = TC_H_MAJ(clid); + if (portid != TC_H_ROOT) { u32 qid1 = TC_H_MAJ(portid); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net-next] netlink: have length check of rtnl msg before deref 2013-03-26 3:36 ` [PATCH v2 net-next] netlink: have length check of rtnl msg before deref Hong Zhiguo @ 2013-03-26 8:59 ` Thomas Graf 2013-03-26 16:36 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Thomas Graf @ 2013-03-26 8:59 UTC (permalink / raw) To: Hong Zhiguo; +Cc: netdev, davem, stephen On 03/26/13 at 11:36am, Hong Zhiguo wrote: > When the legacy array rtm_min still exists, the length check within > these functions is covered by rtm_min[RTM_NEWTFILTER], > rtm_min[RTM_NEWQDISC] and rtm_min[RTM_NEWTCLASS]. > > But after Thomas Graf removed rtm_min several days ago, these checks > are missing. Other doit functions should be OK. > > Signed-off-by: Hong Zhiguo <honkiko@gmail.com> Acked-by: Thomas Graf <tgraf@suug.ch> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net-next] netlink: have length check of rtnl msg before deref 2013-03-26 8:59 ` Thomas Graf @ 2013-03-26 16:36 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2013-03-26 16:36 UTC (permalink / raw) To: tgraf; +Cc: honkiko, netdev, stephen From: Thomas Graf <tgraf@suug.ch> Date: Tue, 26 Mar 2013 08:59:37 +0000 > On 03/26/13 at 11:36am, Hong Zhiguo wrote: >> When the legacy array rtm_min still exists, the length check within >> these functions is covered by rtm_min[RTM_NEWTFILTER], >> rtm_min[RTM_NEWQDISC] and rtm_min[RTM_NEWTCLASS]. >> >> But after Thomas Graf removed rtm_min several days ago, these checks >> are missing. Other doit functions should be OK. >> >> Signed-off-by: Hong Zhiguo <honkiko@gmail.com> > > Acked-by: Thomas Graf <tgraf@suug.ch> Applied, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-26 16:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-03-25 17:22 [PATCH net-next] netlink: add missing length check of rtnl msg Hong Zhiguo 2013-03-25 17:54 ` David Miller 2013-03-25 23:12 ` Thomas Graf 2013-03-26 3:36 ` [PATCH v2 net-next] netlink: have length check of rtnl msg before deref Hong Zhiguo 2013-03-26 8:59 ` Thomas Graf 2013-03-26 16:36 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).