| Message ID | 20181004213355.14899-9-dsahern@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | rtnetlink: Add support for rigid checking of data in dump request | expand |
On Thu, Oct 04, 2018 at 02:33:43PM -0700, David Ahern wrote: > From: David Ahern <dsahern@gmail.com> > > Update rtnl_dump_ifinfo for strict data checking. If the flag is set, > the dump request is expected to have an ifinfomsg struct as the header > potentially followed by one or more attributes. Any data passed in the > header or as an attribute is taken as a request to influence the data > returned. Only values supported by the dump handler are allowed to be > non-0 or set in the request. At the moment only the IFA_TARGET_NETNSID, > IFLA_EXT_MASK, IFLA_MASTER, and IFLA_LINKINFO attributes are supported. > > Existing code does not fail the dump if nlmsg_parse fails. That behavior > is kept for non-strict checking. > > Signed-off-by: David Ahern <dsahern@gmail.com> > --- > net/core/rtnetlink.c | 97 +++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 69 insertions(+), 28 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index c3b434d724ea..4fd27b5db787 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1880,6 +1880,8 @@ EXPORT_SYMBOL_GPL(rtnl_get_net_ns_capable); > > static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > { > + struct netlink_ext_ack *extack = cb->extack; > + const struct nlmsghdr *nlh = cb->nlh; > struct net *net = sock_net(skb->sk); > struct net *tgt_net = net; > int h, s_h; > @@ -1892,44 +1894,84 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > unsigned int flags = NLM_F_MULTI; > int master_idx = 0; > int netnsid = -1; > - int err; > + int err, i; > int hdrlen; > > s_h = cb->args[0]; > s_idx = cb->args[1]; > > - /* A hack to preserve kernel<->userspace interface. > - * The correct header is ifinfomsg. It is consistent with rtnl_getlink. > - * However, before Linux v3.9 the code here assumed rtgenmsg and that's > - * what iproute2 < v3.9.0 used. > - * We can detect the old iproute2. Even including the IFLA_EXT_MASK > - * attribute, its netlink message is shorter than struct ifinfomsg. > - */ > - hdrlen = nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg) ? > - sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg); > + if (cb->strict_check) { > + struct ifinfomsg *ifm; > > - if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX, > - ifla_policy, cb->extack) >= 0) { > - if (tb[IFLA_TARGET_NETNSID]) { > - netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]); > - tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); > - if (IS_ERR(tgt_net)) > - return PTR_ERR(tgt_net); > + hdrlen = sizeof(*ifm); > + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) { > + NL_SET_ERR_MSG(extack, "Invalid header"); > + return -EINVAL; > } > > - if (tb[IFLA_EXT_MASK]) > - ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]); > - > - if (tb[IFLA_MASTER]) > - master_idx = nla_get_u32(tb[IFLA_MASTER]); > + ifm = nlmsg_data(nlh); > + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags || > + ifm->ifi_change) { > + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); > + return -EINVAL; > + } > + if (ifm->ifi_index) { > + NL_SET_ERR_MSG(extack, "Filter by device index not supported"); > + return -EINVAL; > + } > + } else { > + /* A hack to preserve kernel<->userspace interface. > + * The correct header is ifinfomsg. It is consistent with > + * rtnl_getlink. However, before Linux v3.9 the code here > + * assumed rtgenmsg and that's what iproute2 < v3.9.0 used. > + * We can detect the old iproute2. Even including the > + * IFLA_EXT_MASK attribute, its netlink message is shorter > + * than struct ifinfomsg. > + */ > + hdrlen = nlmsg_len(nlh) < sizeof(struct ifinfomsg) ? > + sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg); > + } > > - if (tb[IFLA_LINKINFO]) > - kind_ops = linkinfo_to_kind_ops(tb[IFLA_LINKINFO]); > + err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack); > + if (err < 0) { > + if (cb->strict_check) > + return -EINVAL; > + goto walk_entries; > + } > > - if (master_idx || kind_ops) > - flags |= NLM_F_DUMP_FILTERED; > + for (i = 0; i <= IFLA_MAX; ++i) { > + if (!tb[i]) > + continue; > + switch (i) { > + case IFLA_TARGET_NETNSID: > + netnsid = nla_get_s32(tb[i]); > + tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); > + if (IS_ERR(tgt_net)) { > + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); > + return PTR_ERR(tgt_net); > + } > + break; > + case IFLA_EXT_MASK: > + ext_filter_mask = nla_get_u32(tb[i]); > + break; > + case IFLA_MASTER: > + master_idx = nla_get_u32(tb[i]); > + break; > + case IFLA_LINKINFO: > + kind_ops = linkinfo_to_kind_ops(tb[i]); > + break; > + default: > + if (cb->strict_check) { > + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); > + return -EINVAL; > + } > + } This might make sense to be split into two helpers for parsing: <blablabla>_strict() and <blablabla>_lenient(). :) > } > > + if (master_idx || kind_ops) > + flags |= NLM_F_DUMP_FILTERED; > + > +walk_entries: > for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) { > idx = 0; > head = &tgt_net->dev_index_head[h]; > @@ -1941,8 +1983,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) > err = rtnl_fill_ifinfo(skb, dev, net, > RTM_NEWLINK, > NETLINK_CB(cb->skb).portid, > - cb->nlh->nlmsg_seq, 0, > - flags, > + nlh->nlmsg_seq, 0, flags, > ext_filter_mask, 0, NULL, 0, > netnsid); > > -- > 2.11.0 >
On 10/5/18 11:59 AM, Christian Brauner wrote: >> + err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack); >> + if (err < 0) { >> + if (cb->strict_check) >> + return -EINVAL; >> + goto walk_entries; >> + } >> >> - if (master_idx || kind_ops) >> - flags |= NLM_F_DUMP_FILTERED; >> + for (i = 0; i <= IFLA_MAX; ++i) { >> + if (!tb[i]) >> + continue; >> + switch (i) { >> + case IFLA_TARGET_NETNSID: >> + netnsid = nla_get_s32(tb[i]); >> + tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); >> + if (IS_ERR(tgt_net)) { >> + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); >> + return PTR_ERR(tgt_net); >> + } >> + break; >> + case IFLA_EXT_MASK: >> + ext_filter_mask = nla_get_u32(tb[i]); >> + break; >> + case IFLA_MASTER: >> + master_idx = nla_get_u32(tb[i]); >> + break; >> + case IFLA_LINKINFO: >> + kind_ops = linkinfo_to_kind_ops(tb[i]); >> + break; >> + default: >> + if (cb->strict_check) { >> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); >> + return -EINVAL; >> + } >> + } > > This might make sense to be split into two helpers for parsing: > <blablabla>_strict() and <blablabla>_lenient(). :) I thought about that, but there is so much overlap - they are mostly common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg is the one that has been (ab)used for other message types, so strict versus lenient does not really have a differentiator for this message type - other than checking the elements of the struct.
On Fri, Oct 05, 2018 at 01:22:24PM -0600, David Ahern wrote: > On 10/5/18 11:59 AM, Christian Brauner wrote: > >> + err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack); > >> + if (err < 0) { > >> + if (cb->strict_check) > >> + return -EINVAL; > >> + goto walk_entries; > >> + } > >> > >> - if (master_idx || kind_ops) > >> - flags |= NLM_F_DUMP_FILTERED; > >> + for (i = 0; i <= IFLA_MAX; ++i) { > >> + if (!tb[i]) > >> + continue; > >> + switch (i) { > >> + case IFLA_TARGET_NETNSID: > >> + netnsid = nla_get_s32(tb[i]); > >> + tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); > >> + if (IS_ERR(tgt_net)) { > >> + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); > >> + return PTR_ERR(tgt_net); > >> + } > >> + break; > >> + case IFLA_EXT_MASK: > >> + ext_filter_mask = nla_get_u32(tb[i]); > >> + break; > >> + case IFLA_MASTER: > >> + master_idx = nla_get_u32(tb[i]); > >> + break; > >> + case IFLA_LINKINFO: > >> + kind_ops = linkinfo_to_kind_ops(tb[i]); > >> + break; > >> + default: > >> + if (cb->strict_check) { > >> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); > >> + return -EINVAL; > >> + } > >> + } > > > > This might make sense to be split into two helpers for parsing: > > <blablabla>_strict() and <blablabla>_lenient(). :) > > I thought about that, but there is so much overlap - they are mostly > common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg > is the one that has been (ab)used for other message types, so strict > versus lenient does not really have a differentiator for this message > type - other than checking the elements of the struct. It's mostly about the function being extremely long and convoluted. Having parts moved out into (a) descriptive helper(s) with whatever name might make this way more readable than it is now especially with the new handling we need for strict checking.
On 10/7/18 4:29 AM, Christian Brauner wrote: >> I thought about that, but there is so much overlap - they are mostly >> common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg >> is the one that has been (ab)used for other message types, so strict >> versus lenient does not really have a differentiator for this message >> type - other than checking the elements of the struct. > > It's mostly about the function being extremely long and convoluted. > Having parts moved out into (a) descriptive helper(s) with whatever name > might make this way more readable than it is now especially with the new > handling we need for strict checking. > understood. In the next version I have pushed most of the checking into helpers.
On Sun, Oct 07, 2018 at 07:29:13PM -0600, David Ahern wrote: > On 10/7/18 4:29 AM, Christian Brauner wrote: > >> I thought about that, but there is so much overlap - they are mostly > >> common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg > >> is the one that has been (ab)used for other message types, so strict > >> versus lenient does not really have a differentiator for this message > >> type - other than checking the elements of the struct. > > > > It's mostly about the function being extremely long and convoluted. > > Having parts moved out into (a) descriptive helper(s) with whatever name > > might make this way more readable than it is now especially with the new > > handling we need for strict checking. > > > > understood. In the next version I have pushed most of the checking into > helpers. Thank you!
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index c3b434d724ea..4fd27b5db787 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1880,6 +1880,8 @@ EXPORT_SYMBOL_GPL(rtnl_get_net_ns_capable); static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) { + struct netlink_ext_ack *extack = cb->extack; + const struct nlmsghdr *nlh = cb->nlh; struct net *net = sock_net(skb->sk); struct net *tgt_net = net; int h, s_h; @@ -1892,44 +1894,84 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) unsigned int flags = NLM_F_MULTI; int master_idx = 0; int netnsid = -1; - int err; + int err, i; int hdrlen; s_h = cb->args[0]; s_idx = cb->args[1]; - /* A hack to preserve kernel<->userspace interface. - * The correct header is ifinfomsg. It is consistent with rtnl_getlink. - * However, before Linux v3.9 the code here assumed rtgenmsg and that's - * what iproute2 < v3.9.0 used. - * We can detect the old iproute2. Even including the IFLA_EXT_MASK - * attribute, its netlink message is shorter than struct ifinfomsg. - */ - hdrlen = nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg) ? - sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg); + if (cb->strict_check) { + struct ifinfomsg *ifm; - if (nlmsg_parse(cb->nlh, hdrlen, tb, IFLA_MAX, - ifla_policy, cb->extack) >= 0) { - if (tb[IFLA_TARGET_NETNSID]) { - netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]); - tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); - if (IS_ERR(tgt_net)) - return PTR_ERR(tgt_net); + hdrlen = sizeof(*ifm); + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) { + NL_SET_ERR_MSG(extack, "Invalid header"); + return -EINVAL; } - if (tb[IFLA_EXT_MASK]) - ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]); - - if (tb[IFLA_MASTER]) - master_idx = nla_get_u32(tb[IFLA_MASTER]); + ifm = nlmsg_data(nlh); + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags || + ifm->ifi_change) { + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); + return -EINVAL; + } + if (ifm->ifi_index) { + NL_SET_ERR_MSG(extack, "Filter by device index not supported"); + return -EINVAL; + } + } else { + /* A hack to preserve kernel<->userspace interface. + * The correct header is ifinfomsg. It is consistent with + * rtnl_getlink. However, before Linux v3.9 the code here + * assumed rtgenmsg and that's what iproute2 < v3.9.0 used. + * We can detect the old iproute2. Even including the + * IFLA_EXT_MASK attribute, its netlink message is shorter + * than struct ifinfomsg. + */ + hdrlen = nlmsg_len(nlh) < sizeof(struct ifinfomsg) ? + sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg); + } - if (tb[IFLA_LINKINFO]) - kind_ops = linkinfo_to_kind_ops(tb[IFLA_LINKINFO]); + err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack); + if (err < 0) { + if (cb->strict_check) + return -EINVAL; + goto walk_entries; + } - if (master_idx || kind_ops) - flags |= NLM_F_DUMP_FILTERED; + for (i = 0; i <= IFLA_MAX; ++i) { + if (!tb[i]) + continue; + switch (i) { + case IFLA_TARGET_NETNSID: + netnsid = nla_get_s32(tb[i]); + tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid); + if (IS_ERR(tgt_net)) { + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); + return PTR_ERR(tgt_net); + } + break; + case IFLA_EXT_MASK: + ext_filter_mask = nla_get_u32(tb[i]); + break; + case IFLA_MASTER: + master_idx = nla_get_u32(tb[i]); + break; + case IFLA_LINKINFO: + kind_ops = linkinfo_to_kind_ops(tb[i]); + break; + default: + if (cb->strict_check) { + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); + return -EINVAL; + } + } } + if (master_idx || kind_ops) + flags |= NLM_F_DUMP_FILTERED; + +walk_entries: for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) { idx = 0; head = &tgt_net->dev_index_head[h]; @@ -1941,8 +1983,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) err = rtnl_fill_ifinfo(skb, dev, net, RTM_NEWLINK, NETLINK_CB(cb->skb).portid, - cb->nlh->nlmsg_seq, 0, - flags, + nlh->nlmsg_seq, 0, flags, ext_filter_mask, 0, NULL, 0, netnsid);