linux-kernel - Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

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: <5747ae06-9641-613d-d8d0-dd9ce3f68af8@amd.com>
Date:   Tue, 28 Mar 2017 10:26:50 -0500
From:   Gary R Hook <ghook@....com>
To:     Arnd Bergmann <arnd@...db.de>, "Hook, Gary" <Gary.Hook@....com>
CC:     "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] crypto: ccp - Mark driver as little-endian only

On 03/28/2017 09:59 AM, Arnd Bergmann wrote:
> On Tue, Mar 28, 2017 at 4:08 PM, Gary R Hook <ghook@....com> wrote:
>
>>> In fact, the use of bit fields in hardware defined data structures is
>>> not portable to start with, so until all these bit fields get replaced
>>> by something else, the driver cannot work on big-endian machines, and
>>> I'm adding an annotation here to prevent it from being selected.
>>
>>
>> This is a driver that talks to hardware, a device which, AFAIK, has no
>> plan to be implemented in a big endian flavor. I clearly need to be more
>> diligent in building with various checkers enabled. I'd prefer my fix
>> over your suggested refusal to compile, if that's okay.
>
> It's hard to predict the future. If this device ever makes it into an
> ARM based chip, the chances are relatively high that someone
> will eventually run a big-endian kernel on it. As long as it's guaranteed
> to be x86-only, the risk of anyone running into the bug is close to
> zero, but we normally still try to write device drivers in portable C
> code to prevent it from getting copied incorrectly into another driver.

Understood, and I had surmised as such. Totally agree.

>>> The CCPv3 code seems to not suffer from this problem, only v5 uses
>>> bitfields.
>>
>>
>> Yes, I took a different approach when I wrote the code. IMO (arguably)
>> more readable. Same result: words full of hardware-dependent bit patterns.
>>
>> Please help me understand what I could do better.
>
> The rule for portable drivers is that you must not use bitfields in
> structures
> that can be accessed by the hardware. I think you can do this in a more
> readable way by removing the CCP5_CMD_* macros etc completely
> and just accessing the members of the structure as __le32 words.
> The main advantage for readability here is that you can grep for the
> struct members and see where they are used without following the
> macros. If it helps, you can also encapsulate the generation of the
> word inside of an inline function, like:
>

Please see my follow-on patch.

-- 
This is my day job. Follow me at:
IG/Twitter/Facebook: @grhookphoto
IG/Twitter/Facebook: @grhphotographer