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

[David Ahern]

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

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

[Christian Brauner]

On Thu, Oct 04, 2018 at 02:33:54PM -0700, David Ahern wrote:
> From: David Ahern 
> 
> 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 

Acked-by: Christian Brauner 

> ---
>  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,
> _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 !=