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

Message-ID: <87czkhp1c0.fsf@nvidia.com>
Date:   Mon, 24 Jan 2022 12:58:00 +0100
From:   Petr Machata <petrm@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Moshe Tal <moshet@...dia.com>,
        "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

Andrew Lunn <andrew@...n.ch> writes:

>> 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.
>
> It sounds to me like the type system is being bypassed somewhere. If
> the compiler knows we are assigning an emum to a u8, it should perform
> a cast and i would expect it to get it correct, independent of big or
> little endian. When that u8 is assigned back to an enum, the compiler
> should do another cast, and everything works out.
>
> I assume there are no compiler warnings? The enum -> u8 is an
> assignment to a smaller type, which is something you sometimes see
> compilers warn about. So it might be there is an explicit cast
> somewhere?

The only cast I'm aware of is the implicit cast in the call to
nla_put_u8(). The C standard has this to say about the conversions:

    When a value with integer type is converted to another integer type
    other than _Bool, if the value can be represented by the new type,
    it is unchanged.

There's more verbiage about what happens when it doesn't fit, but we are
on a happy path here.

I'm not especially well-versed in various warnings GCC gives, but I
think it only warns about expressions that involve literals. So
assigning an overlarge literal to a narrow type, or literal-only
expressions that would overflow the type of the expression.

> But you are saying this is not actually happening, the wrong end is
> being discarded. Should we not actually be trying to find where the
> type system is broken?

The mistake was in the union. Both the u8 and the enums are laid out to
start on the same address. When an enumerator is stored into an enum
field on a little-endian machine, the LSB is stored first, and that's
where u8 is laid out in the enum as well, so you get the enumerator
value back when reading the u8.

On big-endian machines however, the byte that u8 is laid out on is the
MSB, which is a zero in our case.

The underlying type for an enum is an integer. So de iure the fix should
have been u8->int, not u8->u32. Practically I suspect it does not
matter.