netdev - Re: [PATCH net] ethtool: Fix link extended state for big endian

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]

Date:   Sun, 23 Jan 2022 09:08:41 +0000
From:   Moshe Tal <moshet@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ido Schimmel <idosch@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Amit Cohen <amcohen@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        Petr Machata <petrm@...dia.com>, Gal Pressman <gal@...dia.com>,
        Tariq Toukan <tariqt@...dia.com>
Subject: Re: [PATCH net] ethtool: Fix link extended state for big endian

On 20/01/2022 16:43, Andrew Lunn wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Jan 20, 2022 at 11:55:50AM +0200, Moshe Tal wrote:
>> The link extended sub-states are assigned as enum that is an integer
>> size but read from a union as u8, this is working for small values on
>> little endian systems but for big endian this always give 0. Fix the
>> variable in the union to match the enum size.
>>
>> Fixes: ecc31c60240b ("ethtool: Add link extended state")
>> Signed-off-by: Moshe Tal <moshet@...dia.com>
>> Reviewed-by: Ido Schimmel <idosch@...dia.com>
>> Tested-by: Ido Schimmel <idosch@...dia.com>
>> Reviewed-by: Gal Pressman <gal@...dia.com>
>> Reviewed-by: Amit Cohen <amcohen@...dia.com>
>> ---
>>   include/linux/ethtool.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index a26f37a27167..11efc45de66a 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -111,7 +111,7 @@ struct ethtool_link_ext_state_info {
>>                enum ethtool_link_ext_substate_bad_signal_integrity bad_signal_integrity;
>>                enum ethtool_link_ext_substate_cable_issue cable_issue;
>>                enum ethtool_link_ext_substate_module module;
>> -             u8 __link_ext_substate;
>> +             u32 __link_ext_substate;
> 
> Not my area of expertise, but:
> 
> static int linkstate_reply_size(const struct ethnl_req_info *req_base,
>                                  const struct ethnl_reply_data *reply_base)
> {
>          struct linkstate_reply_data *data = LINKSTATE_REPDATA(reply_base);
>          int len;
> 
>         if (data->ethtool_link_ext_state_info.__link_ext_substate)
>                  len += nla_total_size(sizeof(u8)); /* LINKSTATE_EXT_SUBSTATE */
> 
> and
> 
> static int linkstate_fill_reply(struct sk_buff *skb,
>                                  const struct ethnl_req_info *req_base,
>                                  const struct ethnl_reply_data *reply_base)
> {
> 
>                  if (data->ethtool_link_ext_state_info.__link_ext_substate &&
>                      nla_put_u8(skb, ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,
>                                 data->ethtool_link_ext_state_info.__link_ext_substate))
>                          return -EMSGSIZE;
> 
> This seems to suggest it is a u8, not a u32.
> 
> I guess i don't understand something here...
> 
>    Andrew

The Netlink message was defined to only pass u8 and we can't change the 
message format without causing incompatibility issues.
So, we are assuming that values will be under 255.

Still, the compiler is storing enum as int, this isn't matter what the 
size of the other members of the union.
If it will be read into u8 - on BE systems the MSB will be read and so 
it will always pass a zero.
So this was solved by reading it as u32. Later, when the compiler will 
pass it to the function as u8 parameter, it will take the right part - 
the LSB on either system.

Reading enum by u8 from a union:
==================================
|             enum               |
==================================
|  u8   |
==================================
|  MSB  |        |       |  LSB  | On BE systems
==================================
|  LSB  |        |       |  MSB  | On LE systems
==================================

Converting u32 to u8:
  =========       =======
| u32 LSB |  => |   u8  |  On all the systems
  =========       =======