| Message ID | 20181004213355.14899-10-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:44PM -0700, David Ahern wrote: > From: David Ahern <dsahern@gmail.com> > > Update rtnl_bridge_getlink 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 IFLA_EXT_MASK > attribute is supported. > > Signed-off-by: David Ahern <dsahern@gmail.com> > --- > net/core/rtnetlink.c | 50 ++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 10 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 4fd27b5db787..d2c8d41a6fbc 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -4000,27 +4000,57 @@ EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink); > > static int rtnl_bridge_getlink(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 nlattr *tb[IFLA_MAX+1]; > struct net_device *dev; > int idx = 0; > u32 portid = NETLINK_CB(cb->skb).portid; > - u32 seq = cb->nlh->nlmsg_seq; > + u32 seq = nlh->nlmsg_seq; > u32 filter_mask = 0; > - int err; > + int err, i; > > - if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) { > - struct nlattr *extfilt; > + if (cb->strict_check) { > + struct ifinfomsg *ifm; > > - extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg), > - IFLA_EXT_MASK); > - if (extfilt) { > - if (nla_len(extfilt) < sizeof(filter_mask)) > - return -EINVAL; > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) { > + NL_SET_ERR_MSG(extack, "Invalid header"); > + return -EINVAL; > + } > + > + ifm = nlmsg_data(nlh); > + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags || > + ifm->ifi_change || ifm->ifi_index) { > + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); > + return -EINVAL; > + } > + } > > - filter_mask = nla_get_u32(extfilt); > + err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX, > + ifla_policy, extack); > + if (err < 0) { > + if (cb->strict_check) > + return -EINVAL; > + goto walk_entries; > + } What's the point of moving this out of the if (cb->strict_check) {} branch above? This looks like it would cause the same parse warnings that we're trying to get rid of in inet{4,6} dumps. Seems to make more sense to make the nlmsg_parse() itself conditional as well unless I'm lacking context. > + > + for (i = 0; i <= IFLA_MAX; ++i) { > + if (!tb[i]) > + continue; > + switch (i) { I'm a fan of \n between different conditions etc. so can we please have one after the continue. :) > + case IFLA_EXT_MASK: > + filter_mask = nla_get_u32(tb[i]); > + break; > + default: > + if (cb->strict_check) { > + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); > + return -EINVAL; > + } > } > } > > +walk_entries: > rcu_read_lock(); > for_each_netdev_rcu(net, dev) { > const struct net_device_ops *ops = dev->netdev_ops; > -- > 2.11.0 >
On 10/7/18 4:36 AM, Christian Brauner wrote: >> + if (cb->strict_check) { >> + struct ifinfomsg *ifm; >> >> - extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg), >> - IFLA_EXT_MASK); >> - if (extfilt) { >> - if (nla_len(extfilt) < sizeof(filter_mask)) >> - return -EINVAL; >> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) { >> + NL_SET_ERR_MSG(extack, "Invalid header"); >> + return -EINVAL; >> + } >> + >> + ifm = nlmsg_data(nlh); >> + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags || >> + ifm->ifi_change || ifm->ifi_index) { >> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); >> + return -EINVAL; >> + } >> + } >> >> - filter_mask = nla_get_u32(extfilt); >> + err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX, >> + ifla_policy, extack); >> + if (err < 0) { >> + if (cb->strict_check) >> + return -EINVAL; >> + goto walk_entries; >> + } > > What's the point of moving this out of the > if (cb->strict_check) {} branch above? This looks like it would cause > the same parse warnings that we're trying to get rid of in inet{4,6} > dumps. Link messages don't have the problem in general because they use ifinfomsg as the header - which is the one abused for other message types. That said ... > Seems to make more sense to make the nlmsg_parse() itself conditional as > well unless I'm lacking context. ... I now have nlmsg_parse and nlmsg_parse_strict.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 4fd27b5db787..d2c8d41a6fbc 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -4000,27 +4000,57 @@ EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink); static int rtnl_bridge_getlink(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 nlattr *tb[IFLA_MAX+1]; struct net_device *dev; int idx = 0; u32 portid = NETLINK_CB(cb->skb).portid; - u32 seq = cb->nlh->nlmsg_seq; + u32 seq = nlh->nlmsg_seq; u32 filter_mask = 0; - int err; + int err, i; - if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) { - struct nlattr *extfilt; + if (cb->strict_check) { + struct ifinfomsg *ifm; - extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg), - IFLA_EXT_MASK); - if (extfilt) { - if (nla_len(extfilt) < sizeof(filter_mask)) - return -EINVAL; + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) { + NL_SET_ERR_MSG(extack, "Invalid header"); + return -EINVAL; + } + + ifm = nlmsg_data(nlh); + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags || + ifm->ifi_change || ifm->ifi_index) { + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); + return -EINVAL; + } + } - filter_mask = nla_get_u32(extfilt); + err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX, + ifla_policy, extack); + if (err < 0) { + if (cb->strict_check) + return -EINVAL; + goto walk_entries; + } + + for (i = 0; i <= IFLA_MAX; ++i) { + if (!tb[i]) + continue; + switch (i) { + case IFLA_EXT_MASK: + filter_mask = nla_get_u32(tb[i]); + break; + default: + if (cb->strict_check) { + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); + return -EINVAL; + } } } +walk_entries: rcu_read_lock(); for_each_netdev_rcu(net, dev) { const struct net_device_ops *ops = dev->netdev_ops;