linux-kernel - Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM

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:   Tue, 18 Apr 2017 10:34:01 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Ard Biesheuvel <ard.biesheuvel@...aro.org>
Cc:     Paul Gortmaker <paul.gortmaker@...driver.com>,
        Will Deacon <will.deacon@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Catalin Marinas <catalin.marinas@....com>,
        Greg Hackmann <ghackmann@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Robin Murphy <robin.murphy@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Grant Grundler <grundler@...omium.org>,
        Michael Davidson <md@...gle.com>
Subject: Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to
 ASM_EXPORT

El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:

> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@...driver.com> wrote:
> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@...omium.org> wrote:
> >> The operand is an integer constant, make the constness explicit by
> >> adding the modifier. This is needed for clang to generate valid code
> >> and also works with gcc.
> >
> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> > only use for syntax checking (and hence I don't care if it is the latest and
> > greatest) and this commit breaks it:
> >
> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> > operand prefix '%c'
> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));

Sorry about that :(

> > I'm currently reverting this change locally so I can continue to use the old
> > toolchain:
> >
> > $ aarch64-linux-gnu-gcc --version
> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> > Copyright (C) 2013 Free Software Foundation, Inc.
> >
> > $ aarch64-linux-gnu-as --version
> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> > 2013.11) 2.24.0.20131220
> > Copyright 2013 Free Software Foundation, Inc.
> >
> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >
> 
> Thanks for the report. I think we care more about GCC 4.8 than about
> Clang, which argues for reverting this patch.

I agree, we certainly shouldn't break GCC.

> I understand these issues must be frustrating if you are working on
> this stuff, but to me, it is not entirely obvious why we want to
> support Clang in the first place (i.e., what does it buy you if your
> distro/environment is not already using Clang for userland),

There are environments that use Clang for userland, like Chrome OS or
Android. Debian releases with GCC, but also does Clang builds of most
of its repository: http://clang.debian.net/

I would also argue that Linux directly benefits from additional error
checks provided by Clang.

> and why the burden is on Linux to make modifications to support
> Clang, especially when it comes to GCC extensions such as inline
> assembly syntax.

Personally I'm on the pragmatic side. 99.9x% of the kernel already
builds with Clang (at least for arm64 and x86), only a very tiny
fraction of the code needs adaptation, which should ideally result in
the same code for gcc and clang. And it's not like Clang is just
another proprietary or niche compiler, it's the other big open source
compiler out there, since the effort is relatively small it seems
desirable to support it without out-of-tree patches. In parallel there
is also work ongoing with upstream Clang to improve compatiblity.

> It is ultimately up to the maintainers to decide what to do with this
> patch, but my vote would be to revert it, especially given that the %c
> placeholder prefix is not documented anywhere, and appears to simply
> trigger some GCC internals that happen to do the right thing in this
> case.
> 
> However, the I -> i change is arguably an improvement, and considering
> that the following
> 
> asm("foo: .long %0" :: "i"(some value))
> 
> doesn't compile with clang either, I suggest you (Matthias) file a bug
> against Clang to get this fixed, and we can propose another patch just
> for the I->i change.

Ok, I will see if we can get this fixed in Clang.

Thanks

Matthias