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] [day] [month] [year] [list]

Message-ID: <06f2b84a-c015-a878-6c36-745e32e17e5e@intel.com>
Date:   Mon, 27 Jul 2020 16:18:37 -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:53 PM, Michal Kubecek wrote:
> On Mon, Jul 27, 2020 at 03:32:34PM -0700, Jacob Keller wrote:
>> 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.
> 
> It depends on the context. In requests, value=0x1,mask=0x1 means "set
> bit 0 and leave the rest untouched" while nomask bitmap with value=0x1
> would mean "set bit 0 and clear the rest".
> 
> For kernel replies, it should be documented which variant is expected.
> 

Right, I was mostly referring to the reply side of things. I've updated
the patch in v2 to explicitly reject trying to print the mask of a NO
MASK bitset.

Thanks,
Jake