netdev - Re: [PATCH v2 net-next 22/23] rtnetlink: Move input checking for rtnl_fdb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181008110119.4dqascblzuxbe3vc@brauner.io>
Date:   Mon, 8 Oct 2018 13:01:20 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Ahern <dsahern@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jbenc@...hat.com,
        stephen@...workplumber.org, David Ahern <dsahern@...il.com>
Subject: Re: [PATCH v2 net-next 22/23] rtnetlink: Move input checking for
 rtnl_fdb_dump to helper

On Sun, Oct 07, 2018 at 08:16:43PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@...il.com>
> 
> Move the existing input checking for rtnl_fdb_dump into a helper,
> valid_fdb_dump_legacy. This function will retain the current
> logic that works around the 2 headers that userspace has been
> allowed to send up to this point.
> 
> Signed-off-by: David Ahern <dsahern@...il.com>

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

> ---
>  net/core/rtnetlink.c | 53 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f6d2609cfa9f..c7509c789fb6 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3799,22 +3799,13 @@ int ndo_dflt_fdb_dump(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(ndo_dflt_fdb_dump);
>  
> -static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
> +static int valid_fdb_dump_legacy(const struct nlmsghdr *nlh,
> +				 int *br_idx, int *brport_idx,
> +				 struct netlink_ext_ack *extack)
>  {
> -	struct net_device *dev;
> +	struct ifinfomsg *ifm = nlmsg_data(nlh);

You could move this cast after the

if (nlmsg_len(nlh) != sizeof(struct ndmsg) &&
    (nlmsg_len(nlh) != sizeof(struct ndmsg) +

check. It doesn't matter that much but it minimizes the risk of someone
accidently accessing struct ifinfomsg *ifm when it's an invalid cast.


>  	struct nlattr *tb[IFLA_MAX+1];
> -	struct net_device *br_dev = NULL;
> -	const struct net_device_ops *ops = NULL;
> -	const struct net_device_ops *cops = NULL;
> -	struct ifinfomsg *ifm = nlmsg_data(cb->nlh);
> -	struct net *net = sock_net(skb->sk);
> -	struct hlist_head *head;
> -	int brport_idx = 0;
> -	int br_idx = 0;
> -	int h, s_h;
> -	int idx = 0, s_idx;
> -	int err = 0;
> -	int fidx = 0;
> +	int err;
>  
>  	/* A hack to preserve kernel<->userspace interface.
>  	 * Before Linux v4.12 this code accepted ndmsg since iproute2 v3.3.0.
> @@ -3823,20 +3814,42 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	 * Fortunately these sizes don't conflict with the size of ifinfomsg
>  	 * with an optional attribute.
>  	 */
> -	if (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) &&
> -	    (nlmsg_len(cb->nlh) != sizeof(struct ndmsg) +
> +	if (nlmsg_len(nlh) != sizeof(struct ndmsg) &&
> +	    (nlmsg_len(nlh) != sizeof(struct ndmsg) +
>  	     nla_attr_size(sizeof(u32)))) {
> -		err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> -				  IFLA_MAX, ifla_policy, cb->extack);
> +		err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
> +				  ifla_policy, extack);
>  		if (err < 0) {
>  			return -EINVAL;
>  		} else if (err == 0) {
>  			if (tb[IFLA_MASTER])
> -				br_idx = nla_get_u32(tb[IFLA_MASTER]);
> +				*br_idx = nla_get_u32(tb[IFLA_MASTER]);
>  		}
>  
> -		brport_idx = ifm->ifi_index;
> +		*brport_idx = ifm->ifi_index;
>  	}
> +	return 0;
> +}
> +
> +static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	struct net_device *dev;
> +	struct net_device *br_dev = NULL;
> +	const struct net_device_ops *ops = NULL;
> +	const struct net_device_ops *cops = NULL;
> +	struct net *net = sock_net(skb->sk);
> +	struct hlist_head *head;
> +	int brport_idx = 0;
> +	int br_idx = 0;
> +	int h, s_h;
> +	int idx = 0, s_idx;
> +	int err = 0;
> +	int fidx = 0;
> +
> +	err = valid_fdb_dump_legacy(cb->nlh, &br_idx, &brport_idx,
> +				    cb->extack);
> +	if (err < 0)
> +		return err;
>  
>  	if (br_idx) {
>  		br_dev = __dev_get_by_index(net, br_idx);
> -- 
> 2.11.0
>