rtnetlink: Add support for rigid checking of data in dump request
Subsystems (8)
David Ahern
maintainercover rtnetlink: Add support for rigid checking of data in dump request
From: David Ahern <[email protected]>
There are many use cases where a user wants to influence what is returned in a dump for some rtnetlink command: one is wanting data for a different namespace than the one the request is received and another is limiting the amount of data returned in the dump to a specific set of interest to userspace, reducing the cpu overhead of both kernel and userspace. Unfortunately, the kernel has historically not been strict with checking for the proper header or checking the values passed in the header. This lenient implementation has allowed iproute2 and other packages to pass any struct or data in the dump request as long as the family is the first byte. For example, ifinfomsg struct is used by iproute2 for all generic dump requests - links, addresses, routes and rules when it is really only valid for link requests.
There is 1 is example where the kernel deals with the wrong struct: link dumps after VF support was added. Older iproute2 was sending rtgenmsg as the header instead of ifinfomsg so a patch was added to try and detect old userspace vs new:e5eca6d41f53 ("rtnetlink: fix userspace API breakage for iproute2 < v3.9.0")
The latest example is Christian's patch set wanting to return addresses for a target namespace. It guesses the header struct is an ifaddrmsg and if it guesses wrong a netlink warning is generated in the kernel log on every address dump which is unacceptable.
Another example where the kernel is a bit lenient is route dumps: iproute2 can send either a request with either ifinfomsg or a rtmsg as the header struct, yet the kernel always treats the header as an rtmsg (see inet_dump_fib and rtm_flags check). The header inconsistency impacts the ability to add kernel side filters for route dumps - a necessary feature for scale setups with 100k+ routes.
How to resolve the problem of not breaking old userspace yet be able to move forward with new features such as kernel side filtering which are crucial for efficient operation at high scale?
This patch set addresses the problem by adding a new socket flag, NETLINK_DUMP_STRICT_CHK, that userspace can use with setsockopt to request strict checking of headers and attributes on dump requests and hence unlock the ability to use kernel side filters as they are added.
Kernel side, the dump handlers are updated to verify the message contains at least the expected header struct:
RTM_GETLINK: ifinfomsg
RTM_GETADDR: ifaddrmsg
RTM_GETMULTICAST: ifaddrmsg
RTM_GETANYCAST: ifaddrmsg
RTM_GETADDRLABEL: ifaddrlblmsg
RTM_GETROUTE: rtmsg
RTM_GETSTATS: if_stats_msg
RTM_GETNEIGH: ndmsg
RTM_GETNEIGHTBL: ndtmsg
RTM_GETNSID: rtnl_net_dumpid
RTM_GETRULE: fib_rule_hdr
RTM_GETNETCONF: netconfmsg
RTM_GETMDB: br_port_msgAnd then every field in the header struct should be 0 with the exception of the family. There are a few exceptions to this rule where the kernel already influences the data returned by values in the struct. Next the message should not contain attributes unless the kernel implements filtering for it. Any unexpected data causes the dump to fail with EINVAL. If the new flag is honored by the kernel and the dump contents adjusted by any data passed in the request, the dump handler can set the NLM_F_DUMP_FILTERED flag in the netlink message header.
For old userspace on new kernel there is no impact as all checks are wrapped in a check on the new strict flag. For new userspace on old kernel, the data in the headers and any appended attributes are silently ignored though the setsockopt failing is the clue to userspace the feature is not supported. New userspace on new kernel gets the requested data dump.
This set applies on top of the "Extend dump filter to proxy neighbor dumps" patch:
http://patchwork.ozlabs.org/patch/978583/
which was extracted from this set to keep the number of patches to 20 or less.
As an example of how this new strict parsing can be used for kernel side filters on dump requests, route dump filtering patches can be found here:
https://github.com/dsahern/linux.git rtnl-dump-enhancements
those will be sent once this infrastructure is in.
iproute2 patches can be found here:
https://github.com/dsahern/iproute2 dump-enhancements
Changes since rfc-v2
- dropped the NLM_F_DUMP_FILTERED flag from target nsid dumps per Jiri's objections
- changed the opt-in uapi from a netlink message flag to a socket flag. setsockopt provides an api for userspace to definitively know if the kernel supports strict checking on dumps.
- re-ordered patches to peel off the extack on dumps if needed to keep this set size within limits
- misc cleanups in patches based on testing
David Ahern (20):
netlink: Pass extack to dump handlers
netlink: Add extack message to nlmsg_parse for invalid header length
net: Add extack to nlmsg_parse
net/ipv6: Refactor address dump to push inet6_fill_args to
in6_dump_addrs
netlink: Add new socket option to enable strict checking on dumps net/ipv4: Update inet_dump_ifaddr for strict data checking net/ipv6: Update inet6_dump_addr for strict data checking
rtnetlink: Update rtnl_dump_ifinfo for strict data checking
rtnetlink: Update rtnl_bridge_getlink for strict data checking
rtnetlink: Update rtnl_stats_dump for strict data checking
rtnetlink: Update inet6_dump_ifinfo for strict data checking
rtnetlink: Update ipmr_rtm_dumplink for strict data checking
rtnetlink: Update fib dumps for strict data checking net/neighbor: Update neigh_dump_info for strict data checking net/neighbor: Update neightbl_dump_info for strict data checking net/namespace: Update rtnl_net_dumpid for strict data checking net/fib_rules: Update fib_nl_dumprule for strict data checking net/ipv6: Update ip6addrlbl_dump for strict data checking
net: Update netconf dump handlers for strict data checking net/bridge: Update br_mdb_dump for strict data checking
include/linux/netlink.h | 3 + include/net/ip_fib.h | 2 + include/net/netlink.h | 4 +- include/uapi/linux/netlink.h | 1 + net/bridge/br_mdb.c | 29 +++++++ net/core/devlink.c | 2 +- net/core/fib_rules.c | 36 ++++++++- net/core/neighbour.c | 96 +++++++++++++++++++---- net/core/net_namespace.c | 8 ++ net/core/rtnetlink.c | 173 +++++++++++++++++++++++++++++++---------- net/ipv4/devinet.c | 82 +++++++++++++++---- net/ipv4/fib_frontend.c | 43 +++++++++- net/ipv4/ipmr.c | 41 ++++++++++ net/ipv6/addrconf.c | 159 ++++++++++++++++++++++++++++--------- net/ipv6/addrlabel.c | 35 ++++++++- net/ipv6/ip6_fib.c | 8 ++ net/ipv6/ip6mr.c | 9 +++ net/ipv6/route.c | 2 +- net/mpls/af_mpls.c | 28 ++++++- net/netfilter/ipvs/ip_vs_ctl.c | 2 +- net/netlink/af_netlink.c | 33 +++++++- net/netlink/af_netlink.h | 1 + net/sched/act_api.c | 2 +- net/sched/cls_api.c | 6 +- net/sched/sch_api.c | 3 +- net/xfrm/xfrm_user.c | 2 +- 26 files changed, 687 insertions(+), 123 deletions(-)
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 1/20 netlink: Pass extack to dump handlers
From: David Ahern <[email protected]>
Declare extack in netlink_dump and pass to dump handlers via netlink_callback. Add any extack message after the dump_done_errno allowing error messages to be returned. This will be useful when strict checking is done on dump requests, returning why the dump fails EINVAL.
--- include/linux/netlink.h | 1 + net/netlink/af_netlink.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-)
include/linux/netlink.h +1 -0
@@ -176,6 +176,7 @@ struct netlink_callback {void *data;/* the module that dump function belong to */ struct module *module; +struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc;unsigned int prev_seq, seq;
net/netlink/af_netlink.c +11 -1
@@ -2171,6 +2171,7 @@ EXPORT_SYMBOL(__nlmsg_put);static int netlink_dump (struct sock *sk) {struct netlink_sock *nlk =nlk_sk (sk); +struct netlink_ext_ack extack = {};struct netlink_callback *cb;struct sk_buff *skb =NULL ;struct nlmsghdr *nlh;
@@ -2222,8 +2223,11 @@ static int netlink_dump(struct sock *sk)skb_reserve (skb,skb_tailroom (skb) - alloc_size);netlink_skb_set_owner_r (skb, sk); -if (nlk->dump_done_errno >0 ) +if (nlk->dump_done_errno >0 ) { + cb->extack = &extack; nlk->dump_done_errno = cb->dump (skb, cb); + cb->extack =NULL ; + }if (nlk->dump_done_errno >0 ||skb_tailroom (skb) <nlmsg_total_size (sizeof (nlk->dump_done_errno))) {
@@ -2246,6 +2250,12 @@ static int netlink_dump(struct sock *sk)memcpy (nlmsg_data (nlh), &nlk->dump_done_errno,sizeof (nlk->dump_done_errno)); +if (extack._msg && nlk->flags &NETLINK_F_EXT_ACK ) { + nlh->nlmsg_flags |=NLM_F_ACK_TLVS ; +if (!nla_put_string (skb,NLMSGERR_ATTR_MSG , extack._msg)) +nlmsg_end (skb, nlh); + } +if (sk_filter (sk, skb))kfree_skb (skb);else
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 2/20 netlink: Add extack message to nlmsg_parse for invalid header length
In reply to David AhernFrom: David Ahern <[email protected]>
Give a user a reason why EINVAL is returned in nlmsg_parse.
--- include/net/netlink.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
include/net/netlink.h +3 -1
@@ -516,8 +516,10 @@ static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,const struct nla_policy *policy, struct netlink_ext_ack *extack) { - if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) { + NL_SET_ERR_MSG(extack, "Invalid header length");return -EINVAL ; + }return nla_parse (tb, maxtype,nlmsg_attrdata (nlh, hdrlen),nlmsg_attrlen (nlh, hdrlen), policy, extack);
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 3/20 net: Add extack to nlmsg_parse
In reply to David AhernFrom: David Ahern <[email protected]>
Make sure extack is passed to nlmsg_parse where easy to do so. Most of these are dump handlers and leveraging the extack in the netlink_callback.
--- net/core/devlink.c | 2 +- net/core/neighbour.c | 3 ++- net/core/rtnetlink.c | 4 ++-- net/ipv4/devinet.c | 9 +++++---- net/ipv6/addrconf.c | 2 +- net/ipv6/route.c | 2 +- net/mpls/af_mpls.c | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 2 +- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 6 ++++-- net/sched/sch_api.c | 3 ++- net/xfrm/xfrm_user.c | 2 +- 12 files changed, 22 insertions(+), 17 deletions(-)
net/core/devlink.c +1 -1
@@ -3489,7 +3489,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, start_offset = *((u64 *)&cb->args[0 ]); err =nlmsg_parse (cb->nlh,GENL_HDRLEN + devlink_nl_family.hdrsize, - attrs,DEVLINK_ATTR_MAX , ops->policy,NULL ); + attrs,DEVLINK_ATTR_MAX , ops->policy, cb->extack);if (err) goto out;
net/core/neighbour.c +2 -1
@@ -2445,7 +2445,8 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY) proxy =1 ; - err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL ,NULL ); + err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , + cb->extack);if (!err) {if (tb[NDA_IFINDEX ]) {if (nla_len (tb[NDA_IFINDEX ]) !=sizeof (u32))
net/core/rtnetlink.c +2 -2
@@ -1909,7 +1909,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);if (nlmsg_parse (cb->nlh, hdrlen, tb,IFLA_MAX , - ifla_policy,NULL ) >=0 ) { + 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);
@@ -3764,7 +3764,7 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)int fidx =0 ; err =nlmsg_parse (cb->nlh,sizeof (struct ifinfomsg ), tb, -IFLA_MAX , ifla_policy,NULL ); +IFLA_MAX , ifla_policy, cb->extack);if (err <0 ) {return -EINVAL ; }else if (err ==0 ) {
net/ipv4/devinet.c +5 -4
@@ -782,7 +782,8 @@ static void set_ifa_lifetime(struct in_ifaddr *ifa, __u32 valid_lft, }static struct in_ifaddr *rtm_to_ifaddr (struct net *net,struct nlmsghdr *nlh, -__u32 *pvalid_lft,__u32 *pprefered_lft) +__u32 *pvalid_lft,__u32 *pprefered_lft, +struct netlink_ext_ack *extack) {struct nlattr *tb[IFA_MAX +1 ];struct in_ifaddr *ifa;
@@ -792,7 +793,7 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,int err; err =nlmsg_parse (nlh,sizeof (*ifm), tb,IFA_MAX , ifa_ipv4_policy, -NULL ); + extack);if (err <0 ) goto errout;
@@ -897,7 +898,7 @@ static int inet_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,ASSERT_RTNL (); - ifa =rtm_to_ifaddr (net, nlh, &valid_lft, &prefered_lft); + ifa =rtm_to_ifaddr (net, nlh, &valid_lft, &prefered_lft, extack);if (IS_ERR (ifa))return PTR_ERR (ifa);
@@ -1684,7 +1685,7 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) s_ip_idx = ip_idx = cb->args[2 ];if (nlmsg_parse (cb->nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , - ifa_ipv4_policy,NULL ) >=0 ) { + ifa_ipv4_policy, cb->extack) >=0 ) {if (tb[IFA_TARGET_NETNSID ]) { fillargs.netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]);
net/ipv6/addrconf.c +1 -1
@@ -5021,7 +5021,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb, s_ip_idx = ip_idx = cb->args[2 ];if (nlmsg_parse (cb->nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , - ifa_ipv6_policy,NULL ) >=0 ) { + ifa_ipv6_policy, cb->extack) >=0 ) {if (tb[IFA_TARGET_NETNSID ]) { netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]);
net/ipv6/route.c +1 -1
@@ -4137,7 +4137,7 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,int err; err =nlmsg_parse (nlh,sizeof (*rtm), tb,RTA_MAX , rtm_ipv6_policy, -NULL ); + extack);if (err <0 ) goto errout;
net/mpls/af_mpls.c +1 -1
@@ -1223,7 +1223,7 @@ static int mpls_netconf_get_devconf(struct sk_buff *in_skb,int err; err =nlmsg_parse (nlh,sizeof (*ncm), tb,NETCONFA_MAX , - devconf_mpls_policy,NULL ); + devconf_mpls_policy, extack);if (err <0 ) goto errout;
net/netfilter/ipvs/ip_vs_ctl.c +1 -1
@@ -3234,7 +3234,7 @@ static int ip_vs_genl_dump_dests(struct sk_buff *skb,/* Try to find the service for which to dump destinations */ if (nlmsg_parse (cb->nlh,GENL_HDRLEN , attrs,IPVS_CMD_ATTR_MAX , - ip_vs_cmd_policy,NULL )) + ip_vs_cmd_policy, cb->extack)) goto out_err;
net/sched/act_api.c +1 -1
@@ -1452,7 +1452,7 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)u32 act_count =0 ; ret =nlmsg_parse (cb->nlh,sizeof (struct tcamsg ), tb,TCA_ROOT_MAX , - tcaa_policy,NULL ); + tcaa_policy, cb->extack);if (ret <0 )return ret;
net/sched/cls_api.c +4 -2
@@ -1727,7 +1727,8 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)if (nlmsg_len (cb->nlh) <sizeof (*tcm))return skb->len; - err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL , + cb->extack);if (err)return err;
@@ -2054,7 +2055,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)if (nlmsg_len (cb->nlh) <sizeof (*tcm))return skb->len; - err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL , + cb->extack);if (err)return err;
net/sched/sch_api.c +2 -1
@@ -1656,7 +1656,8 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb) idx =0 ;ASSERT_RTNL (); - err =nlmsg_parse (nlh,sizeof (struct tcmsg ), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (nlh,sizeof (struct tcmsg ), tca,TCA_MAX ,NULL , + cb->extack);if (err <0 )return err;
net/xfrm/xfrm_user.c +1 -1
@@ -1007,7 +1007,7 @@ static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)int err; err =nlmsg_parse (cb->nlh,0 , attrs,XFRMA_MAX , xfrma_policy, -NULL ); + cb->extack);if (err <0 )return err;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 4/20 net/ipv6: Refactor address dump to push inet6_fill_args to in6_dump_addrs
In reply to David AhernFrom: David Ahern <[email protected]>
Pull the inet6_fill_args arg up to in6_dump_addrs and move netnsid into it.
--- net/ipv6/addrconf.c | 57 ++++++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 27 deletions(-)
net/ipv6/addrconf.c +30 -27
@@ -4793,12 +4793,19 @@ static inline int inet6_ifaddr_msgsize(void) +nla_total_size (4 )/* IFA_RT_PRIORITY */ ; } +enum addr_type_t { +UNICAST_ADDR , +MULTICAST_ADDR , +ANYCAST_ADDR , +}; +struct inet6_fill_args {u32 portid;u32 seq;int event;unsigned int flags;int netnsid; +enum addr_type_t type; }; static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
@@ -4930,39 +4937,28 @@ static int inet6_fill_ifacaddr(struct sk_buff *skb, struct ifacaddr6 *ifaca,return 0 ; } -enum addr_type_t { -UNICAST_ADDR , -MULTICAST_ADDR , -ANYCAST_ADDR , -}; -/* called with rcu_read_lock() */ static int in6_dump_addrs (struct inet6_dev *idev,struct sk_buff *skb, -struct netlink_callback *cb,enum addr_type_t type, -int s_ip_idx,int *p_ip_idx,int netnsid) +struct netlink_callback *cb, +int s_ip_idx,int *p_ip_idx, +struct inet6_fill_args *fillargs) { -struct inet6_fill_args fillargs = { - .portid =NETLINK_CB (cb->skb).portid, - .seq = cb->nlh->nlmsg_seq, - .flags =NLM_F_MULTI , - .netnsid = netnsid, - };struct ifmcaddr6 *ifmca;struct ifacaddr6 *ifaca;int err =1 ;int ip_idx = *p_ip_idx;read_lock_bh (&idev->lock); -switch (type) { +switch (fillargs->type) {case UNICAST_ADDR : {struct inet6_ifaddr *ifa; - fillargs.event =RTM_NEWADDR ; + fillargs->event =RTM_NEWADDR ;/* unicast address incl. temp addr */ list_for_each_entry (ifa, &idev->addr_list, if_list) {if (++ip_idx < s_ip_idx)continue ; - err =inet6_fill_ifaddr (skb, ifa, &fillargs); + err =inet6_fill_ifaddr (skb, ifa, fillargs);if (err <0 )break ;nl_dump_check_consistent (cb,nlmsg_hdr (skb));
@@ -4970,26 +4966,26 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,break ; }case MULTICAST_ADDR : - fillargs.event =RTM_GETMULTICAST ; + fillargs->event =RTM_GETMULTICAST ;/* multicast address */ for (ifmca = idev->mc_list; ifmca; ifmca = ifmca->next, ip_idx++) {if (ip_idx < s_ip_idx)continue ; - err =inet6_fill_ifmcaddr (skb, ifmca, &fillargs); + err =inet6_fill_ifmcaddr (skb, ifmca, fillargs);if (err <0 )break ; }break ;case ANYCAST_ADDR : - fillargs.event =RTM_GETANYCAST ; + fillargs->event =RTM_GETANYCAST ;/* anycast address */ for (ifaca = idev->ac_list; ifaca; ifaca = ifaca->aca_next, ip_idx++) {if (ip_idx < s_ip_idx)continue ; - err =inet6_fill_ifacaddr (skb, ifaca, &fillargs); + err =inet6_fill_ifacaddr (skb, ifaca, fillargs);if (err <0 )break ; }
@@ -5005,10 +5001,16 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,static int inet6_dump_addr (struct sk_buff *skb,struct netlink_callback *cb,enum addr_type_t type) { +struct inet6_fill_args fillargs = { + .portid =NETLINK_CB (cb->skb).portid, + .seq = cb->nlh->nlmsg_seq, + .flags =NLM_F_MULTI , + .netnsid =-1 , + .type = type, + };struct net *net =sock_net (skb->sk);struct nlattr *tb[IFA_MAX +1 ];struct net *tgt_net = net; -int netnsid =-1 ;int h, s_h;int idx, ip_idx;int s_idx, s_ip_idx;
@@ -5023,9 +5025,10 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,if (nlmsg_parse (cb->nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , ifa_ipv6_policy, cb->extack) >=0 ) {if (tb[IFA_TARGET_NETNSID ]) { - netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]); + fillargs.netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]); - tgt_net =rtnl_get_net_ns_capable (skb->sk, netnsid); + tgt_net =rtnl_get_net_ns_capable (skb->sk, + fillargs.netnsid);if (IS_ERR (tgt_net))return PTR_ERR (tgt_net); }
@@ -5046,8 +5049,8 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb,if (!idev) goto cont; -if (in6_dump_addrs (idev, skb, cb, type, - s_ip_idx, &ip_idx, netnsid) <0 ) +if (in6_dump_addrs (idev, skb, cb, s_ip_idx, &ip_idx, + &fillargs) <0 ) goto done; cont: idx++;
@@ -5058,7 +5061,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb, cb->args[0 ] = h; cb->args[1 ] = idx; cb->args[2 ] = ip_idx; -if (netnsid >=0 ) +if (fillargs.netnsid >=0 )put_net (tgt_net);return skb->len;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 6/20 net/ipv4: Update inet_dump_ifaddr for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
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).
--- net/ipv4/devinet.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 10 deletions(-)
net/ipv4/devinet.c +43 -10
@@ -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 ; + } } }
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 5/20 netlink: Add new socket option to enable strict checking on dumps
In reply to David AhernFrom: David Ahern <[email protected]>
Add a new socket option, NETLINK_DUMP_STRICT_CHK, that userspace can use via setsockopt to request strict checking of headers and attributes on dump requests.
To get dump features such as kernel side filtering based on data in the header or attributes appended to the dump request, userspace must call setsockopt() for NETLINK_DUMP_STRICT_CHK and a non-zero value. Since the netlink sock and its flags are private to the af_netlink code, the strict checking flag is passed to dump handlers via a flag in the netlink_callback struct.
For old userspace on new kernel there is no impact as all of the data checks in later patches are wrapped in a check on the new strict flag.
For new userspace on old kernel, the setsockopt will fail and even if new userspace sets data in the headers and appended attributes the kernel will silently ignore it.
New userspace on new kernel setting the socket option gets the benefit of the improved data dump.
Kernel side the NETLINK_DUMP_STRICT_CHK uapi is converted to a generic NETLINK_F_STRICT_CHK flag which can potentially be leveraged for tighter checking on the NEW, DEL, and SET commands.
--- include/linux/netlink.h | 2 ++ include/uapi/linux/netlink.h | 1 + net/netlink/af_netlink.c | 21 ++++++++++++++++++++- net/netlink/af_netlink.h | 1 + 4 files changed, 24 insertions(+), 1 deletion(-)
include/linux/netlink.h +2 -0
@@ -179,6 +179,8 @@ struct netlink_callback {struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc; + unsigned int strict_check:1, + unused:31;unsigned int prev_seq, seq;long args[6 ]; };
include/uapi/linux/netlink.h +1 -0
@@ -155,6 +155,7 @@ enum nlmsgerr_attrs {#define NETLINK_LIST_MEMBERSHIPS 9#define NETLINK_CAP_ACK 10#define NETLINK_EXT_ACK 11 +#define NETLINK_DUMP_STRICT_CHK 12struct nl_pktinfo {__u32 group;
net/netlink/af_netlink.c +20 -1
@@ -1706,6 +1706,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname, nlk->flags &= ~NETLINK_F_EXT_ACK ; err =0 ;break ; +case NETLINK_DUMP_STRICT_CHK : +if (val) + nlk->flags |=NETLINK_F_STRICT_CHK ; +else + nlk->flags &= ~NETLINK_F_STRICT_CHK ; + err =0 ; +break ;default : err = -ENOPROTOOPT ; }
@@ -1799,6 +1806,15 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,return -EFAULT ; err =0 ;break ; +case NETLINK_DUMP_STRICT_CHK : +if (len <sizeof (int )) +return -EINVAL ; + len =sizeof (int ); + val = nlk->flags &NETLINK_F_STRICT_CHK ?1 :0 ; +if (put_user (len, optlen) ||put_user (val, optval)) +return -EFAULT ; + err =0 ; +break ;default : err = -ENOPROTOOPT ; }
@@ -2282,9 +2298,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,const struct nlmsghdr *nlh, struct netlink_dump_control *control) { + struct netlink_sock *nlk, *nlk2;struct netlink_callback *cb;struct sock *sk; -struct netlink_sock *nlk;int ret;refcount_inc (&skb->users);
@@ -2318,6 +2334,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb, cb->min_dump_alloc = control->min_dump_alloc; cb->skb = skb; + nlk2 =nlk_sk (NETLINK_CB (skb).sk); + cb->strict_check = !!(nlk2->flags &NETLINK_F_STRICT_CHK ); +if (control->start) { ret = control->start (cb);if (ret)
net/netlink/af_netlink.h +1 -0
@@ -15,6 +15,7 @@#define NETLINK_F_LISTEN_ALL_NSID 0x10#define NETLINK_F_CAP_ACK 0x20#define NETLINK_F_EXT_ACK 0x40 +#define NETLINK_F_STRICT_CHK 0x80#define NLGRPSZ (x) (ALIGN(x, sizeof(unsigned long) * 8) / 8)#define NLGRPLONGS (x) (NLGRPSZ(x)/sizeof(unsigned long))
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 7/20 net/ipv6: Update inet6_dump_addr for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update inet6_dump_addr 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 suppored 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 add support for other fields (e.g., honor ifa_index and only return data for the given device index).
--- net/ipv6/addrconf.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-)
net/ipv6/addrconf.c +42 -9
@@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,static int inet6_dump_addr (struct sk_buff *skb,struct netlink_callback *cb,enum addr_type_t type) { +struct netlink_ext_ack *extack = cb->extack; +const struct nlmsghdr *nlh = cb->nlh;struct inet6_fill_args fillargs = { .portid =NETLINK_CB (cb->skb).portid, .seq = cb->nlh->nlmsg_seq,
@@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb, .type = type, };struct net *net =sock_net (skb->sk); -struct nlattr *tb[IFA_MAX +1 ];struct net *tgt_net = net;int h, s_h;int idx, ip_idx;
@@ -5022,15 +5023,47 @@ static int inet6_dump_addr(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_ipv6_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 ; + } - tgt_net =rtnl_get_net_ns_capable (skb->sk, - fillargs.netnsid); -if (IS_ERR (tgt_net)) -return PTR_ERR (tgt_net); + err =nlmsg_parse (nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , + ifa_ipv6_policy, extack); +if (err <0 ) +return err; + +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 ; + } } }
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 8/20 rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
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.
--- net/core/rtnetlink.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 28 deletions(-)
net/core/rtnetlink.c +69 -28
@@ -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);
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 9/20 rtnetlink: Update rtnl_bridge_getlink for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
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.
--- net/core/rtnetlink.c | 50 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-)
net/core/rtnetlink.c +40 -10
@@ -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;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 10/20 rtnetlink: Update rtnl_stats_dump for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update rtnl_stats_dump for strict data checking. If the flag is set, the dump request is expected to have an if_stats_msg struct as the header. All elements of the struct are expected to be 0 except filter_mask which must be non-0 (legacy behavior). No attributes are supported.
--- net/core/rtnetlink.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)
net/core/rtnetlink.c +22 -2
@@ -4643,6 +4643,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh,static int rtnl_stats_dump (struct sk_buff *skb,struct netlink_callback *cb) { +struct netlink_ext_ack *extack = cb->extack;int h, s_h, err, s_idx, s_idxattr, s_prividx;struct net *net =sock_net (skb->sk);unsigned int flags =NLM_F_MULTI ;
@@ -4659,13 +4660,32 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb) cb->seq = net->dev_base_seq; -if (nlmsg_len (cb->nlh) <sizeof (*ifsm)) +if (nlmsg_len (cb->nlh) <sizeof (*ifsm)) { +NL_SET_ERR_MSG (extack,"Invalid header" );return -EINVAL ; + } ifsm =nlmsg_data (cb->nlh); + +/* only requests using NLM_F_DUMP_PROPER_HDR can pass data to +* influence the dump. The legacy exception is filter_mask. +*/ +if (cb->strict_check) { +if (ifsm->pad1 || ifsm->pad2 || ifsm->ifindex) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; + } +if (cb->nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifsm))) { +NL_SET_ERR_MSG (extack,"Invalid attributes after header" ); +return -EINVAL ; + } + } + filter_mask = ifsm->filter_mask; -if (!filter_mask) +if (!filter_mask) { +NL_SET_ERR_MSG (extack,"Invalid filter_mask" );return -EINVAL ; + }for (h = s_h; h <NETDEV_HASHENTRIES ; h++, s_idx =0 ) { idx =0 ;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 11/20 rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update inet6_dump_ifinfo for strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/ipv6/addrconf.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
net/ipv6/addrconf.c +35 -0
@@ -5628,6 +5628,31 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,return -EMSGSIZE ; } +static int inet6_valid_dump_ifinfo (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int inet6_dump_ifinfo (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);
@@ -5637,6 +5662,16 @@ static int inet6_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)struct inet6_dev *idev;struct hlist_head *head; +/* only requests using strict checking can pass data to +* influence the dump +*/ +if (cb->strict_check) { +int err =inet6_valid_dump_ifinfo (cb->nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_idx = cb->args[1 ];
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 12/20 rtnetlink: Update ipmr_rtm_dumplink for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update ipmr_rtm_dumplink for strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/ipv4/ipmr.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
net/ipv4/ipmr.c +32 -0
@@ -2710,6 +2710,31 @@ static bool ipmr_fill_vif(struct mr_table *mrt, u32 vifid, struct sk_buff *skb)return true; } +static int ipmr_valid_dumplink (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int ipmr_rtm_dumplink (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);
@@ -2718,6 +2743,13 @@ static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb)unsigned int e =0 , s_e;struct mr_table *mrt; +if (cb->strict_check) { +int err =ipmr_valid_dumplink (cb->nlh, cb->extack); + +if (err) +return err; + } + s_t = cb->args[0 ]; s_e = cb->args[1 ];
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 13/20 rtnetlink: Update fib dumps for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Add helper to check netlink message for route dumps. If the strict flag is set the dump request is expected to have an rtmsg struct as the header. All elements of the struct are expected to be 0 with the exception of rtm_flags (which is used by both ipv4 and ipv6 dumps) and no attributes can be appended. rtm_flags can only have RTM_F_CLONED and RTM_F_PREFIX set.
Update inet_dump_fib, inet6_dump_fib, mpls_dump_routes, ipmr_rtm_dumproute, and ip6mr_rtm_dumproute to call this helper if strict data checking is enabled.
--- include/net/ip_fib.h | 2 ++ net/ipv4/fib_frontend.c | 43 +++++++++++++++++++++++++++++++++++++++++-- net/ipv4/ipmr.c | 9 +++++++++ net/ipv6/ip6_fib.c | 8 ++++++++ net/ipv6/ip6mr.c | 9 +++++++++ net/mpls/af_mpls.c | 8 ++++++++ 6 files changed, 77 insertions(+), 2 deletions(-)
include/net/ip_fib.h +2 -0
@@ -452,4 +452,6 @@ static inline void fib_proc_exit(struct net *net)u32 ip_mtu_from_fib_result (struct fib_result *res,__be32 daddr); +int ip_valid_fib_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack);#endif /* _NET_FIB_H */
net/ipv4/fib_frontend.c +41 -2
57 lines not on this page · raw message
net/ipv4/ipmr.c +9 -0
@@ -2527,6 +2527,15 @@ static int ipmr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,static int ipmr_rtm_dumproute (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh; + +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } +return mr_rtm_dumproute (skb, cb, ipmr_mr_table_iter, _ipmr_fill_mroute, &mfc_unres_lock); }
net/ipv6/ip6_fib.c +8 -0
@@ -568,6 +568,7 @@ static int fib6_dump_table(struct fib6_table *table, struct sk_buff *skb,static int inet6_dump_fib (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);unsigned int h, s_h;unsigned int e =0 , s_e;
@@ -577,6 +578,13 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)struct hlist_head *head;int res =0 ; +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_e = cb->args[1 ];
net/ipv6/ip6mr.c +9 -0
@@ -2457,6 +2457,15 @@ static void mrt6msg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt)static int ip6mr_rtm_dumproute (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh; + +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } +return mr_rtm_dumproute (skb, cb, ip6mr_mr_table_iter, _ip6mr_fill_mroute, &mfc_unres_lock); }
net/mpls/af_mpls.c +8 -0
@@ -2017,6 +2017,7 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event,static int mpls_dump_routes (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct mpls_route __rcu **platform_label;size_t platform_labels;
@@ -2024,6 +2025,13 @@ static int mpls_dump_routes(struct sk_buff *skb, struct netlink_callback *cb)ASSERT_RTNL (); +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } + index = cb->args[0 ];if (index <MPLS_LABEL_FIRST_UNRESERVED ) index =MPLS_LABEL_FIRST_UNRESERVED ;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 14/20 net/neighbor: Update neigh_dump_info for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update neigh_dump_info for strict data checking. If the flag is set, the dump request is expected to have an ndmsg 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 NDA_IFINDEX and NDA_MASTER attributes are supported.
Existing code does not fail the dump if nlmsg_parse fails. That behavior is kept for non-strict checking.
--- net/core/neighbour.c | 59 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 11 deletions(-)
net/core/neighbour.c +48 -11
@@ -2428,13 +2428,14 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,static int neigh_dump_info (struct sk_buff *skb,struct netlink_callback *cb) { +struct netlink_ext_ack *extack = cb->extack;const struct nlmsghdr *nlh = cb->nlh;struct neigh_dump_filter filter = {};struct nlattr *tb[NDA_MAX +1 ];struct neigh_table *tbl;int t, family, s_t;int proxy =0 ; -int err; +int err, i; family = ((struct rtgenmsg *)nlmsg_data (nlh))->rtgen_family;
@@ -2445,20 +2446,56 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY) proxy =1 ; - err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , - cb->extack); -if (!err) { -if (tb[NDA_IFINDEX ]) { -if (nla_len (tb[NDA_IFINDEX ]) !=sizeof (u32)) -return -EINVAL ; - filter.dev_idx =nla_get_u32 (tb[NDA_IFINDEX ]); +if (cb->strict_check) { +struct ndmsg *ndm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ndm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ndm =nlmsg_data (nlh); +if (ndm->ndm_pad1 || ndm->ndm_pad2 || ndm->ndm_ifindex || + ndm->ndm_state || ndm->ndm_flags || ndm->ndm_type) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; } -if (tb[NDA_MASTER ]) { -if (nla_len (tb[NDA_MASTER ]) !=sizeof (u32)) + } + + err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , extack); +if (err <0 ) { +if (cb->strict_check) +return -EINVAL ; + goto walk_entries; + } + +for (i =0 ; i <=NDA_MAX ; ++i) { +if (!tb[i]) +continue ; +switch (i) { +case NDA_IFINDEX : +if (nla_len (tb[i]) !=sizeof (u32)) { +NL_SET_ERR_MSG (extack,"Invalid IFINDEX attribute" );return -EINVAL ; - filter.master_idx =nla_get_u32 (tb[NDA_MASTER ]); + } + filter.dev_idx =nla_get_u32 (tb[i]); +break ; +case NDA_MASTER : +if (nla_len (tb[i]) !=sizeof (u32)) { +NL_SET_ERR_MSG (extack,"Invalid MASTER attribute" ); +return -EINVAL ; + } + filter.master_idx =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: s_t = cb->args[0 ];for (t =0 ; t <NEIGH_NR_TABLES ; t++) {
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 15/20 net/neighbor: Update neightbl_dump_info for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update neightbl_dump_info for strict data checking. If the flag is set, the dump request is expected to have an ndtmsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/core/neighbour.c | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-)
net/core/neighbour.c +35 -3
@@ -2164,15 +2164,47 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,return err; } +static int neightbl_valid_dump_info (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ndtmsg *ndtm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ndtm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ndtm =nlmsg_data (nlh); +if (ndtm->ndtm_pad1 || ndtm->ndtm_pad2) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ndtm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int neightbl_dump_info (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int family, tidx, nidx =0 ;int tbl_skip = cb->args[0 ];int neigh_skip = cb->args[1 ];struct neigh_table *tbl; - family = ((struct rtgenmsg *)nlmsg_data (cb->nlh))->rtgen_family; +if (cb->strict_check) { +int err =neightbl_valid_dump_info (nlh, cb->extack); + +if (err) +return err; + } + + family = ((struct rtgenmsg *)nlmsg_data (nlh))->rtgen_family;for (tidx =0 ; tidx <NEIGH_NR_TABLES ; tidx++) {struct neigh_parms *p;
@@ -2185,7 +2217,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)continue ;if (neightbl_fill_info (skb, tbl,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq,RTM_NEWNEIGHTBL , + nlh->nlmsg_seq,RTM_NEWNEIGHTBL ,NLM_F_MULTI ) <0 )break ;
@@ -2200,7 +2232,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)if (neightbl_fill_param_info (skb, tbl, p,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNEIGHTBL ,NLM_F_MULTI ) <0 ) goto out;
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 16/20 net/namespace: Update rtnl_net_dumpid for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update rtnl_net_dumpid for strict data checking. If the flag is set, the dump request is expected to have an rtgenmsg struct as the header which has the family as the only element. No data may be appended.
--- net/core/net_namespace.c | 8 ++++++++ 1 file changed, 8 insertions(+)
net/core/net_namespace.c +8 -0
@@ -844,6 +844,7 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)static int rtnl_net_dumpid (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct rtnl_net_dump_cb net_cb = { .net = net,
@@ -853,6 +854,13 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb) .s_idx = cb->args[0], }; +if (cb->strict_check) { +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (struct rtgenmsg ))) { +NL_SET_ERR_MSG (cb->extack,"Unknown data in dump request" ); +return -EINVAL ; + } + } +spin_lock_bh (&net->nsid_lock);idr_for_each (&net->netns_ids, rtnl_net_dumpid_one, &net_cb);spin_unlock_bh (&net->nsid_lock);
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 17/20 net/fib_rules: Update fib_nl_dumprule for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update fib_nl_dumprule for strict data checking. If the flag is set, the dump request is expected to have fib_rule_hdr struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/core/fib_rules.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)
net/core/fib_rules.c +35 -1
@@ -1063,13 +1063,47 @@ static int dump_rules(struct sk_buff *skb, struct netlink_callback *cb,return err; } +static int fib_valid_dumprule (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct fib_rule_hdr *frh; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*frh))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + frh =nlmsg_data (nlh); +if (frh->dst_len || frh->src_len || frh->tos || frh->table || + frh->res1 || frh->res2 || frh->action || frh->flags) { +NL_SET_ERR_MSG (extack, +"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*frh))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int fib_nl_dumprule (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct fib_rules_ops *ops;int idx =0 , family; - family =rtnl_msg_family (cb->nlh); +if (cb->strict_check) { +int err =fib_valid_dumprule (nlh, cb->extack); + +if (err) +return err; + } + + family =rtnl_msg_family (nlh);if (family !=AF_UNSPEC ) {/* Protocol specific dump request */ ops =lookup_rules_ops (net, family);
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 18/20 net/ipv6: Update ip6addrlbl_dump for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update ip6addrlbl_dump for strict data checking. If the flag is set, the dump request is expected to have an ifaddrlblmsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/ipv6/addrlabel.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
net/ipv6/addrlabel.c +34 -1
@@ -458,20 +458,53 @@ static int ip6addrlbl_fill(struct sk_buff *skb,return 0 ; } +static int ip6addrlbl_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifaddrlblmsg *ifal; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifal))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ifal =nlmsg_data (nlh); +if (ifal->__ifal_reserved || ifal->ifal_prefixlen || + ifal->ifal_flags || ifal->ifal_index || ifal->ifal_seq) { +NL_SET_ERR_MSG (extack, +"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ifal))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int ip6addrlbl_dump (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct ip6addrlbl_entry *p;int idx =0 , s_idx = cb->args[0 ];int err; +if (cb->strict_check) { + err =ip6addrlbl_valid_dump_req (nlh, cb->extack); +if (err) +return err; + } +rcu_read_lock ();hlist_for_each_entry_rcu (p, &net->ipv6.ip6addrlbl_table.head, list) {if (idx >= s_idx) { err =ip6addrlbl_fill (skb, p, net->ipv6.ip6addrlbl_table.seq,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWADDRLABEL ,NLM_F_MULTI );if (err <0 )
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 19/20 net: Update netconf dump handlers for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update inet_netconf_dump_devconf, inet6_netconf_dump_devconf, and mpls_netconf_dump_devconf for strict data checking. If the flag is set, the dump request is expected to have an netconfmsg struct as the header. The struct only has the family member and no attributes can be appended.
--- net/ipv4/devinet.c | 22 +++++++++++++++++++--- net/ipv6/addrconf.c | 22 +++++++++++++++++++--- net/mpls/af_mpls.c | 18 +++++++++++++++++- 3 files changed, 55 insertions(+), 7 deletions(-)
net/ipv4/devinet.c +19 -3
@@ -2069,6 +2069,7 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,static int inet_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int h, s_h;int idx, s_idx;
@@ -2076,6 +2077,21 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,struct in_device *in_dev;struct hlist_head *head; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];
@@ -2095,7 +2111,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb, dev->ifindex, &in_dev->cnf,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {
@@ -2112,7 +2128,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb,NETCONFA_IFINDEX_ALL , net->ipv4.devconf_all,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;
@@ -2123,7 +2139,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb,NETCONFA_IFINDEX_DEFAULT , net->ipv4.devconf_dflt,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;
net/ipv6/addrconf.c +19 -3
@@ -666,6 +666,7 @@ static int inet6_netconf_get_devconf(struct sk_buff *in_skb,static int inet6_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int h, s_h;int idx, s_idx;
@@ -673,6 +674,21 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,struct inet6_dev *idev;struct hlist_head *head; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];
@@ -692,7 +708,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb, dev->ifindex, &idev->cnf,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {
@@ -709,7 +725,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb,NETCONFA_IFINDEX_ALL , net->ipv6.devconf_all,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;
@@ -720,7 +736,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb,NETCONFA_IFINDEX_DEFAULT , net->ipv6.devconf_dflt,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;
net/mpls/af_mpls.c +17 -1
@@ -1263,6 +1263,7 @@ static int mpls_netconf_get_devconf(struct sk_buff *in_skb,static int mpls_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct hlist_head *head;struct net_device *dev;
@@ -1270,6 +1271,21 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb,int idx, s_idx;int h, s_h; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];
@@ -1286,7 +1302,7 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb, goto cont;if (mpls_netconf_fill_devconf (skb, mdev,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {
Signature · 1 line
2.11.0
David Ahern
maintainerPATCH 20/20 net/bridge: Update br_mdb_dump for strict data checking
In reply to David AhernFrom: David Ahern <[email protected]>
Update br_mdb_dump for strict data checking. If the flag is set, the dump request is expected to have a br_port_msg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
--- net/bridge/br_mdb.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
net/bridge/br_mdb.c +29 -0
@@ -162,6 +162,28 @@ static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,return err; } +static int br_mdb_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct br_port_msg *bpm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } +if (bpm->ifindex) { +NL_SET_ERR_MSG (extack, +"Filtering by device index is not supported" ); +return -EINVAL ; + } +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int br_mdb_dump (struct sk_buff *skb,struct netlink_callback *cb) {struct net_device *dev;
@@ -169,6 +191,13 @@ static int br_mdb_dump(struct sk_buff *skb, struct netlink_callback *cb)struct nlmsghdr *nlh =NULL ;int idx =0 , s_idx; +if (cb->strict_check) { +int err =br_mdb_valid_dump_req (cb->nlh, cb->extack); + +if (err) +return err; + } + s_idx = cb->args[0 ];rcu_read_lock ();
Signature · 1 line
2.11.0
David Miller
maintainerRe: [PATCH net-next 20/20] net/bridge: Update br_mdb_dump for strict data checking
From: David Ahern <[email protected]>
Date: Thu, 4 Oct 2018 14:33:55 -0700
Quoted 4 lines
net/bridge/br_mdb.c +0
-0@@ -162,6 +162,28 @@ static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,return err; }
net/bridge/br_mdb.c static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb, · line 174
Quoted 10 lines
net/bridge/br_mdb.c +10
-0+static int br_mdb_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct br_port_msg *bpm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } +if (bpm->ifindex) {
'bpm' is never initialized.
Re: [PATCH net-next 20/20] net/bridge: Update br_mdb_dump for strict data checking
Quoted 7 lines · David Miller
From: David Ahern <[email protected]>
Date: Thu, 4 Oct 2018 14:33:55 -0700Quoted 4 lines
net/bridge/br_mdb.c +0
-0@@ -162,6 +162,28 @@ static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,return err; }
Quoted 13 lines
net/bridge/br_mdb.c static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb, · line 174
Quoted 10 lines
net/bridge/br_mdb.c +10
-0+static int br_mdb_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct br_port_msg *bpm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } +if (bpm->ifindex) {'bpm' is never initialized.
Thanks.
I had not updated the bridge command for strict checking. Doing so and bridge mdb show generates a trace. Will fix.
Re: [PATCH net-next 20/20] net/bridge: Update br_mdb_dump for strict data checking
In reply to David AhernQuoted 46 lines · David Ahern
From: David Ahern <[email protected]>
Update
br_mdb_dumpfor strict data checking. If the flag is set, the dump request is expected to have abr_port_msgstruct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>--- net/bridge/br_mdb.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)net/bridge/br_mdb.c +22
-0@@ -162,6 +162,28 @@ static int br_mdb_fill_info(struct sk_buff *skb, struct netlink_callback *cb,return err; } +static int br_mdb_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct br_port_msg *bpm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } +if (bpm->ifindex) { +NL_SET_ERR_MSG (extack, +"Filtering by device index is not supported" ); +return -EINVAL ; + } +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*bpm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int br_mdb_dump (struct sk_buff *skb,struct netlink_callback *cb) {struct net_device *dev;
net/bridge/br_mdb.c static int br_mdb_dump(struct sk_buff *skb, struct netlink_callback *cb) · line 198
Quoted 8 lines
net/bridge/br_mdb.c +5
-0@@ -169,6 +191,13 @@ static int br_mdb_dump(struct sk_buff *skb, struct netlink_callback *cb)struct nlmsghdr *nlh =NULL ;int idx =0 , s_idx; +if (cb->strict_check) { +int err =br_mdb_valid_dump_req (cb->nlh, cb->extack); + +if (err) +return err;
Nit: unnecessarry newline :)
Quoted 8 lines
net/bridge/br_mdb.c +2
-0+ } + s_idx = cb->args[0 ];rcu_read_lock ();Signature · 1 line
2.11.0
Re: [PATCH net-next 05/20] netlink: Add new socket option to enable strict checking on dumps
In reply to David AhernQuoted 40 lines · David Ahern
From: David Ahern <[email protected]>
Add a new socket option,
NETLINK_DUMP_STRICT_CHK, that userspace can use via setsockopt to request strict checking of headers and attributes on dump requests.To get dump features such as kernel side filtering based on data in the header or attributes appended to the dump request, userspace must call
setsockopt()forNETLINK_DUMP_STRICT_CHKand a non-zero value. Since the netlink sock and its flags are private to theaf_netlinkcode, the strict checking flag is passed to dump handlers via a flag in thenetlink_callbackstruct.For old userspace on new kernel there is no impact as all of the data checks in later patches are wrapped in a check on the new strict flag.
For new userspace on old kernel, the setsockopt will fail and even if new userspace sets data in the headers and appended attributes the kernel will silently ignore it.
New userspace on new kernel setting the socket option gets the benefit of the improved data dump.
Kernel side the
NETLINK_DUMP_STRICT_CHKuapi is converted to a genericNETLINK_F_STRICT_CHKflag which can potentially be leveraged for tighter checking on the NEW, DEL, and SET commands.Signed-off-by David Ahern <[email protected]>--- include/linux/netlink.h | 2 ++ include/uapi/linux/netlink.h | 1 + net/netlink/af_netlink.c | 21 ++++++++++++++++++++- net/netlink/af_netlink.h | 1 + 4 files changed, 24 insertions(+), 1 deletion(-)include/linux/netlink.h +0
-0
include/linux/netlink.h struct netlink_callback { · line 183
Quoted 5 lines
include/linux/netlink.h +2
-0@@ -179,6 +179,8 @@ struct netlink_callback {struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc; + unsigned int strict_check:1, + unused:31;
I like this idea a lot. :) but I'm not a fan of bitfields if not necessary. Is that really necessary here?
Quoted 85 lines
include/linux/netlink.h +0
-0unsigned int prev_seq, seq;long args[6 ]; };include/uapi/linux/netlink.h +1
-0@@ -155,6 +155,7 @@ enum nlmsgerr_attrs {#define NETLINK_LIST_MEMBERSHIPS 9#define NETLINK_CAP_ACK 10#define NETLINK_EXT_ACK 11 +#define NETLINK_DUMP_STRICT_CHK 12struct nl_pktinfo {__u32 group;net/netlink/af_netlink.c +20
-1@@ -1706,6 +1706,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname, nlk->flags &= ~NETLINK_F_EXT_ACK ; err =0 ;break ; +case NETLINK_DUMP_STRICT_CHK : +if (val) + nlk->flags |=NETLINK_F_STRICT_CHK ; +else + nlk->flags &= ~NETLINK_F_STRICT_CHK ; + err =0 ; +break ;default : err = -ENOPROTOOPT ; }@@ -1799,6 +1806,15 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,return -EFAULT ; err =0 ;break ; +case NETLINK_DUMP_STRICT_CHK : +if (len <sizeof (int )) +return -EINVAL ; + len =sizeof (int ); + val = nlk->flags &NETLINK_F_STRICT_CHK ?1 :0 ; +if (put_user (len, optlen) ||put_user (val, optval)) +return -EFAULT ; + err =0 ; +break ;default : err = -ENOPROTOOPT ; }@@ -2282,9 +2298,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,const struct nlmsghdr *nlh, struct netlink_dump_control *control) { + struct netlink_sock *nlk, *nlk2;struct netlink_callback *cb;struct sock *sk; -struct netlink_sock *nlk;int ret;refcount_inc (&skb->users);@@ -2318,6 +2334,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb, cb->min_dump_alloc = control->min_dump_alloc; cb->skb = skb; + nlk2 =nlk_sk (NETLINK_CB (skb).sk); + cb->strict_check = !!(nlk2->flags &NETLINK_F_STRICT_CHK ); +if (control->start) { ret = control->start (cb);if (ret)net/netlink/af_netlink.h +1
-0@@ -15,6 +15,7 @@#define NETLINK_F_LISTEN_ALL_NSID 0x10#define NETLINK_F_CAP_ACK 0x20#define NETLINK_F_EXT_ACK 0x40 +#define NETLINK_F_STRICT_CHK 0x80#define NLGRPSZ (x) (ALIGN(x, sizeof(unsigned long) * 8) / 8)#define NLGRPLONGS (x) (NLGRPSZ(x)/sizeof(unsigned long))Signature · 1 line
2.11.0
Re: [PATCH net-next 03/20] net: Add extack to nlmsg_parse
In reply to David AhernQuoted 7 lines · David Ahern
From: David Ahern <[email protected]>
Make sure extack is passed to
nlmsg_parsewhere easy to do so. Most of these are dump handlers and leveraging the extack in thenetlink_callback.Signed-off-by David Ahern <[email protected]>
Yeah, having extack in dump requests sounds really useful to me!
Quoted 224 lines
--- net/core/devlink.c | 2 +- net/core/neighbour.c | 3 ++- net/core/rtnetlink.c | 4 ++-- net/ipv4/devinet.c | 9 +++++---- net/ipv6/addrconf.c | 2 +- net/ipv6/route.c | 2 +- net/mpls/af_mpls.c | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 2 +- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 6 ++++-- net/sched/sch_api.c | 3 ++- net/xfrm/xfrm_user.c | 2 +- 12 files changed, 22 insertions(+), 17 deletions(-)net/core/devlink.c +1
-1@@ -3489,7 +3489,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct sk_buff *skb, start_offset = *((u64 *)&cb->args[0 ]); err =nlmsg_parse (cb->nlh,GENL_HDRLEN + devlink_nl_family.hdrsize, - attrs,DEVLINK_ATTR_MAX , ops->policy,NULL ); + attrs,DEVLINK_ATTR_MAX , ops->policy, cb->extack);if (err) goto out;net/core/neighbour.c +2
-1@@ -2445,7 +2445,8 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY) proxy =1 ; - err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL ,NULL ); + err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , + cb->extack);if (!err) {if (tb[NDA_IFINDEX ]) {if (nla_len (tb[NDA_IFINDEX ]) !=sizeof (u32))net/core/rtnetlink.c +2
-2@@ -1909,7 +1909,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) sizeof(struct rtgenmsg) : sizeof(struct ifinfomsg);if (nlmsg_parse (cb->nlh, hdrlen, tb,IFLA_MAX , - ifla_policy,NULL ) >=0 ) { + 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);@@ -3764,7 +3764,7 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)int fidx =0 ; err =nlmsg_parse (cb->nlh,sizeof (struct ifinfomsg ), tb, -IFLA_MAX , ifla_policy,NULL ); +IFLA_MAX , ifla_policy, cb->extack);if (err <0 ) {return -EINVAL ; }else if (err ==0 ) {net/ipv4/devinet.c +5
-4@@ -782,7 +782,8 @@ static void set_ifa_lifetime(struct in_ifaddr *ifa, __u32 valid_lft, }static struct in_ifaddr *rtm_to_ifaddr (struct net *net,struct nlmsghdr *nlh, -__u32 *pvalid_lft,__u32 *pprefered_lft) +__u32 *pvalid_lft,__u32 *pprefered_lft, +struct netlink_ext_ack *extack) {struct nlattr *tb[IFA_MAX +1 ];struct in_ifaddr *ifa;@@ -792,7 +793,7 @@ static struct in_ifaddr *rtm_to_ifaddr(struct net *net, struct nlmsghdr *nlh,int err; err =nlmsg_parse (nlh,sizeof (*ifm), tb,IFA_MAX , ifa_ipv4_policy, -NULL ); + extack);if (err <0 ) goto errout;@@ -897,7 +898,7 @@ static int inet_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,ASSERT_RTNL (); - ifa =rtm_to_ifaddr (net, nlh, &valid_lft, &prefered_lft); + ifa =rtm_to_ifaddr (net, nlh, &valid_lft, &prefered_lft, extack);if (IS_ERR (ifa))return PTR_ERR (ifa);@@ -1684,7 +1685,7 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) s_ip_idx = ip_idx = cb->args[2 ];if (nlmsg_parse (cb->nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , - ifa_ipv4_policy,NULL ) >=0 ) { + ifa_ipv4_policy, cb->extack) >=0 ) {if (tb[IFA_TARGET_NETNSID ]) { fillargs.netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]);net/ipv6/addrconf.c +1
-1@@ -5021,7 +5021,7 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb, s_ip_idx = ip_idx = cb->args[2 ];if (nlmsg_parse (cb->nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , - ifa_ipv6_policy,NULL ) >=0 ) { + ifa_ipv6_policy, cb->extack) >=0 ) {if (tb[IFA_TARGET_NETNSID ]) { netnsid =nla_get_s32 (tb[IFA_TARGET_NETNSID ]);net/ipv6/route.c +1
-1@@ -4137,7 +4137,7 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,int err; err =nlmsg_parse (nlh,sizeof (*rtm), tb,RTA_MAX , rtm_ipv6_policy, -NULL ); + extack);if (err <0 ) goto errout;net/mpls/af_mpls.c +1
-1@@ -1223,7 +1223,7 @@ static int mpls_netconf_get_devconf(struct sk_buff *in_skb,int err; err =nlmsg_parse (nlh,sizeof (*ncm), tb,NETCONFA_MAX , - devconf_mpls_policy,NULL ); + devconf_mpls_policy, extack);if (err <0 ) goto errout;net/netfilter/ipvs/ip_vs_ctl.c +1
-1@@ -3234,7 +3234,7 @@ static int ip_vs_genl_dump_dests(struct sk_buff *skb,/* Try to find the service for which to dump destinations */ if (nlmsg_parse (cb->nlh,GENL_HDRLEN , attrs,IPVS_CMD_ATTR_MAX , - ip_vs_cmd_policy,NULL )) + ip_vs_cmd_policy, cb->extack)) goto out_err;net/sched/act_api.c +1
-1@@ -1452,7 +1452,7 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)u32 act_count =0 ; ret =nlmsg_parse (cb->nlh,sizeof (struct tcamsg ), tb,TCA_ROOT_MAX , - tcaa_policy,NULL ); + tcaa_policy, cb->extack);if (ret <0 )return ret;net/sched/cls_api.c +4
-2@@ -1727,7 +1727,8 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)if (nlmsg_len (cb->nlh) <sizeof (*tcm))return skb->len; - err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL , + cb->extack);if (err)return err;@@ -2054,7 +2055,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)if (nlmsg_len (cb->nlh) <sizeof (*tcm))return skb->len; - err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (cb->nlh,sizeof (*tcm), tca,TCA_MAX ,NULL , + cb->extack);if (err)return err;net/sched/sch_api.c +2
-1@@ -1656,7 +1656,8 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb) idx =0 ;ASSERT_RTNL (); - err =nlmsg_parse (nlh,sizeof (struct tcmsg ), tca,TCA_MAX ,NULL ,NULL ); + err =nlmsg_parse (nlh,sizeof (struct tcmsg ), tca,TCA_MAX ,NULL , + cb->extack);if (err <0 )return err;net/xfrm/xfrm_user.c +1
-1@@ -1007,7 +1007,7 @@ static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)int err; err =nlmsg_parse (cb->nlh,0 , attrs,XFRMA_MAX , xfrma_policy, -NULL ); + cb->extack);if (err <0 )return err;Signature · 1 line
2.11.0
Re: [PATCH net-next 01/20] netlink: Pass extack to dump handlers
In reply to David AhernQuoted 9 lines · David Ahern
From: David Ahern <[email protected]>
Declare extack in
netlink_dumpand pass to dump handlers vianetlink_callback. Add any extack message after thedump_done_errnoallowing error messages to be returned. This will be useful when strict checking is done on dump requests, returning why the dump fails EINVAL.Signed-off-by David Ahern <[email protected]>
Quoted 58 lines
--- include/linux/netlink.h | 1 + net/netlink/af_netlink.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-)include/linux/netlink.h +1
-0@@ -176,6 +176,7 @@ struct netlink_callback {void *data;/* the module that dump function belong to */ struct module *module; +struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc;unsigned int prev_seq, seq;net/netlink/af_netlink.c +11
-1@@ -2171,6 +2171,7 @@ EXPORT_SYMBOL(__nlmsg_put);static int netlink_dump (struct sock *sk) {struct netlink_sock *nlk =nlk_sk (sk); +struct netlink_ext_ack extack = {};struct netlink_callback *cb;struct sk_buff *skb =NULL ;struct nlmsghdr *nlh;@@ -2222,8 +2223,11 @@ static int netlink_dump(struct sock *sk)skb_reserve (skb,skb_tailroom (skb) - alloc_size);netlink_skb_set_owner_r (skb, sk); -if (nlk->dump_done_errno >0 ) +if (nlk->dump_done_errno >0 ) { + cb->extack = &extack; nlk->dump_done_errno = cb->dump (skb, cb); + cb->extack =NULL ; + }if (nlk->dump_done_errno >0 ||skb_tailroom (skb) <nlmsg_total_size (sizeof (nlk->dump_done_errno))) {@@ -2246,6 +2250,12 @@ static int netlink_dump(struct sock *sk)memcpy (nlmsg_data (nlh), &nlk->dump_done_errno,sizeof (nlk->dump_done_errno)); +if (extack._msg && nlk->flags &NETLINK_F_EXT_ACK ) { + nlh->nlmsg_flags |=NLM_F_ACK_TLVS ; +if (!nla_put_string (skb,NLMSGERR_ATTR_MSG , extack._msg)) +nlmsg_end (skb, nlh); + } +if (sk_filter (sk, skb))kfree_skb (skb);else Signature · 1 line
2.11.0
Re: [PATCH net-next 02/20] netlink: Add extack message to nlmsg_parse for invalid header length
In reply to David AhernQuoted 5 lines · David Ahern
From: David Ahern <[email protected]>
Give a user a reason why EINVAL is returned in
nlmsg_parse.Signed-off-by David Ahern <[email protected]>
Quoted 23 lines
--- include/net/netlink.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)include/net/netlink.h +3
-1@@ -516,8 +516,10 @@ static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,const struct nla_policy *policy, struct netlink_ext_ack *extack) { - if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) + if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen)) { + NL_SET_ERR_MSG(extack, "Invalid header length");return -EINVAL ; + }return nla_parse (tb, maxtype,nlmsg_attrdata (nlh, hdrlen),nlmsg_attrlen (nlh, hdrlen), policy, extack);Signature · 1 line
2.11.0
Re: [PATCH net-next 16/20] net/namespace: Update rtnl_net_dumpid for strict data checking
In reply to David AhernQuoted 7 lines · David Ahern
From: David Ahern <[email protected]>
Update
rtnl_net_dumpidfor strict data checking. If the flag is set, the dump request is expected to have an rtgenmsg struct as the header which has the family as the only element. No data may be appended.Signed-off-by David Ahern <[email protected]>
Quoted 33 lines
--- net/core/net_namespace.c | 8 ++++++++ 1 file changed, 8 insertions(+)net/core/net_namespace.c +8
-0@@ -844,6 +844,7 @@ static int rtnl_net_dumpid_one(int id, void *peer, void *data)static int rtnl_net_dumpid (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct rtnl_net_dump_cb net_cb = { .net = net,@@ -853,6 +854,13 @@ static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb) .s_idx = cb->args[0], }; +if (cb->strict_check) { +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (struct rtgenmsg ))) { +NL_SET_ERR_MSG (cb->extack,"Unknown data in dump request" ); +return -EINVAL ; + } + } +spin_lock_bh (&net->nsid_lock);idr_for_each (&net->netns_ids, rtnl_net_dumpid_one, &net_cb);spin_unlock_bh (&net->nsid_lock);Signature · 1 line
2.11.0
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to David AhernQuoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_ifinfofor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
This is on top of current net-next? Are your patches ensuring that ipv6 addr requests don't generate log messages anymore when a wrong header is passed but the strict socket option is not passed? The context here doesn't seem to indicate that. :)
Quoted 60 lines
--- net/ipv6/addrconf.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)net/ipv6/addrconf.c +35
-0@@ -5628,6 +5628,31 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,return -EMSGSIZE ; } +static int inet6_valid_dump_ifinfo (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int inet6_dump_ifinfo (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);@@ -5637,6 +5662,16 @@ static int inet6_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)struct inet6_dev *idev;struct hlist_head *head; +/* only requests using strict checking can pass data to +* influence the dump +*/ +if (cb->strict_check) { +int err =inet6_valid_dump_ifinfo (cb->nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_idx = cb->args[1 ];Signature · 1 line
2.11.0
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
Quoted 8 lines · Christian Brauner
Quoted 7 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_ifinfofor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
Quoted 6 lines
Quoted 1 line
Signed-off-by David Ahern <[email protected]>This is on top of current net-next? Are your patches ensuring that ipv6 addr requests don't generate log messages anymore when a wrong header is passed but the strict socket option is not passed? The context here doesn't seem to indicate that. :)
Nm, 9/10 takes care of that. Thanks! :)
Quoted 61 lines
Quoted 61 lines
--- net/ipv6/addrconf.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)net/ipv6/addrconf.c +35
-0@@ -5628,6 +5628,31 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,return -EMSGSIZE ; } +static int inet6_valid_dump_ifinfo (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int inet6_dump_ifinfo (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);@@ -5637,6 +5662,16 @@ static int inet6_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)struct inet6_dev *idev;struct hlist_head *head; +/* only requests using strict checking can pass data to +* influence the dump +*/ +if (cb->strict_check) { +int err =inet6_valid_dump_ifinfo (cb->nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_idx = cb->args[1 ];Signature · 1 line
2.11.0
Re: [PATCH net-next 07/20] net/ipv6: Update inet6_dump_addr for strict data checking
In reply to David AhernQuoted 11 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_addrfor 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 suppored by the dump handler are allowed to be non-0 or set in the request. At the moment only theIFA_TARGET_NETNSIDattribute is supported. Follow on patches can add support for other fields (e.g., honorifa_indexand only return data for the given device index).
Quoted 1 line
Signed-off-by David Ahern <[email protected]>
Quoted 84 lines
--- net/ipv6/addrconf.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-)net/ipv6/addrconf.c +42
-9@@ -5001,6 +5001,8 @@ static int in6_dump_addrs(struct inet6_dev *idev, struct sk_buff *skb,static int inet6_dump_addr (struct sk_buff *skb,struct netlink_callback *cb,enum addr_type_t type) { +struct netlink_ext_ack *extack = cb->extack; +const struct nlmsghdr *nlh = cb->nlh;struct inet6_fill_args fillargs = { .portid =NETLINK_CB (cb->skb).portid, .seq = cb->nlh->nlmsg_seq,@@ -5009,7 +5011,6 @@ static int inet6_dump_addr(struct sk_buff *skb, struct netlink_callback *cb, .type = type, };struct net *net =sock_net (skb->sk); -struct nlattr *tb[IFA_MAX +1 ];struct net *tgt_net = net;int h, s_h;int idx, ip_idx;@@ -5022,15 +5023,47 @@ static int inet6_dump_addr(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_ipv6_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 ; + } - tgt_net =rtnl_get_net_ns_capable (skb->sk, - fillargs.netnsid); -if (IS_ERR (tgt_net)) -return PTR_ERR (tgt_net); + err =nlmsg_parse (nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , + ifa_ipv6_policy, extack); +if (err <0 ) +return err; + +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 ; + } } }Signature · 1 line
2.11.0
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to David AhernQuoted 21 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_ifinfofor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>--- net/ipv6/addrconf.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)net/ipv6/addrconf.c +1
-0@@ -5628,6 +5628,31 @@ static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev,return -EMSGSIZE ; } +static int inet6_valid_dump_ifinfo(const struct nlmsghdr *nlh,
net/ipv6/addrconf.c static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev, · line 5641
Quoted 10 lines
net/ipv6/addrconf.c +10
-0+ struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) {
Shouldn't ipv6 specific dump requests at least support IFA_TARGET_NETNSID?
Quoted 37 lines
net/ipv6/addrconf.c +24
-0+NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int inet6_dump_ifinfo (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);@@ -5637,6 +5662,16 @@ static int inet6_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)struct inet6_dev *idev;struct hlist_head *head; +/* only requests using strict checking can pass data to +* influence the dump +*/ +if (cb->strict_check) { +int err =inet6_valid_dump_ifinfo (cb->nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_idx = cb->args[1 ];Signature · 1 line
2.11.0
Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to David AhernQuoted 123 lines · David Ahern
From: David Ahern <[email protected]>
Update
rtnl_dump_ifinfofor 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 theIFA_TARGET_NETNSID,IFLA_EXT_MASK,IFLA_MASTER, andIFLA_LINKINFOattributes are supported.Existing code does not fail the dump if
nlmsg_parsefails. That behavior is kept for non-strict checking.Signed-off-by David Ahern <[email protected]>--- net/core/rtnetlink.c | 97 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 28 deletions(-)net/core/rtnetlink.c +54
-26@@ -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]);
net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1968
Quoted 10 lines
net/core/rtnetlink.c +10
-0+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(). :)
Quoted 22 lines
net/core/rtnetlink.c +5
-2} +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);Signature · 1 line
2.11.0
Re: [PATCH net-next 06/20] net/ipv4: Update inet_dump_ifaddr for strict data checking
In reply to David AhernQuoted 70 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet_dump_ifaddrfor 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 theIFA_TARGET_NETNSIDattribute is supported. Follow on patches can support for other fields (e.g., honorifa_indexand only return data for the given device index).Signed-off-by David Ahern <[email protected]>--- net/ipv4/devinet.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 10 deletions(-)net/ipv4/devinet.c +26
-6@@ -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 )
net/ipv4/devinet.c static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) · line 1716
Quoted 10 lines
net/ipv4/devinet.c +5
-4+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.
net/ipv4/devinet.c static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) · line 1722
Quoted 6 lines
net/ipv4/devinet.c +6
-0+ 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. :)
Quoted 12 lines
net/ipv4/devinet.c +6
-0+return PTR_ERR (tgt_net); + } + }else { +NL_SET_ERR_MSG (extack,"Unsupported attribute in dump request" ); +return -EINVAL ; + } } }Signature · 1 line
2.11.0
Re: [PATCH net-next 03/20] net: Add extack to nlmsg_parse
In reply to Christian BraunerQuoted 11 lines · Christian Brauner
Quoted 7 lines · David Ahern
From: David Ahern <[email protected]>
Make sure extack is passed to
nlmsg_parsewhere easy to do so. Most of these are dump handlers and leveraging the extack in thenetlink_callback.Signed-off-by David Ahern <[email protected]>Yeah, having extack in dump requests sounds really useful to me!
it's been invaluable updating iproute2.
Re: [PATCH net-next 05/20] netlink: Add new socket option to enable strict checking on dumps
In reply to Christian BraunerQuoted 5 lines · Christian Brauner
Quoted 5 lines
include/linux/netlink.h +0
-0@@ -179,6 +179,8 @@ struct netlink_callback {
Quoted 9 lines
include/linux/netlink.h struct netlink_callback { · line 183
Quoted 5 lines
include/linux/netlink.h +2
-0struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc; + unsigned int strict_check:1, + unused:31;I like this idea a lot. :) but I'm not a fan of bitfields if not necessary. Is that really necessary here?
no strong opinions on a bitfield vs a bool.
Re: [PATCH net-next 05/20] netlink: Add new socket option to enable strict checking on dumps
Quoted 16 lines · David Ahern
Quoted 5 lines · Christian Brauner
Quoted 5 lines
include/linux/netlink.h +0
-0@@ -179,6 +179,8 @@ struct netlink_callback {Quoted 9 lines
include/linux/netlink.h struct netlink_callback { · line 183
Quoted 5 lines
include/linux/netlink.h +2
-0struct netlink_ext_ack *extack;u16 family;u16 min_dump_alloc; + unsigned int strict_check:1, + unused:31;I like this idea a lot. :) but I'm not a fan of bitfields if not necessary. Is that really necessary here?
Quoted 1 line
no strong opinions on a bitfield vs a bool.
Just feels like this is something that is
rarely used. Having a bool or traditional
flag might be more readable and easier to
maintain. :)
Re: [PATCH net-next 06/20] net/ipv4: Update inet_dump_ifaddr for strict data checking
In reply to Christian BraunerQuoted 18 lines · Christian Brauner
Quoted 4 lines
net/ipv4/devinet.c +4
-0+ + err =nlmsg_parse (nlh,sizeof (struct ifaddrmsg ), tb,IFA_MAX , + ifa_ipv4_policy, extack); +if (err <0 )net/ipv4/devinet.c static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) · line 1716
Quoted 10 lines
net/ipv4/devinet.c +5
-4+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.
Quoted 10 lines
net/ipv4/devinet.c static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb) · line 1722
Quoted 6 lines
net/ipv4/devinet.c +6
-0+ 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.
Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to Christian BraunerQuoted 31 lines · Christian Brauner
Quoted 31 lines
net/core/rtnetlink.c +28
-2+ 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 :
Quoted 8 lines
net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1968
Quoted 5 lines
net/core/rtnetlink.c +5
-0+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.
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to Christian BraunerQuoted 8 lines · Christian Brauner
Quoted 7 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_ifinfofor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.
Quoted 6 lines
Quoted 1 line
Signed-off-by David Ahern <[email protected]>This is on top of current net-next? Are your patches ensuring that ipv6 addr requests don't generate log messages anymore when a wrong header is passed but the strict socket option is not passed? The context here doesn't seem to indicate that. :)
this is an AF_INET6 GETLINK handler. Why? no idea, but I think you are confusing this patch with the GETADDR patch which generated the "netlink: 16 bytes leftover after parsing attributes in process `ip'." message before this set.
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to Christian BraunerQuoted 5 lines · Christian Brauner
Quoted 5 lines
net/ipv6/addrconf.c +5
-0+static int inet6_valid_dump_ifinfo (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; +
Quoted 8 lines
net/ipv6/addrconf.c static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev, · line 5641
Quoted 6 lines
net/ipv6/addrconf.c +6
-0+if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) {Shouldn't ipv6 specific dump requests at least support
IFA_TARGET_NETNSID?
It does not today. The AF_UNSPEC GETLINK dumps it but the AF_INET6 does not.
Some one can add it later if desired.
Quoted 1 line · David Ahern
From: David Ahern <[email protected]>
...
Quoted 4 lines
This patch set addresses the problem by adding a new socket flag,
NETLINK_DUMP_STRICT_CHK, that userspace can use with setsockopt to request strict checking of headers and attributes on dump requests and hence unlock the ability to use kernel side filters as they are added.
...
Quoted 14 lines
David Ahern (20):
netlink: Pass extack to dump handlers
netlink: Add extack message tonlmsg_parsefor invalid header length
net: Add extack tonlmsg_parse
net/ipv6: Refactor address dump to pushinet6_fill_argsto
in6_dump_addrs
netlink: Add new socket option to enable strict checking on dumps net/ipv4: Updateinet_dump_ifaddrfor strict data checking net/ipv6: Updateinet6_dump_addrfor strict data checking
rtnetlink: Updatertnl_dump_ifinfofor strict data checking
rtnetlink: Updatertnl_bridge_getlinkfor strict data checking
rtnetlink: Updatertnl_stats_dumpfor strict data checking
rtnetlink: Updateinet6_dump_ifinfofor strict data checking
rtnetlink: Updateipmr_rtm_dumplinkfor strict data checking
Quoted 9 lines
rtnetlink: Update fib dumps for strict data checking net/neighbor: Update
neigh_dump_infofor strict data checking net/neighbor: Updateneightbl_dump_infofor strict data checking net/namespace: Updatertnl_net_dumpidfor strict data checkingnet/fib_rules: Updatefib_nl_dumprulefor strict data checking net/ipv6: Updateip6addrlbl_dumpfor strict data checking
net: Update netconf dump handlers for strict data checking net/bridge: Updatebr_mdb_dumpfor strict data checking
One thing I missed in the rfc and v1 versions is strict attribute parsing -- ie., there should be nothing remaining after nla_parse is done. I have a new patch that adds an nlmsg_parse_strict and nla_parse_strict that returns -EINVAL (with extack filled in) if that happens. The new patch pushes the set over 20.
I can peel off the first 3 patches from this set which add extack to the dumps and down to nlmsg_parse and send those separately if preferred.
David Miller
maintainerFrom: David Ahern <[email protected]>
Date: Fri, 5 Oct 2018 15:18:11 -0600
Quoted 8 lines
One thing I missed in the rfc and v1 versions is strict attribute parsing -- ie., there should be nothing remaining after
nla_parseis done. I have a new patch that adds annlmsg_parse_strictandnla_parse_strictthat returns-EINVAL(with extack filled in) if that happens. The new patch pushes the set over 20.I can peel off the first 3 patches from this set which add extack to the dumps and down to
nlmsg_parseand send those separately if preferred.
Don't worry about it, just send the whole thing.
My rule with patch series sizes is "be reasonable", rather than a strict number requirement.
Thanks.
Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to David AhernQuoted 15 lines · David Ahern
Quoted 5 lines · Christian Brauner
Quoted 5 lines
net/ipv6/addrconf.c +5
-0+static int inet6_valid_dump_ifinfo (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; +Quoted 8 lines
net/ipv6/addrconf.c static int inet6_fill_ifinfo(struct sk_buff *skb, struct inet6_dev *idev, · line 5641
Quoted 6 lines
net/ipv6/addrconf.c +6
-0+if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) {Shouldn't ipv6 specific dump requests at least support
IFA_TARGET_NETNSID?
Quoted 4 lines
It does not today. The
AF_UNSPECGETLINK dumps it but theAF_INET6does not.Some one can add it later if desired.
Weird, I thought I had sent a patch for that as well. Doesn't matter now I'll just send one once your branch lands. :) Thanks!
Christian Brauner
Acked-byRe: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
In reply to David AhernQuoted 10 lines · David Ahern
Quoted 9 lines · Christian Brauner
Quoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet6_dump_ifinfofor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
Quoted 10 lines
Quoted 5 lines
This is on top of current net-next? Are your patches ensuring that ipv6 addr requests don't generate log messages anymore when a wrong header is passed but the strict socket option is not passed? The context here doesn't seem to indicate that. :)
this is an
AF_INET6GETLINK handler. Why? no idea, but I think you are confusing this patch with the GETADDR patch which generated the "netlink: 16 bytes leftover after parsing attributes in process `ip'." message before this set.
Yes, I realized this immediately afterwards.
Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to David AhernQuoted 41 lines · David Ahern
Quoted 31 lines · Christian Brauner
Quoted 31 lines
net/core/rtnetlink.c +28
-2+ 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 :Quoted 8 lines
net/core/rtnetlink.c static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb) · line 1968
Quoted 5 lines
net/core/rtnetlink.c +5
-0+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(). :)
Quoted 5 lines
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.
Re: [PATCH net-next 09/20] rtnetlink: Update rtnl_bridge_getlink for strict data checking
In reply to David AhernQuoted 57 lines · David Ahern
From: David Ahern <[email protected]>
Update
rtnl_bridge_getlinkfor 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 theIFLA_EXT_MASKattribute is supported.Signed-off-by David Ahern <[email protected]>--- net/core/rtnetlink.c | 50 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-)net/core/rtnetlink.c +18
-9@@ -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 ; + }
net/core/rtnetlink.c EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink); · line 4036
Quoted 10 lines
net/core/rtnetlink.c +8
-1+ } - 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.
net/core/rtnetlink.c EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink); · line 4041
Quoted 5 lines
net/core/rtnetlink.c +5
-0+ +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. :)
Quoted 18 lines
net/core/rtnetlink.c +9
-0+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;Signature · 1 line
2.11.0
Re: [PATCH net-next 10/20] rtnetlink: Update rtnl_stats_dump for strict data checking
In reply to David AhernQuoted 35 lines · David Ahern
From: David Ahern <[email protected]>
Update
rtnl_stats_dumpfor strict data checking. If the flag is set, the dump request is expected to have anif_stats_msgstruct as the header. All elements of the struct are expected to be 0 exceptfilter_maskwhich must be non-0 (legacy behavior). No attributes are supported.Signed-off-by David Ahern <[email protected]>--- net/core/rtnetlink.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)net/core/rtnetlink.c +4
-1@@ -4643,6 +4643,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh,static int rtnl_stats_dump (struct sk_buff *skb,struct netlink_callback *cb) { +struct netlink_ext_ack *extack = cb->extack;int h, s_h, err, s_idx, s_idxattr, s_prividx;struct net *net =sock_net (skb->sk);unsigned int flags =NLM_F_MULTI ;@@ -4659,13 +4660,32 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb) cb->seq = net->dev_base_seq; -if (nlmsg_len (cb->nlh) <sizeof (*ifsm)) +if (nlmsg_len (cb->nlh) <sizeof (*ifsm)) { +NL_SET_ERR_MSG (extack,"Invalid header" );return -EINVAL ; + } ifsm =nlmsg_data (cb->nlh);
net/core/rtnetlink.c static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb) · line 4678
Quoted 10 lines
net/core/rtnetlink.c +10
-0+ +/* only requests using NLM_F_DUMP_PROPER_HDR can pass data to +* influence the dump. The legacy exception is filter_mask. +*/ +if (cb->strict_check) { +if (ifsm->pad1 || ifsm->pad2 || ifsm->ifindex) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; + } +if (cb->nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifsm))) {
Nit: \n appreciated :)
net/core/rtnetlink.c static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb) · line 4686
Quoted 9 lines
net/core/rtnetlink.c +7
-1+NL_SET_ERR_MSG (extack,"Invalid attributes after header" ); +return -EINVAL ; + } + } + filter_mask = ifsm->filter_mask; -if (!filter_mask) +if (!filter_mask) { +NL_SET_ERR_MSG (extack,"Invalid filter_mask" );
Nit: probably better to have this read "Invalid filter mask".
Quoted 8 lines
net/core/rtnetlink.c +1
-0return -EINVAL ; + }for (h = s_h; h <NETDEV_HASHENTRIES ; h++, s_idx =0 ) { idx =0 ;Signature · 1 line
2.11.0
Re: [PATCH net-next 12/20] rtnetlink: Update ipmr_rtm_dumplink for strict data checking
In reply to David AhernQuoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
ipmr_rtm_dumplinkfor strict data checking. If the flag is set, the dump request is expected to have an ifinfomsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
Just one really tiny nit below. :)
Quoted 41 lines
--- net/ipv4/ipmr.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)net/ipv4/ipmr.c +25
-0@@ -2710,6 +2710,31 @@ static bool ipmr_fill_vif(struct mr_table *mrt, u32 vifid, struct sk_buff *skb)return true; } +static int ipmr_valid_dumplink (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifinfomsg *ifm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len >nlmsg_msg_size (sizeof (*ifm))) { +NL_SET_ERR_MSG (extack,"Invalid data after 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 ; + } + +return 0 ; +} +static int ipmr_rtm_dumplink (struct sk_buff *skb,struct netlink_callback *cb) {struct net *net =sock_net (skb->sk);
net/ipv4/ipmr.c static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb) · line 2750
Quoted 8 lines
net/ipv4/ipmr.c +5
-0@@ -2718,6 +2743,13 @@ static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb)unsigned int e =0 , s_e;struct mr_table *mrt; +if (cb->strict_check) { +int err =ipmr_valid_dumplink (cb->nlh, cb->extack); + +if (err) +return err;
Nit: can we remove the unnecessary \n, please.
Quoted 8 lines
net/ipv4/ipmr.c +2
-0+ } + s_t = cb->args[0 ]; s_e = cb->args[1 ];Signature · 1 line
2.11.0
Re: [PATCH net-next 13/20] rtnetlink: Update fib dumps for strict data checking
In reply to David AhernQuoted 9 lines · David Ahern
From: David Ahern <[email protected]>
Add helper to check netlink message for route dumps. If the strict flag is set the dump request is expected to have an rtmsg struct as the header. All elements of the struct are expected to be 0 with the exception of
rtm_flags(which is used by both ipv4 and ipv6 dumps) and no attributes can be appended.rtm_flagscan only haveRTM_F_CLONEDandRTM_F_PREFIXset.
Quoted 3 lines
Update
inet_dump_fib,inet6_dump_fib,mpls_dump_routes,ipmr_rtm_dumproute, andip6mr_rtm_dumprouteto call this helper if strict data checking is enabled.
This also looks good to me apart from the \n after every:
+ type <err> = foo();
+
+ if (<err> < 0)
/* handle error */
I haven't seen this style in the code a lot or I haven't looked close enough. It just seems visually cleaner to have this close together without a newline:
+ type <err> = foo();
+ if (<err> < 0)
/* handle error */
Quoted 180 lines
Signed-off-by David Ahern <[email protected]>--- include/net/ip_fib.h | 2 ++ net/ipv4/fib_frontend.c | 43 +++++++++++++++++++++++++++++++++++++++++-- net/ipv4/ipmr.c | 9 +++++++++ net/ipv6/ip6_fib.c | 8 ++++++++ net/ipv6/ip6mr.c | 9 +++++++++ net/mpls/af_mpls.c | 8 ++++++++ 6 files changed, 77 insertions(+), 2 deletions(-)include/net/ip_fib.h +2
-0@@ -452,4 +452,6 @@ static inline void fib_proc_exit(struct net *net)u32 ip_mtu_from_fib_result (struct fib_result *res,__be32 daddr); +int ip_valid_fib_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack);#endif /* _NET_FIB_H */ net/ipv4/fib_frontend.c +41
-2@@ -802,8 +802,41 @@ static int inet_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh,return err; } +int ip_valid_fib_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct rtmsg *rtm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*rtm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + rtm =nlmsg_data (nlh); +if (rtm->rtm_dst_len || rtm->rtm_src_len || rtm->rtm_tos || + rtm->rtm_table || rtm->rtm_protocol || rtm->rtm_scope || + rtm->rtm_type) { +NL_SET_ERR_MSG (extack, +"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (rtm->rtm_flags & ~(RTM_F_CLONED |RTM_F_PREFIX )) { +NL_SET_ERR_MSG (extack,"Invalid flags for dump request" ); +return -EINVAL ; + } +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*rtm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +EXPORT_SYMBOL_GPL (ip_valid_fib_dump_req); +static int inet_dump_fib (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);unsigned int h, s_h;unsigned int e =0 , s_e;@@ -811,8 +844,14 @@ static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)struct hlist_head *head;int dumped =0 , err; -if (nlmsg_len (cb->nlh) >=sizeof (struct rtmsg ) && - ((struct rtmsg *)nlmsg_data (cb->nlh))->rtm_flags &RTM_F_CLONED ) +if (cb->strict_check) { + err =ip_valid_fib_dump_req (nlh, cb->extack); +if (err) +return err; + } + +if (nlmsg_len (nlh) >=sizeof (struct rtmsg ) && + ((struct rtmsg *)nlmsg_data (nlh))->rtm_flags &RTM_F_CLONED )return skb->len; s_h = cb->args[0 ];net/ipv4/ipmr.c +9
-0@@ -2527,6 +2527,15 @@ static int ipmr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,static int ipmr_rtm_dumproute (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh; + +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } +return mr_rtm_dumproute (skb, cb, ipmr_mr_table_iter, _ipmr_fill_mroute, &mfc_unres_lock); }net/ipv6/ip6_fib.c +8
-0@@ -568,6 +568,7 @@ static int fib6_dump_table(struct fib6_table *table, struct sk_buff *skb,static int inet6_dump_fib (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);unsigned int h, s_h;unsigned int e =0 , s_e;@@ -577,6 +578,13 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)struct hlist_head *head;int res =0 ; +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } + s_h = cb->args[0 ]; s_e = cb->args[1 ];net/ipv6/ip6mr.c +9
-0@@ -2457,6 +2457,15 @@ static void mrt6msg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt)static int ip6mr_rtm_dumproute (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh; + +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } +return mr_rtm_dumproute (skb, cb, ip6mr_mr_table_iter, _ip6mr_fill_mroute, &mfc_unres_lock); }net/mpls/af_mpls.c +8
-0@@ -2017,6 +2017,7 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event,static int mpls_dump_routes (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct mpls_route __rcu **platform_label;size_t platform_labels;@@ -2024,6 +2025,13 @@ static int mpls_dump_routes(struct sk_buff *skb, struct netlink_callback *cb)ASSERT_RTNL (); +if (cb->strict_check) { +int err =ip_valid_fib_dump_req (nlh, cb->extack); + +if (err) +return err; + } + index = cb->args[0 ];if (index <MPLS_LABEL_FIRST_UNRESERVED ) index =MPLS_LABEL_FIRST_UNRESERVED ;Signature · 1 line
2.11.0
Re: [PATCH net-next 14/20] net/neighbor: Update neigh_dump_info for strict data checking
In reply to David AhernQuoted 39 lines · David Ahern
From: David Ahern <[email protected]>
Update
neigh_dump_infofor strict data checking. If the flag is set, the dump request is expected to have an ndmsg 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 theNDA_IFINDEXandNDA_MASTERattributes are supported.Existing code does not fail the dump if
nlmsg_parsefails. That behavior is kept for non-strict checking.Signed-off-by David Ahern <[email protected]>--- net/core/neighbour.c | 59 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 11 deletions(-)net/core/neighbour.c +2
-1@@ -2428,13 +2428,14 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,static int neigh_dump_info (struct sk_buff *skb,struct netlink_callback *cb) { +struct netlink_ext_ack *extack = cb->extack;const struct nlmsghdr *nlh = cb->nlh;struct neigh_dump_filter filter = {};struct nlattr *tb[NDA_MAX +1 ];struct neigh_table *tbl;int t, family, s_t;int proxy =0 ; -int err; +int err, i; family = ((struct rtgenmsg *)nlmsg_data (nlh))->rtgen_family;
net/core/neighbour.c static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) · line 2449
Quoted 5 lines
net/core/neighbour.c +0
-2@@ -2445,20 +2446,56 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb) ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY) proxy =1 ; - err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , - cb->extack);
So right, this seems like there was precedent in the kernel where an nlmsg_parse() would fail and a warning as generated in the logs. Anyway, as we have decided to not cause such warnings to be printed it would make sense to make the whole nlmsg_parse() conditional and move it under the if (strict) branch.
Quoted 64 lines
net/core/neighbour.c +46
-8-if (!err) { -if (tb[NDA_IFINDEX ]) { -if (nla_len (tb[NDA_IFINDEX ]) !=sizeof (u32)) -return -EINVAL ; - filter.dev_idx =nla_get_u32 (tb[NDA_IFINDEX ]); +if (cb->strict_check) { +struct ndmsg *ndm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ndm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ndm =nlmsg_data (nlh); +if (ndm->ndm_pad1 || ndm->ndm_pad2 || ndm->ndm_ifindex || + ndm->ndm_state || ndm->ndm_flags || ndm->ndm_type) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; } -if (tb[NDA_MASTER ]) { -if (nla_len (tb[NDA_MASTER ]) !=sizeof (u32)) + } + + err =nlmsg_parse (nlh,sizeof (struct ndmsg ), tb,NDA_MAX ,NULL , extack); +if (err <0 ) { +if (cb->strict_check) +return -EINVAL ; + goto walk_entries; + } + +for (i =0 ; i <=NDA_MAX ; ++i) { +if (!tb[i]) +continue ; +switch (i) { +case NDA_IFINDEX : +if (nla_len (tb[i]) !=sizeof (u32)) { +NL_SET_ERR_MSG (extack,"Invalid IFINDEX attribute" );return -EINVAL ; - filter.master_idx =nla_get_u32 (tb[NDA_MASTER ]); + } + filter.dev_idx =nla_get_u32 (tb[i]); +break ; +case NDA_MASTER : +if (nla_len (tb[i]) !=sizeof (u32)) { +NL_SET_ERR_MSG (extack,"Invalid MASTER attribute" ); +return -EINVAL ; + } + filter.master_idx =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: s_t = cb->args[0 ];for (t =0 ; t <NEIGH_NR_TABLES ; t++) {Signature · 1 line
2.11.0
Re: [PATCH net-next 15/20] net/neighbor: Update neightbl_dump_info for strict data checking
In reply to David AhernQuoted 52 lines · David Ahern
From: David Ahern <[email protected]>
Update
neightbl_dump_infofor strict data checking. If the flag is set, the dump request is expected to have an ndtmsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>--- net/core/neighbour.c | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-)net/core/neighbour.c +25
-0@@ -2164,15 +2164,47 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,return err; } +static int neightbl_valid_dump_info (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ndtmsg *ndtm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ndtm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ndtm =nlmsg_data (nlh); +if (ndtm->ndtm_pad1 || ndtm->ndtm_pad2) { +NL_SET_ERR_MSG (extack,"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ndtm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int neightbl_dump_info (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int family, tidx, nidx =0 ;int tbl_skip = cb->args[0 ];int neigh_skip = cb->args[1 ];struct neigh_table *tbl;
net/core/neighbour.c static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh, · line 2207
Quoted 10 lines
net/core/neighbour.c +8
-1- family = ((struct rtgenmsg *)nlmsg_data (cb->nlh))->rtgen_family; +if (cb->strict_check) { +int err =neightbl_valid_dump_info (nlh, cb->extack); + +if (err) +return err; + } + + family = ((struct rtgenmsg *)nlmsg_data (nlh))->rtgen_family;
So this already was a problem prior to your patch: what happens when you pass in the wrong struct? Then this case is not safe to do and might contain all kinds of crap.
Quoted 24 lines
net/core/neighbour.c +2
-2for (tidx =0 ; tidx <NEIGH_NR_TABLES ; tidx++) {struct neigh_parms *p;@@ -2185,7 +2217,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)continue ;if (neightbl_fill_info (skb, tbl,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq,RTM_NEWNEIGHTBL , + nlh->nlmsg_seq,RTM_NEWNEIGHTBL ,NLM_F_MULTI ) <0 )break ;@@ -2200,7 +2232,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)if (neightbl_fill_param_info (skb, tbl, p,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNEIGHTBL ,NLM_F_MULTI ) <0 ) goto out;Signature · 1 line
2.11.0
Re: [PATCH net-next 19/20] net: Update netconf dump handlers for strict data checking
In reply to David AhernQuoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
inet_netconf_dump_devconf,inet6_netconf_dump_devconf, andmpls_netconf_dump_devconffor strict data checking. If the flag is set, the dump request is expected to have an netconfmsg struct as the header. The struct only has the family member and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
Quoted 25 lines
--- net/ipv4/devinet.c | 22 +++++++++++++++++++--- net/ipv6/addrconf.c | 22 +++++++++++++++++++--- net/mpls/af_mpls.c | 18 +++++++++++++++++- 3 files changed, 55 insertions(+), 7 deletions(-)net/ipv4/devinet.c +4
-0@@ -2069,6 +2069,7 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,static int inet_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int h, s_h;int idx, s_idx;@@ -2076,6 +2077,21 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,struct in_device *in_dev;struct hlist_head *head; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm;
net/ipv4/devinet.c static int inet_netconf_dump_devconf(struct sk_buff *skb, · line 2092
Quoted 10 lines
net/ipv4/devinet.c +10
-0+ +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + }
Hm, I think this could just be one branch with !=
But if you've done this to report back a more meaningful error message to userspace, fine. :)
Quoted 139 lines
net/ipv4/devinet.c +5
-3+ } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];@@ -2095,7 +2111,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb, dev->ifindex, &in_dev->cnf,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {@@ -2112,7 +2128,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb,NETCONFA_IFINDEX_ALL , net->ipv4.devconf_all,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;@@ -2123,7 +2139,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,if (inet_netconf_fill_devconf (skb,NETCONFA_IFINDEX_DEFAULT , net->ipv4.devconf_dflt,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;net/ipv6/addrconf.c +19
-3@@ -666,6 +666,7 @@ static int inet6_netconf_get_devconf(struct sk_buff *in_skb,static int inet6_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int h, s_h;int idx, s_idx;@@ -673,6 +674,21 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,struct inet6_dev *idev;struct hlist_head *head; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];@@ -692,7 +708,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb, dev->ifindex, &idev->cnf,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {@@ -709,7 +725,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb,NETCONFA_IFINDEX_ALL , net->ipv6.devconf_all,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;@@ -720,7 +736,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,if (inet6_netconf_fill_devconf (skb,NETCONFA_IFINDEX_DEFAULT , net->ipv6.devconf_dflt,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) goto done;net/mpls/af_mpls.c +17
-1@@ -1263,6 +1263,7 @@ static int mpls_netconf_get_devconf(struct sk_buff *in_skb,static int mpls_netconf_dump_devconf (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct hlist_head *head;struct net_device *dev;@@ -1270,6 +1271,21 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb,int idx, s_idx;int h, s_h; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + } + s_h = cb->args[0 ]; s_idx = idx = cb->args[1 ];@@ -1286,7 +1302,7 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb, goto cont;if (mpls_netconf_fill_devconf (skb, mdev,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWNETCONF ,NLM_F_MULTI ,NETCONFA_ALL ) <0 ) {Signature · 1 line
2.11.0
Re: [PATCH net-next 18/20] net/ipv6: Update ip6addrlbl_dump for strict data checking
In reply to David AhernQuoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
ip6addrlbl_dumpfor strict data checking. If the flag is set, the dump request is expected to have an ifaddrlblmsg struct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
Quoted 66 lines
--- net/ipv6/addrlabel.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)net/ipv6/addrlabel.c +34
-1@@ -458,20 +458,53 @@ static int ip6addrlbl_fill(struct sk_buff *skb,return 0 ; } +static int ip6addrlbl_valid_dump_req (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct ifaddrlblmsg *ifal; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ifal))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + ifal =nlmsg_data (nlh); +if (ifal->__ifal_reserved || ifal->ifal_prefixlen || + ifal->ifal_flags || ifal->ifal_index || ifal->ifal_seq) { +NL_SET_ERR_MSG (extack, +"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ifal))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int ip6addrlbl_dump (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct ip6addrlbl_entry *p;int idx =0 , s_idx = cb->args[0 ];int err; +if (cb->strict_check) { + err =ip6addrlbl_valid_dump_req (nlh, cb->extack); +if (err) +return err; + } +rcu_read_lock ();hlist_for_each_entry_rcu (p, &net->ipv6.ip6addrlbl_table.head, list) {if (idx >= s_idx) { err =ip6addrlbl_fill (skb, p, net->ipv6.ip6addrlbl_table.seq,NETLINK_CB (cb->skb).portid, - cb->nlh->nlmsg_seq, + nlh->nlmsg_seq,RTM_NEWADDRLABEL ,NLM_F_MULTI );if (err <0 )Signature · 1 line
2.11.0
Re: [PATCH net-next 17/20] net/fib_rules: Update fib_nl_dumprule for strict data checking
In reply to David AhernQuoted 8 lines · David Ahern
From: David Ahern <[email protected]>
Update
fib_nl_dumprulefor strict data checking. If the flag is set, the dump request is expected to havefib_rule_hdrstruct as the header. All elements of the struct are expected to be 0 and no attributes can be appended.Signed-off-by David Ahern <[email protected]>
Quoted 60 lines
--- net/core/fib_rules.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)net/core/fib_rules.c +35
-1@@ -1063,13 +1063,47 @@ static int dump_rules(struct sk_buff *skb, struct netlink_callback *cb,return err; } +static int fib_valid_dumprule (const struct nlmsghdr *nlh, +struct netlink_ext_ack *extack) +{ +struct fib_rule_hdr *frh; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*frh))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } + + frh =nlmsg_data (nlh); +if (frh->dst_len || frh->src_len || frh->tos || frh->table || + frh->res1 || frh->res2 || frh->action || frh->flags) { +NL_SET_ERR_MSG (extack, +"Invalid values in header for dump request" ); +return -EINVAL ; + } + +if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*frh))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + } + +return 0 ; +} +static int fib_nl_dumprule (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);struct fib_rules_ops *ops;int idx =0 , family; - family =rtnl_msg_family (cb->nlh); +if (cb->strict_check) { +int err =fib_valid_dumprule (nlh, cb->extack); + +if (err) +return err; + } + + family =rtnl_msg_family (nlh);if (family !=AF_UNSPEC ) {/* Protocol specific dump request */ ops =lookup_rules_ops (net, family);Signature · 1 line
2.11.0
Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to Christian BraunerQuoted 11 lines · Christian Brauner
Quoted 5 lines
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.
Re: [PATCH net-next 09/20] rtnetlink: Update rtnl_bridge_getlink for strict data checking
In reply to Christian BraunerQuoted 21 lines · Christian Brauner
Quoted 21 lines
net/core/rtnetlink.c +14
-5+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 ; + } + }
Quoted 13 lines
net/core/rtnetlink.c EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink); · line 4036
Quoted 8 lines
net/core/rtnetlink.c +7
-1- 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 ...
Quoted 2 lines
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.
Re: [PATCH net-next 12/20] rtnetlink: Update ipmr_rtm_dumplink for strict data checking
In reply to Christian BraunerQuoted 11 lines · Christian Brauner
net/ipv4/ipmr.c static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb) · line 2750
Quoted 9 lines
net/ipv4/ipmr.c +5
-0@@ -2718,6 +2743,13 @@ static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb)unsigned int e =0 , s_e;struct mr_table *mrt; +if (cb->strict_check) { +int err =ipmr_valid_dumplink (cb->nlh, cb->extack); + +if (err) +return err;Nit: can we remove the unnecessary \n, please.
Coding standards dictate a newline between declarations and code. And that is my preference too.
Re: [PATCH net-next 15/20] net/neighbor: Update neightbl_dump_info for strict data checking
In reply to Christian BraunerQuoted 10 lines · Christian Brauner
Quoted 10 lines
net/core/neighbour.c +2
-0+static int neightbl_dump_info (struct sk_buff *skb,struct netlink_callback *cb) { +const struct nlmsghdr *nlh = cb->nlh;struct net *net =sock_net (skb->sk);int family, tidx, nidx =0 ;int tbl_skip = cb->args[0 ];int neigh_skip = cb->args[1 ];struct neigh_table *tbl;
Quoted 13 lines
net/core/neighbour.c static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh, · line 2207
Quoted 9 lines
net/core/neighbour.c +8
-1- family = ((struct rtgenmsg *)nlmsg_data (cb->nlh))->rtgen_family; +if (cb->strict_check) { +int err =neightbl_valid_dump_info (nlh, cb->extack); + +if (err) +return err; + } + + family = ((struct rtgenmsg *)nlmsg_data (nlh))->rtgen_family;So this already was a problem prior to your patch: what happens when you pass in the wrong struct? Then this case is not safe to do and might contain all kinds of crap.
'This case' meaning the above dereference? family is *always* the first element in all of the header structs. It is core to the rtnetlink processing.
Re: [PATCH net-next 19/20] net: Update netconf dump handlers for strict data checking
In reply to Christian BraunerQuoted 13 lines · Christian Brauner
Quoted 13 lines
net/ipv4/devinet.c +9
-0@@ -2076,6 +2077,21 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,struct in_device *in_dev;struct hlist_head *head; +if (cb->strict_check) { +struct netlink_ext_ack *extack = cb->extack; +struct netconfmsg *ncm; + +if (nlh->nlmsg_len <nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid header" ); +return -EINVAL ; + } +
Quoted 8 lines
net/ipv4/devinet.c static int inet_netconf_dump_devconf(struct sk_buff *skb, · line 2092
Quoted 4 lines
net/ipv4/devinet.c +4
-0+if (nlh->nlmsg_len !=nlmsg_msg_size (sizeof (*ncm))) { +NL_SET_ERR_MSG (extack,"Invalid data after header" ); +return -EINVAL ; + }Hm, I think this could just be one branch with !=
But if you've done this to report back a more meaningful error message to userspace, fine. :)
Consistency with other dump handlers and better userspace error messages. If netconf ever gets a filter the length check is removed in favor of nlmsg_parse_strict
Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
In reply to David AhernQuoted 7 lines · David Ahern
Quoted 6 lines · Christian Brauner
Quoted 5 lines
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.
Quoted 8 lines
Quoted 5 lines
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!