linux-kernel - Re: [PATCH] ARM: unaligned.h: Use an arch-specific version

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:   Fri, 22 Sep 2017 14:36:01 -0700
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Romain Izard <romain.izard.pro@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Al Viro <viro@...iv.linux.org.uk>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ARM: unaligned.h: Use an arch-specific version

On 20 September 2017 at 13:35, Arnd Bergmann <arnd@...db.de> wrote:
> On Wed, Sep 20, 2017 at 5:26 PM, Ard Biesheuvel
> <ard.biesheuvel@...aro.org> wrote:
>> On 20 September 2017 at 08:18, Romain Izard <romain.izard.pro@...il.com> wrote:
>>> Add an arch-specific header to ARM, to retain other optimizations that
>>> rely on HAVE_EFFICIENT_UNALIGNED_ACCESS, while making sure that access
>>> that explicitly rely on the unaligned accessors are correctly handled by
>>> the compiler.
>>>
>>> Signed-off-by: Romain Izard <romain.izard.pro@...il.com>
>>> ---
>>>
>>
>> If access_ok.h has been observed to produce different output from the
>> struct versions (using any compiler), I guess we cannot simply change
>> the asm-generic default and expect everybody to be ok with that. So I
>> agree this is the most appropriate course of action.
>
> But is that actually the case? I think patching ARM like this is
> correct, but perhaps we can simply remove the access_ok.h
> version entirely and just use the struct version everywhere.
>
> Unfortunately it's been almost 10 years since the various
> implementations got introduced, so it's hard to find out all
> the concerns that went into it then.
>
> Commit 064106a91be5 ("kernel: add common
> infrastructure for unaligned access") lists
>
>     Currently there are five implementations:
>     1) packed_struct.h: C-struct based, from asm-generic/unaligned.h
>     2) le_byteshift.h: Open coded byte-swapping, heavily based on asm-arm
>     3) be_byteshift.h: Open coded byte-swapping, heavily based on asm-arm
>     4) memmove.h: taken from multiple implementations in tree
>     5) access_ok.h: taken from x86 and others, unaligned access is ok.
>
> The only architectectures that use memmove.h are m32r (always has,
> probably not intentionally) and openrisc. That one says that GCC on
> OR32 "optimizes too aggressively" for struct.h, which is a bit scary
> but wouldn't change anything here since they don't use the asm-generic
> file.
>
> The architectures that do use include/asm-generic/unaligned.h and
> also set HAVE_EFFICIENT_UNALIGNED_ACCESS in some configurations
> are arm, arm64, metag, s390 and arc.
>
> This is a rather short list, and three of them (arm64, metag and arc) only
> support very recent compilers, so we can probably just ask the respective
> arch maintainers to ack the patch that changes the asm-generic file
> for everyone.
>

If we can limit the fallout like that, I agree that we should simply
make the struct flavor the default. It elegantly informs the compiler
about the size of the access and the potential misalignment, so it
should allow compilers for any architecture to select the most
appropriate instruction.

But doesn't that mean that any code that currently relies on
HAVE_EFFICIENT_UNALIGNED_ACCESS should be using get_unaligned instead?
I haven't reviewed the actual use cases (other than the ones I added
myself to the crypto subsystem), but it seems to me that it is
generally unsafe to do any unaligned accesses directly on ARM, given
that the compiler may merge adjacent LDRs into LDMs or LDRDs (and
likewise for stores)