netdev - Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK

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: <20200727222158.bg52mg2mfsta2f37@lion.mk-sys.cz>
Date:   Tue, 28 Jul 2020 00:21:58 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Jamie Gloudon <jamie.gloudon@....fr>
Subject: Re: [ethtool] ethtool: fix netlink bitmasks when sent as NOMASK

On Mon, Jul 27, 2020 at 02:47:00PM -0700, Jacob Keller wrote:
> The ethtool netlink API can send bitsets without an associated bitmask.
> These do not get displayed properly, because the dump_link_modes, and
> bitset_get_bit to not check whether the provided bitset is a NOMASK
> bitset. This results in the inability to display peer advertised link
> modes.
> 
> The dump_link_modes and bitset_get_bit functions are designed so they
> can print either the values or the mask. For a nomask bitmap, this
> doesn't make sense. There is no mask.
> 
> Modify dump_link_modes to check ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always check and print the ETHTOOL_A_BITSET_VALUE bits,
> regardless of the request to display the mask or the value. For full
> size bitmaps, the set of provided bits indicates the valid values,
> without using ETHTOOL_A_BITSET_VALUE fields. Thus, do not skip printing
> bits without this attribute if nomask is set. This essentially means
> that dump_link_modes will treat a NOMASK bitset as having a mask
> equivalent to all of its set bits.
> 
> For bitset_get_bit, also check for ETHTOOL_A_BITSET_NOMASK. For compact
> bitmaps, always use ETHTOOL_A_BITSET_BIT_VALUE as in dump_link_modes.
> For full bitmaps, if nomask is set, then always return true of the bit
> is in the set, rather than only if it provides an
> ETHTOOL_A_BITSET_BIT_VALUE. This will then correctly report the set
> bits.
> 
> This fixes display of link partner advertised fields when using the
> netlink API.
> 
> Reported-by: Jamie Gloudon <jamie.gloudon@....fr>
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
> ---
>  netlink/bitset.c   | 9 ++++++---
>  netlink/settings.c | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/netlink/bitset.c b/netlink/bitset.c
> index 130bcdb5b52c..ba5d3ea77ff7 100644
> --- a/netlink/bitset.c
> +++ b/netlink/bitset.c
> @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>  	DECLARE_ATTR_TB_INFO(bitset_tb);
>  	const struct nlattr *bits;
>  	const struct nlattr *bit;
> +	bool nomask;
>  	int ret;
>  
>  	*retptr = 0;
> @@ -57,8 +58,10 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>  	if (ret < 0)
>  		goto err;
>  
> -	bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> -		      bitset_tb[ETHTOOL_A_BITSET_VALUE];
> +	nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
> +
> +	bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> +		                 bitset_tb[ETHTOOL_A_BITSET_VALUE];
>  	if (bits) {
>  		const uint32_t *bitmap =
>  			(const uint32_t *)mnl_attr_get_payload(bits);

I don't like this part: (mask && nomask) is a situation which should
never happen as it would mean we are trying to get mask value from
a bitmap which does not any. In other words, if we ever see such
combination, it is a result of a bug either on ethtool side or on kernel
side.

Rather than silently returning something else than asked, we should
IMHO report an error. Which is easy in dump_link_modes() but it would
require rewriting bitset_get_bit().

Michal

> @@ -87,7 +90,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx,
>  
>  		my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
>  		if (my_idx == idx)
> -			return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
> +			return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE];
>  	}
>  
>  	return false;
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5dd6d5..29557653336e 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -280,9 +280,11 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
>  	const struct nlattr *bit;
>  	bool first = true;
>  	int prev = -2;
> +	bool nomask;
>  	int ret;
>  
>  	ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info);
> +	nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK];
>  	bits = bitset_tb[ETHTOOL_A_BITSET_BITS];
>  	if (ret < 0)
>  		goto err_nonl;
> @@ -297,8 +299,8 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
>  			goto err_nonl;
>  		lm_strings = global_stringset(ETH_SS_LINK_MODES,
>  					      nlctx->ethnl2_socket);
> -		bits = mask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> -			      bitset_tb[ETHTOOL_A_BITSET_VALUE];
> +		bits = mask && !nomask ? bitset_tb[ETHTOOL_A_BITSET_MASK] :
> +			                 bitset_tb[ETHTOOL_A_BITSET_VALUE];
>  		ret = -EFAULT;
>  		if (!bits || !bitset_tb[ETHTOOL_A_BITSET_SIZE])
>  			goto err_nonl;
> @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset,
>  		if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] ||
>  		    !tb[ETHTOOL_A_BITSET_BIT_NAME])
>  			goto err;
> -		if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
> +		if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE])
>  			continue;
>  
>  		idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]);
> -- 
> 2.26.2
>