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: <10bd731c-8286-f62e-19d4-9ee567910392@intel.com>
Date:   Mon, 27 Jul 2020 15:32:34 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Michal Kubecek <mkubecek@...e.cz>
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 7/27/2020 3:21 PM, Michal Kubecek wrote:
> 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

The "mask" boolean is an indication that you want to print the mask for
a bitmap, rather than its value. I think treating a bitmap without a
predefined mask to have its mask be equivalent to its values is
reasonable. However, I could see the argument for not wanting this since
it is effectively a bug somewhere.

For dump_link_modes, it is trivial. If nomask is set, and mask is
requested, bail out of the function. It looks like we can also report an
error for the bitset_get_bit too. I'll take a look closer.

Thanks,
Jake