netdev - Re: [PATCH net-next] rtnetlink: fix 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1cf63710-8ddb-2e21-4df4-f8687433f3fb@gmail.com>
Date:   Sun, 30 Sep 2018 19:06:16 -0600
From:   David Ahern <dsahern@...il.com>
To:     Mauricio Faria de Oliveira <mfo@...onical.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net
Subject: Re: [PATCH net-next] rtnetlink: fix rtnl_fdb_dump() for shorter
 family headers

On 9/28/18 1:35 PM, Mauricio Faria de Oliveira wrote:
> Currently, rtnl_fdb_dump() assumes the family header is 'struct ifinfomsg',
> which is not always true.  For example, 'struct ndmsg' is used by iproute2
> as well (in the 'ip neigh' command).
> 
> The problem is, the function bails out early if nlmsg_parse() fails, which
> does occur for iproute2 usage of 'struct ndmsg' because the payload length
> is shorter than the family header alone (as 'struct ifinfomsg' is assumed).
> 
> This breaks backward compatibility with userspace (different response) and
> is a regression due to commit 0ff50e83b512 ("net: rtnetlink: bail out from 
>  rtnl_fdb_dump() on parse error").
...

> 
> Fixes: 0ff50e83b512 ("net: rtnetlink: bail out from rtnl_fdb_dump() on parse error")
> Fixes: 5e6d24358799 ("bridge: netlink dump interface at par with brctl")
> Reported-by: Aidan Obley <aobley@...otal.io>
> Signed-off-by: Mauricio Faria de Oliveira <mfo@...onical.com>
> ---
> P.S.: this may be 'net', but labeling as 'net-next' for possible relation to recent thread
> [PATCH RFC net-next 0/5] rtnetlink: Add support for rigid checking of data in dump request
> 
>  net/core/rtnetlink.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 60c928894a78..9695a27cc9b9 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3744,16 +3744,17 @@ static int rtnl_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	int err = 0;
>  	int fidx = 0;
>  
> -	err = nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> -			  IFLA_MAX, ifla_policy, NULL);
> -	if (err < 0) {
> -		return -EINVAL;
> -	} else if (err == 0) {
> +	/* The family header may _not_ be struct ifinfomsg
> +	 * (e.g., struct ndmsg).  Usage of the ifm pointer
> +	 * must check payload length (e.g., nlmsg_parse()).
> +	 */
> +	if (nlmsg_parse(cb->nlh, sizeof(struct ifinfomsg), tb,
> +			IFLA_MAX, ifla_policy, NULL) == 0) {
>  		if (tb[IFLA_MASTER])
>  			br_idx = nla_get_u32(tb[IFLA_MASTER]);
> -	}
>  
> -	brport_idx = ifm->ifi_index;
> +		brport_idx = ifm->ifi_index;
> +	}
>  
>  	if (br_idx) {
>  		br_dev = __dev_get_by_index(net, br_idx);
> 

I suspect rtnl_fdb_dump is forever stuck with the ifinfomsg struct as
the header if any kernel side filtering is to be done. As for the change
above, I suggest something like this:

	/* if header struct is ndmsg, no attributes can be appended */
	if (nlmsg_len(nlh) != sizeof(struct ndmsg)) {
		current ifinfomsg based code
	}

We certainly do not want to ignore parse failures.