Message ID | 20181004213355.14899-7-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:41PM -0700, David Ahern wrote: > From: David Ahern <dsahern@gmail.com> > > Update inet_dump_ifaddr for strict data checking. If the flag is set, > the dump request is expected to have an ifaddrmsg 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 > attribute is supported. Follow on patches can support for other fields > (e.g., honor ifa_index and only return data for the given device index). > > Signed-off-by: David Ahern <dsahern@gmail.com> > --- > net/ipv4/devinet.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 43 insertions(+), 10 deletions(-) > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index ab2b11df5ea4..af968d4fe4fc 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -1662,15 +1662,16 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa, > > static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) > { > + struct netlink_ext_ack *extack = cb->extack; > + const struct nlmsghdr *nlh = cb->nlh; > struct inet_fill_args fillargs = { > .portid = NETLINK_CB(cb->skb).portid, > - .seq = cb->nlh->nlmsg_seq, > + .seq = nlh->nlmsg_seq, > .event = RTM_NEWADDR, > .flags = NLM_F_MULTI, > .netnsid = -1, > }; > struct net *net = sock_net(skb->sk); > - struct nlattr *tb[IFA_MAX+1]; > struct net *tgt_net = net; > int h, s_h; > int idx, s_idx; > @@ -1684,15 +1685,47 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) > s_idx = idx = cb->args[1]; > s_ip_idx = ip_idx = cb->args[2]; > > - if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX, > - ifa_ipv4_policy, cb->extack) >= 0) { > - if (tb[IFA_TARGET_NETNSID]) { > - fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]); > + if (cb->strict_check) { > + struct nlattr *tb[IFA_MAX+1]; > + struct ifaddrmsg *ifm; > + int err, i; > + > + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) { > + NL_SET_ERR_MSG(extack, "Invalid header"); > + return -EINVAL; > + } > + > + ifm = nlmsg_data(nlh); > + if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) { > + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); > + return -EINVAL; > + } > + if (ifm->ifa_index) { > + NL_SET_ERR_MSG(extack, "Filter by device index not supported"); > + return -EINVAL; > + } > + > + err = nlmsg_parse(nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX, > + ifa_ipv4_policy, extack); > + if (err < 0) > + return err; > > - tgt_net = rtnl_get_net_ns_capable(skb->sk, > - fillargs.netnsid); > - if (IS_ERR(tgt_net)) > - return PTR_ERR(tgt_net); > + for (i = 0; i <= IFA_MAX; ++i) { > + if (!tb[i]) > + continue; > + if (i == IFA_TARGET_NETNSID) { Nit: For the sake of readability there could be an additional newline between the "continue" and the next if () condition. > + fillargs.netnsid = nla_get_s32(tb[i]); > + > + tgt_net = rtnl_get_net_ns_capable(skb->sk, > + fillargs.netnsid); > + if (IS_ERR(tgt_net)) { > + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); Nit: Hm, maybe "Invalid target network namespace id" would be better. You never know what namespace comes along some time later. :) > + return PTR_ERR(tgt_net); > + } > + } else { > + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); > + return -EINVAL; > + } > } > } > > -- > 2.11.0 >
On 10/5/18 12:02 PM, Christian Brauner wrote: >> + >> + err = nlmsg_parse(nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX, >> + ifa_ipv4_policy, extack); >> + if (err < 0) >> + return err; >> >> - tgt_net = rtnl_get_net_ns_capable(skb->sk, >> - fillargs.netnsid); >> - if (IS_ERR(tgt_net)) >> - return PTR_ERR(tgt_net); >> + for (i = 0; i <= IFA_MAX; ++i) { >> + if (!tb[i]) >> + continue; >> + if (i == IFA_TARGET_NETNSID) { > > Nit: For the sake of readability there could be an additional newline between the > "continue" and the next if () condition. > >> + fillargs.netnsid = nla_get_s32(tb[i]); >> + >> + tgt_net = rtnl_get_net_ns_capable(skb->sk, >> + fillargs.netnsid); >> + if (IS_ERR(tgt_net)) { >> + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); > > Nit: Hm, maybe "Invalid target network namespace id" would be better. > You never know what namespace comes along some time later. :) > done.
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index ab2b11df5ea4..af968d4fe4fc 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -1662,15 +1662,16 @@ static int inet_fill_ifaddr(struct sk_buff *skb, struct in_ifaddr *ifa, static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) { + struct netlink_ext_ack *extack = cb->extack; + const struct nlmsghdr *nlh = cb->nlh; struct inet_fill_args fillargs = { .portid = NETLINK_CB(cb->skb).portid, - .seq = cb->nlh->nlmsg_seq, + .seq = nlh->nlmsg_seq, .event = RTM_NEWADDR, .flags = NLM_F_MULTI, .netnsid = -1, }; struct net *net = sock_net(skb->sk); - struct nlattr *tb[IFA_MAX+1]; struct net *tgt_net = net; int h, s_h; int idx, s_idx; @@ -1684,15 +1685,47 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) s_idx = idx = cb->args[1]; s_ip_idx = ip_idx = cb->args[2]; - if (nlmsg_parse(cb->nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX, - ifa_ipv4_policy, cb->extack) >= 0) { - if (tb[IFA_TARGET_NETNSID]) { - fillargs.netnsid = nla_get_s32(tb[IFA_TARGET_NETNSID]); + if (cb->strict_check) { + struct nlattr *tb[IFA_MAX+1]; + struct ifaddrmsg *ifm; + int err, i; + + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) { + NL_SET_ERR_MSG(extack, "Invalid header"); + return -EINVAL; + } + + ifm = nlmsg_data(nlh); + if (ifm->ifa_prefixlen || ifm->ifa_flags || ifm->ifa_scope) { + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request"); + return -EINVAL; + } + if (ifm->ifa_index) { + NL_SET_ERR_MSG(extack, "Filter by device index not supported"); + return -EINVAL; + } + + err = nlmsg_parse(nlh, sizeof(struct ifaddrmsg), tb, IFA_MAX, + ifa_ipv4_policy, extack); + if (err < 0) + return err; - tgt_net = rtnl_get_net_ns_capable(skb->sk, - fillargs.netnsid); - if (IS_ERR(tgt_net)) - return PTR_ERR(tgt_net); + for (i = 0; i <= IFA_MAX; ++i) { + if (!tb[i]) + continue; + if (i == IFA_TARGET_NETNSID) { + fillargs.netnsid = nla_get_s32(tb[i]); + + tgt_net = rtnl_get_net_ns_capable(skb->sk, + fillargs.netnsid); + if (IS_ERR(tgt_net)) { + NL_SET_ERR_MSG(extack, "Invalid target namespace id"); + return PTR_ERR(tgt_net); + } + } else { + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request"); + return -EINVAL; + } } }