netdev - Re: [PATCH] ethtool: fix SPEED_UNKNOWN definition to avoid signed-unsigned comparison

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: <E48ED982-95D4-4757-B643-82E943FF4AAC@akamai.com>
Date:   Wed, 3 Apr 2019 17:40:56 +0000
From:   "Zhivich, Michael" <mzhivich@...mai.com>
To:     David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dledford@...hat.com" <dledford@...hat.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "LinoSanfilippo@....de" <LinoSanfilippo@....de>,
        "thomas.lendacky@....com" <thomas.lendacky@....com>,
        "iyappan@...amperecomputing.com" <iyappan@...amperecomputing.com>,
        "quan@...amperecomputing.com" <quan@...amperecomputing.com>,
        "jcliburn@...il.com" <jcliburn@...il.com>,
        "siva.kallam@...adcom.com" <siva.kallam@...adcom.com>,
        "sgoutham@...ium.com" <sgoutham@...ium.com>,
        "shshaikh@...vell.com" <shshaikh@...vell.com>,
        "bh74.an@...sung.com" <bh74.an@...sung.com>,
        "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>
Subject: Re: [PATCH] ethtool: fix SPEED_UNKNOWN definition to avoid
 signed-unsigned comparison

On 4/2/19, 4:26 PM, "David Miller" <davem@...emloft.net> wrote:

> From: Michael Zhivich <mzhivich@...mai.com>
> Date: Mon, 1 Apr 2019 13:14:28 -0400
> 
>> When building C++ userspace code that includes ethtool.h
>> with "-Werror -Wall", g++ complains about signed-unsigned comparison in
>> ethtool_validate_speed() due to definition of SPEED_UNKNOWN as -1.
>> 
>> Change definition of SPEED_UNKNOWN to UINT_MAX to match the type of
>> ethtool_validate_speed() argument (__u32).
>> 
>> Update storage type for link speed in drivers to use u32 instead of int
>> or u16 to bring drivers up-to-date with ethtool.h which includes SPEED_*
>> constants larger than range of u16.
>> 
>> Fix usage of SPEED_* constants in
>> drivers/net/ethernet/cavium/thunder/thunder_bgx.c and
>> drivers/infiniband/ulp/ipoib/ipoib_ethtool.c.
>> 
>> Signed-off-by: Michael Zhivich <mzhivich@...mai.com>
>
>Waaaaay too many changes in one patch.  Do one thing at a time.

OK - I will refactor into a patch series.

>
>Also, are you absolutely sure that the things you changed from 'int'
>aren't used in other ways where the signedness matters?  For example
>for error returns?
>

I will double-check, but I haven't observed that during my last review.

>And maybe the drivers using u16 are legitimate because they are
>operating on speeds only within that bitmap range.  Type matching is
>not an end to itself.  So I don't want to see u16 --> u32 conversions
>where they aren't even necessary.
>

In principle, I would agree; however, there's a problem with keeping 
u16 storage with the current definition of ethtool_validate_speed().

Using *current* definitions, if a u16 variable is assigned SPEED_UNKNOWN (-1),
and then passed to ethtool_validate_speed(), the check will succeed,
but for the wrong reasons:  

(__u32)((u16)-1) <= INT_MAX, rather than (__u32)((u16)-1) == -1.

With *proposed* change of SPEED_UNKNOWN to UINT_MAX, using u16 storage 
will result in -Woverflow warnings during compilation, 
hence the changes to drivers.

>Also:
>
>>  /* Return lane speed in unit of 1e6 bit/sec */
>> -static inline int ib_speed_enum_to_int(int speed)
>> +static inline u32 ib_speed_enum_to_int(int speed)
>
>This is sloppy.
>
>You are changing the return type, but not adjusting the name of the
>function appropriately.  The function name says it converts to
>an 'int' but now it actually converts to and returns a u32.
>

Fair enough, will fix.

>You need to split up, audit, and present these changes more carefully
>and properly.
>
>Thank you.

Thanks for the quick response.

~ Michael