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: <20200727225359.hsy4bzmmfvrkw23e@lion.mk-sys.cz>
Date:   Tue, 28 Jul 2020 00:53:59 +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 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.

>             However, I could see the argument for not wanting this since
> it is effectively a bug somewhere.

We actually had this kind of bug in FEC modes handling as we just found.

Michal

> 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