diff mbox series

[net-next,19/20] net: Update netconf dump handlers for strict data checking

Message ID 20181004213355.14899-20-dsahern@kernel.org
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series rtnetlink: Add support for rigid checking of data in dump request | expand

Commit Message

David Ahern Oct. 4, 2018, 9:33 p.m. UTC 
From: David Ahern <dsahern@gmail.com>

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.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv4/devinet.c  | 22 +++++++++++++++++++---
 net/ipv6/addrconf.c | 22 +++++++++++++++++++---
 net/mpls/af_mpls.c  | 18 +++++++++++++++++-
 3 files changed, 55 insertions(+), 7 deletions(-)

Comments

Christian Brauner Oct. 7, 2018, 10:53 a.m. UTC | #1 
On Thu, Oct 04, 2018 at 02:33:54PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> 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.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>

Acked-by: Christian Brauner <christian@brauner.io>

> ---
>  net/ipv4/devinet.c  | 22 +++++++++++++++++++---
>  net/ipv6/addrconf.c | 22 +++++++++++++++++++---
>  net/mpls/af_mpls.c  | 18 +++++++++++++++++-
>  3 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index af968d4fe4fc..595706d6b672 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -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;
> +		}

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. :)

> +	}
> +
>  	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;
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 693199a29426..9dfe6c2106c1 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -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;
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 3e33934751b4..b80b00b55bdf 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -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) {
> -- 
> 2.11.0
>
David Ahern Oct. 8, 2018, 1:38 a.m. UTC | #2 
On 10/7/18 4:53 AM, Christian Brauner wrote:
>> @@ -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;
>> +		}
> 
> 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
diff mbox series

Patch

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index af968d4fe4fc..595706d6b672 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -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;
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 693199a29426..9dfe6c2106c1 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -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;
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 3e33934751b4..b80b00b55bdf 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -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) {