linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10
@ 2020-05-05 13:59 Arnd Bergmann
  2020-05-05 21:39 ` Jason A. Donenfeld
  0 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2020-05-05 13:59 UTC (permalink / raw)
  To: Herbert Xu, David S. Miller
  Cc: Arnd Bergmann, Jason A . Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-kernel, clang-built-linux

clang-10 produces a warning about excessive stack usage, as well
as rather unoptimized object code when CONFIG_FORTIFY_SOURCE is
set:

lib/crypto/curve25519-hacl64.c:759:6: error: stack frame size of 2400 bytes in function 'curve25519_generic' [-Werror,-Wframe-larger-than=]

Jason Donenfeld managed to track this down to the usage of
CONFIG_FORTIFY_SOURCE, and I found a minimal test case that illustrates
this happening on clang-10 but not clang-9 or clang-11.

To work around this, turn off fortification in this file.

Link: https://bugs.llvm.org/show_bug.cgi?id=45802
Cc: Jason A. Donenfeld <Jason@zx2c4.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 lib/crypto/curve25519-hacl64.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/crypto/curve25519-hacl64.c b/lib/crypto/curve25519-hacl64.c
index c7de61829a66..87adeb4f9276 100644
--- a/lib/crypto/curve25519-hacl64.c
+++ b/lib/crypto/curve25519-hacl64.c
@@ -10,6 +10,10 @@
  * integer types.
  */
 
+#if (CONFIG_CLANG_VERSION >= 100000) && (CONFIG_CLANG_VERSION < 110000)
+#define __NO_FORTIFY /* https://bugs.llvm.org/show_bug.cgi?id=45802 */
+#endif
+
 #include <asm/unaligned.h>
 #include <crypto/curve25519.h>
 #include <linux/string.h>
-- 
2.26.0


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10
  2020-05-05 13:59 [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10 Arnd Bergmann
@ 2020-05-05 21:39 ` Jason A. Donenfeld
  2020-05-05 21:48   ` Nick Desaulniers
  2020-05-05 21:55   ` [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10 Jason A. Donenfeld
  0 siblings, 2 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 21:39 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Herbert Xu, David S. Miller, Ard Biesheuvel,
	Linux Crypto Mailing List, LKML, clang-built-linux

As discussed on IRC, this issue here isn't specific to this file, but
rather fortify source has some serious issues on clang-10, everywhere
in the kernel, and we should probably disable it globally for this
clang version. I'll follow up with some more info in a different
patch.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10
  2020-05-05 21:39 ` Jason A. Donenfeld
@ 2020-05-05 21:48   ` Nick Desaulniers
  2020-05-05 21:49     ` Jason A. Donenfeld
  2020-05-05 21:55   ` [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10 Jason A. Donenfeld
  1 sibling, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2020-05-05 21:48 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Arnd Bergmann, Herbert Xu, David S. Miller, Ard Biesheuvel,
	Linux Crypto Mailing List, LKML, clang-built-linux, Kees Cook,
	George Burgess

+ Kees, George, who have started looking into this, too.

On Tue, May 5, 2020 at 2:40 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> As discussed on IRC, this issue here isn't specific to this file, but
> rather fortify source has some serious issues on clang-10, everywhere
> in the kernel, and we should probably disable it globally for this
> clang version. I'll follow up with some more info in a different
> patch.
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAHmME9oMcfY4nwkknwN9c4rB-O7xD4GCAOFPoZCbdnq%3D034%3DVw%40mail.gmail.com.



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10
  2020-05-05 21:48   ` Nick Desaulniers
@ 2020-05-05 21:49     ` Jason A. Donenfeld
  0 siblings, 0 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 21:49 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Arnd Bergmann, Herbert Xu, David S. Miller, Ard Biesheuvel,
	Linux Crypto Mailing List, LKML, clang-built-linux, Kees Cook,
	George Burgess

On Tue, May 5, 2020 at 3:48 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> + Kees, George, who have started looking into this, too.

I have a smaller reproducer and analysis I'll send out very shortly.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 21:39 ` Jason A. Donenfeld
  2020-05-05 21:48   ` Nick Desaulniers
@ 2020-05-05 21:55   ` Jason A. Donenfeld
  2020-05-05 22:02     ` Nick Desaulniers
  1 sibling, 1 reply; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 21:55 UTC (permalink / raw)
  To: linux-crypto, linux-kernel, clang-built-linux, arnd
  Cc: Jason A. Donenfeld, Kees Cook, George Burgess, Nick Desaulniers

clang-10 has a broken optimization stage that doesn't enable the
compiler to prove at compile time that certain memcpys are within
bounds, and thus the outline memcpy is always called, resulting in
horrific performance, and in some cases, excessive stack frame growth.
Here's a simple reproducer:

    typedef unsigned long size_t;
    void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
    extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
    void blah(char *a)
    {
      unsigned long long b[10], c[10];
      int i;

      memcpy(b, a, sizeof(b));
      for (i = 0; i < 10; ++i)
        c[i] = b[i] ^ b[9 - i];
      for (i = 0; i < 10; ++i)
        b[i] = c[i] ^ a[i];
      memcpy(a, b, sizeof(b));
    }

Compile this with clang-9 and clang-10 and observe:

zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
void blah(char *a)
     ^
1 warning generated.
zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o

Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
properly optimized in the obvious way one would expect, while c10.o has
blown up and includes extern calls to memcpy.

This is present on the most trivial bits of code. Thus, for clang-10, we
just set __NO_FORTIFY globally, so that this issue won't be incurred.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: clang-built-linux <clang-built-linux@googlegroups.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: George Burgess <gbiv@google.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Link: https://bugs.llvm.org/show_bug.cgi?id=45802
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 Makefile | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Makefile b/Makefile
index 49b2709ff44e..f022f077591d 100644
--- a/Makefile
+++ b/Makefile
@@ -768,6 +768,13 @@ KBUILD_CFLAGS += -Wno-gnu
 # source of a reference will be _MergedGlobals and not on of the whitelisted names.
 # See modpost pattern 2
 KBUILD_CFLAGS += -mno-global-merge
+
+# clang-10 has a broken optimization stage that causes memcpy to always be
+# outline, resulting in excessive stack frame growth and poor performance.
+ifeq ($(shell test $(CONFIG_CLANG_VERSION) -ge 100000 && test $(CONFIG_CLANG_VERSION) -lt 110000; echo $$?),0)
+KBUILD_CFLAGS += -D__NO_FORTIFY
+endif
+
 else
 
 # These warnings generated too much noise in a regular build.
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 21:55   ` [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10 Jason A. Donenfeld
@ 2020-05-05 22:02     ` Nick Desaulniers
  2020-05-05 22:25       ` Nathan Chancellor
  0 siblings, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2020-05-05 22:02 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, Kees Cook, George Burgess

On Tue, May 5, 2020 at 2:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> clang-10 has a broken optimization stage that doesn't enable the
> compiler to prove at compile time that certain memcpys are within
> bounds, and thus the outline memcpy is always called, resulting in
> horrific performance, and in some cases, excessive stack frame growth.
> Here's a simple reproducer:
>
>     typedef unsigned long size_t;
>     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
>     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
>     void blah(char *a)
>     {
>       unsigned long long b[10], c[10];
>       int i;
>
>       memcpy(b, a, sizeof(b));
>       for (i = 0; i < 10; ++i)
>         c[i] = b[i] ^ b[9 - i];
>       for (i = 0; i < 10; ++i)
>         b[i] = c[i] ^ a[i];
>       memcpy(a, b, sizeof(b));
>     }
>
> Compile this with clang-9 and clang-10 and observe:
>
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> void blah(char *a)
>      ^
> 1 warning generated.
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
>
> Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> properly optimized in the obvious way one would expect, while c10.o has
> blown up and includes extern calls to memcpy.
>
> This is present on the most trivial bits of code. Thus, for clang-10, we
> just set __NO_FORTIFY globally, so that this issue won't be incurred.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: George Burgess <gbiv@google.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

I'm going to request this not be merged until careful comment from
George and Kees. My hands are quite full at the moment with other
regressions.  I suspect these threads may be relevant, though I
haven't had time to read through them myself.
https://github.com/ClangBuiltLinux/linux/issues/979
https://github.com/ClangBuiltLinux/linux/pull/980


> ---
>  Makefile | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 49b2709ff44e..f022f077591d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -768,6 +768,13 @@ KBUILD_CFLAGS += -Wno-gnu
>  # source of a reference will be _MergedGlobals and not on of the whitelisted names.
>  # See modpost pattern 2
>  KBUILD_CFLAGS += -mno-global-merge
> +
> +# clang-10 has a broken optimization stage that causes memcpy to always be
> +# outline, resulting in excessive stack frame growth and poor performance.
> +ifeq ($(shell test $(CONFIG_CLANG_VERSION) -ge 100000 && test $(CONFIG_CLANG_VERSION) -lt 110000; echo $$?),0)
> +KBUILD_CFLAGS += -D__NO_FORTIFY
> +endif
> +
>  else
>
>  # These warnings generated too much noise in a regular build.
> --
> 2.26.2
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 22:02     ` Nick Desaulniers
@ 2020-05-05 22:25       ` Nathan Chancellor
  2020-05-05 22:37         ` George Burgess IV
  2020-05-05 22:37         ` Jason A. Donenfeld
  0 siblings, 2 replies; 20+ messages in thread
From: Nathan Chancellor @ 2020-05-05 22:25 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jason A. Donenfeld,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, Kees Cook, George Burgess

On Tue, May 05, 2020 at 03:02:16PM -0700, 'Nick Desaulniers' via Clang Built Linux wrote:
> On Tue, May 5, 2020 at 2:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > clang-10 has a broken optimization stage that doesn't enable the
> > compiler to prove at compile time that certain memcpys are within
> > bounds, and thus the outline memcpy is always called, resulting in
> > horrific performance, and in some cases, excessive stack frame growth.
> > Here's a simple reproducer:
> >
> >     typedef unsigned long size_t;
> >     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
> >     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
> >     void blah(char *a)
> >     {
> >       unsigned long long b[10], c[10];
> >       int i;
> >
> >       memcpy(b, a, sizeof(b));
> >       for (i = 0; i < 10; ++i)
> >         c[i] = b[i] ^ b[9 - i];
> >       for (i = 0; i < 10; ++i)
> >         b[i] = c[i] ^ a[i];
> >       memcpy(a, b, sizeof(b));
> >     }
> >
> > Compile this with clang-9 and clang-10 and observe:
> >
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> > b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> > void blah(char *a)
> >      ^
> > 1 warning generated.
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> >
> > Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> > properly optimized in the obvious way one would expect, while c10.o has
> > blown up and includes extern calls to memcpy.
> >
> > This is present on the most trivial bits of code. Thus, for clang-10, we
> > just set __NO_FORTIFY globally, so that this issue won't be incurred.
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: George Burgess <gbiv@google.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> I'm going to request this not be merged until careful comment from
> George and Kees. My hands are quite full at the moment with other
> regressions.  I suspect these threads may be relevant, though I
> haven't had time to read through them myself.
> https://github.com/ClangBuiltLinux/linux/issues/979
> https://github.com/ClangBuiltLinux/linux/pull/980

I believe these issues are one in the same. I did a reverse bisect with
Arnd's test case and converged on George's first patch:

https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a

I think that in lieu of this patch, we should have that patch and its
follow-up fix merged into 10.0.1.

I can file an upstream PR for Tom to take a look out later tonight.

Cheers,
Nathan

> > ---
> >  Makefile | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/Makefile b/Makefile
> > index 49b2709ff44e..f022f077591d 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -768,6 +768,13 @@ KBUILD_CFLAGS += -Wno-gnu
> >  # source of a reference will be _MergedGlobals and not on of the whitelisted names.
> >  # See modpost pattern 2
> >  KBUILD_CFLAGS += -mno-global-merge
> > +
> > +# clang-10 has a broken optimization stage that causes memcpy to always be
> > +# outline, resulting in excessive stack frame growth and poor performance.
> > +ifeq ($(shell test $(CONFIG_CLANG_VERSION) -ge 100000 && test $(CONFIG_CLANG_VERSION) -lt 110000; echo $$?),0)
> > +KBUILD_CFLAGS += -D__NO_FORTIFY
> > +endif
> > +
> >  else
> >
> >  # These warnings generated too much noise in a regular build.
> > --
> > 2.26.2
> >
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers
> 
> -- 
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdk32cDowvrqRPKDRpf2ZiXh%3DjVnBTmhM-NWD%3DOwnq9v3w%40mail.gmail.com.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 22:25       ` Nathan Chancellor
@ 2020-05-05 22:37         ` George Burgess IV
  2020-05-05 22:37         ` Jason A. Donenfeld
  1 sibling, 0 replies; 20+ messages in thread
From: George Burgess IV @ 2020-05-05 22:37 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Nick Desaulniers, Jason A. Donenfeld,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, Kees Cook, George Burgess

This code generated by Clang here is the unfortunate side-effect of a
bug introduced during Clang-10's development phase. From discussion
with Kees on the links Nick mentioned, I surmise that FORTIFY in the
kernel never worked as well for Clang as it does for GCC today. In
many cases, it'd compile into nothing, but with the aforementioned
Clang bug, it would turn into very suboptimal code.

Kees sounded interested in getting a FORTIFY that plays more nicely
with Clang into the kernel. Until that happens, we'll be in a world
where an unpatched Clang-10 generates suboptimal code, and where a
patched Clang-10 only FORTIFYs a subset of the kernel's `mem*`/`str*`
functions. (I haven't checked assembly, but I assume that not every
FORTIFY'ed function gets compiled into 'nothingness').

I don't have sufficient context to be opinionated on whether it's
"better" to prefer a subset of opportune checks vs better codegen on
unpatched versions of clang.

If we do turn it off, it'd be nice to have some idea of when it can be
turned back on (do we need a modified implementation as mentioned
earlier? N months after clang's next point release is released,
provided the fixes land in it?)

> I can file an upstream PR for Tom to take a look out later tonight.

Thank you for the bisection and for handling the merge :)





On Tue, May 5, 2020 at 3:25 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, May 05, 2020 at 03:02:16PM -0700, 'Nick Desaulniers' via Clang Built Linux wrote:
> > On Tue, May 5, 2020 at 2:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > >
> > > clang-10 has a broken optimization stage that doesn't enable the
> > > compiler to prove at compile time that certain memcpys are within
> > > bounds, and thus the outline memcpy is always called, resulting in
> > > horrific performance, and in some cases, excessive stack frame growth.
> > > Here's a simple reproducer:
> > >
> > >     typedef unsigned long size_t;
> > >     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
> > >     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
> > >     void blah(char *a)
> > >     {
> > >       unsigned long long b[10], c[10];
> > >       int i;
> > >
> > >       memcpy(b, a, sizeof(b));
> > >       for (i = 0; i < 10; ++i)
> > >         c[i] = b[i] ^ b[9 - i];
> > >       for (i = 0; i < 10; ++i)
> > >         b[i] = c[i] ^ a[i];
> > >       memcpy(a, b, sizeof(b));
> > >     }
> > >
> > > Compile this with clang-9 and clang-10 and observe:
> > >
> > > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> > > b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> > > void blah(char *a)
> > >      ^
> > > 1 warning generated.
> > > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> > >
> > > Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> > > properly optimized in the obvious way one would expect, while c10.o has
> > > blown up and includes extern calls to memcpy.
> > >
> > > This is present on the most trivial bits of code. Thus, for clang-10, we
> > > just set __NO_FORTIFY globally, so that this issue won't be incurred.
> > >
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: LKML <linux-kernel@vger.kernel.org>
> > > Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: George Burgess <gbiv@google.com>
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
> > I'm going to request this not be merged until careful comment from
> > George and Kees. My hands are quite full at the moment with other
> > regressions.  I suspect these threads may be relevant, though I
> > haven't had time to read through them myself.
> > https://github.com/ClangBuiltLinux/linux/issues/979
> > https://github.com/ClangBuiltLinux/linux/pull/980
>
> I believe these issues are one in the same. I did a reverse bisect with
> Arnd's test case and converged on George's first patch:
>
> https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a
>
> I think that in lieu of this patch, we should have that patch and its
> follow-up fix merged into 10.0.1.
>
> I can file an upstream PR for Tom to take a look out later tonight.
>
> Cheers,
> Nathan
>
> > > ---
> > >  Makefile | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 49b2709ff44e..f022f077591d 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -768,6 +768,13 @@ KBUILD_CFLAGS += -Wno-gnu
> > >  # source of a reference will be _MergedGlobals and not on of the whitelisted names.
> > >  # See modpost pattern 2
> > >  KBUILD_CFLAGS += -mno-global-merge
> > > +
> > > +# clang-10 has a broken optimization stage that causes memcpy to always be
> > > +# outline, resulting in excessive stack frame growth and poor performance.
> > > +ifeq ($(shell test $(CONFIG_CLANG_VERSION) -ge 100000 && test $(CONFIG_CLANG_VERSION) -lt 110000; echo $$?),0)
> > > +KBUILD_CFLAGS += -D__NO_FORTIFY
> > > +endif
> > > +
> > >  else
> > >
> > >  # These warnings generated too much noise in a regular build.
> > > --
> > > 2.26.2
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdk32cDowvrqRPKDRpf2ZiXh%3DjVnBTmhM-NWD%3DOwnq9v3w%40mail.gmail.com.
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200505222540.GA230458%40ubuntu-s3-xlarge-x86.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 22:25       ` Nathan Chancellor
  2020-05-05 22:37         ` George Burgess IV
@ 2020-05-05 22:37         ` Jason A. Donenfeld
  2020-05-05 23:19           ` Kees Cook
  1 sibling, 1 reply; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 22:37 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Nick Desaulniers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, Kees Cook, George Burgess

On Tue, May 5, 2020 at 4:25 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> I believe these issues are one in the same. I did a reverse bisect with
> Arnd's test case and converged on George's first patch:
>
> https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a
>
> I think that in lieu of this patch, we should have that patch and its
> follow-up fix merged into 10.0.1.

If this is fixed in 10.0.1, do we even need to patch the kernel at
all? Or can we just leave it be, considering most organizations using
clang know what they're getting into? I'd personally prefer the
latter, so that we don't clutter things.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 22:37         ` Jason A. Donenfeld
@ 2020-05-05 23:19           ` Kees Cook
  2020-05-05 23:22             ` Jason A. Donenfeld
  2020-05-06  0:14             ` [PATCH v2] security: disable FORTIFY_SOURCE on clang Jason A. Donenfeld
  0 siblings, 2 replies; 20+ messages in thread
From: Kees Cook @ 2020-05-05 23:19 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Nathan Chancellor, Nick Desaulniers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, George Burgess

On Tue, May 05, 2020 at 04:37:38PM -0600, Jason A. Donenfeld wrote:
> On Tue, May 5, 2020 at 4:25 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> > I believe these issues are one in the same. I did a reverse bisect with
> > Arnd's test case and converged on George's first patch:
> >
> > https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a
> >
> > I think that in lieu of this patch, we should have that patch and its
> > follow-up fix merged into 10.0.1.
> 
> If this is fixed in 10.0.1, do we even need to patch the kernel at
> all? Or can we just leave it be, considering most organizations using
> clang know what they're getting into? I'd personally prefer the
> latter, so that we don't clutter things.

I agree: I'd rather this was fixed in 10.0.1 (but if we do want a
kernel-side work-around for 10.0.0, I would suggest doing the version
check in the Kconfig for FORTIFY_SOURCE instead of in the Makefile,
as that's where these things are supposed to live these days).

(Though as was mentioned, it's likely that FORTIFY_SOURCE isn't working
_at all_ under Clang, so I may still send a patch to depend on !clang
just to avoid surprises until it's fixed, but I haven't had time to
chase down a solution yet.)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 23:19           ` Kees Cook
@ 2020-05-05 23:22             ` Jason A. Donenfeld
  2020-05-05 23:22               ` Jason A. Donenfeld
  2020-05-05 23:36               ` Nick Desaulniers
  2020-05-06  0:14             ` [PATCH v2] security: disable FORTIFY_SOURCE on clang Jason A. Donenfeld
  1 sibling, 2 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 23:22 UTC (permalink / raw)
  To: Kees Cook
  Cc: Nathan Chancellor, Nick Desaulniers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, George Burgess

On Tue, May 5, 2020 at 5:19 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 05, 2020 at 04:37:38PM -0600, Jason A. Donenfeld wrote:
> > On Tue, May 5, 2020 at 4:25 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > > I believe these issues are one in the same. I did a reverse bisect with
> > > Arnd's test case and converged on George's first patch:
> > >
> > > https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a
> > >
> > > I think that in lieu of this patch, we should have that patch and its
> > > follow-up fix merged into 10.0.1.
> >
> > If this is fixed in 10.0.1, do we even need to patch the kernel at
> > all? Or can we just leave it be, considering most organizations using
> > clang know what they're getting into? I'd personally prefer the
> > latter, so that we don't clutter things.
>
> I agree: I'd rather this was fixed in 10.0.1 (but if we do want a
> kernel-side work-around for 10.0.0, I would suggest doing the version
> check in the Kconfig for FORTIFY_SOURCE instead of in the Makefile,
> as that's where these things are supposed to live these days).

Indeed this belongs in the Makefile. I can send a patch adjusting
that, if you want, but I think I'd rather do nothing and have a fix be
rolled out in 10.0.1. Clang users should know what to expect in that
regard.

> (Though as was mentioned, it's likely that FORTIFY_SOURCE isn't working
> _at all_ under Clang, so I may still send a patch to depend on !clang
> just to avoid surprises until it's fixed, but I haven't had time to
> chase down a solution yet.)

That might be the most coherent thing to do, at least so that people
don't get some false sense of mitigation.

Jason

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 23:22             ` Jason A. Donenfeld
@ 2020-05-05 23:22               ` Jason A. Donenfeld
  2020-05-05 23:36               ` Nick Desaulniers
  1 sibling, 0 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-05 23:22 UTC (permalink / raw)
  To: Kees Cook
  Cc: Nathan Chancellor, Nick Desaulniers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann, George Burgess

On Tue, May 5, 2020 at 5:22 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Tue, May 5, 2020 at 5:19 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Tue, May 05, 2020 at 04:37:38PM -0600, Jason A. Donenfeld wrote:
> > > On Tue, May 5, 2020 at 4:25 PM Nathan Chancellor
> > > <natechancellor@gmail.com> wrote:
> > > > I believe these issues are one in the same. I did a reverse bisect with
> > > > Arnd's test case and converged on George's first patch:
> > > >
> > > > https://github.com/llvm/llvm-project/commit/2dd17ff08165e6118e70f00e22b2c36d2d4e0a9a
> > > >
> > > > I think that in lieu of this patch, we should have that patch and its
> > > > follow-up fix merged into 10.0.1.
> > >
> > > If this is fixed in 10.0.1, do we even need to patch the kernel at
> > > all? Or can we just leave it be, considering most organizations using
> > > clang know what they're getting into? I'd personally prefer the
> > > latter, so that we don't clutter things.
> >
> > I agree: I'd rather this was fixed in 10.0.1 (but if we do want a
> > kernel-side work-around for 10.0.0, I would suggest doing the version
> > check in the Kconfig for FORTIFY_SOURCE instead of in the Makefile,
> > as that's where these things are supposed to live these days).
>
> Indeed this belongs in the Makefile. I can send a patch adjusting

er, Kconfig, is what I meant to type.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 23:22             ` Jason A. Donenfeld
  2020-05-05 23:22               ` Jason A. Donenfeld
@ 2020-05-05 23:36               ` Nick Desaulniers
  2020-05-06  2:53                 ` Kees Cook
  1 sibling, 1 reply; 20+ messages in thread
From: Nick Desaulniers @ 2020-05-05 23:36 UTC (permalink / raw)
  To: Jason A. Donenfeld, Kees Cook, George Burgess
  Cc: Nathan Chancellor,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann

On Tue, May 5, 2020 at 4:22 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Tue, May 5, 2020 at 5:19 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > (Though as was mentioned, it's likely that FORTIFY_SOURCE isn't working
> > _at all_ under Clang, so I may still send a patch to depend on !clang
> > just to avoid surprises until it's fixed, but I haven't had time to
> > chase down a solution yet.)

Not good.  If it's completely broken, turn it off, and we'll prioritize fixing.

> That might be the most coherent thing to do, at least so that people
> don't get some false sense of mitigation.

Do we have a better test for "this is working as intended" or not
other than excessive stack usage, since that doesn't repro for
clang-9?
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-05 23:19           ` Kees Cook
  2020-05-05 23:22             ` Jason A. Donenfeld
@ 2020-05-06  0:14             ` Jason A. Donenfeld
  2020-05-06  0:57               ` Nick Desaulniers
  2020-05-06  2:54               ` Kees Cook
  1 sibling, 2 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-06  0:14 UTC (permalink / raw)
  To: linux-kernel, clang-built-linux, arnd
  Cc: Jason A. Donenfeld, Kees Cook, George Burgess, Nick Desaulniers

clang-10 has a broken optimization stage that doesn't allow the
compiler to prove at compile time that certain memcpys are within
bounds, and thus the outline memcpy is always called, resulting in
horrific performance, and in some cases, excessive stack frame growth.
Here's a simple reproducer:

    typedef unsigned long size_t;
    void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
    extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
    void blah(char *a)
    {
      unsigned long long b[10], c[10];
      int i;

      memcpy(b, a, sizeof(b));
      for (i = 0; i < 10; ++i)
        c[i] = b[i] ^ b[9 - i];
      for (i = 0; i < 10; ++i)
        b[i] = c[i] ^ a[i];
      memcpy(a, b, sizeof(b));
    }

Compile this with clang-9 and clang-10 and observe:

zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
void blah(char *a)
     ^
1 warning generated.
zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o

Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
properly optimized in the obvious way one would expect, while c10.o has
blown up and includes extern calls to memcpy.

But actually, for versions of clang earlier than 10, fortify source
mostly does nothing. So, between being broken and doing nothing, it
probably doesn't make sense to pretend to offer this option. So, this
commit just disables it entirely when compiling with clang.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: clang-built-linux <clang-built-linux@googlegroups.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: George Burgess <gbiv@google.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Link: https://bugs.llvm.org/show_bug.cgi?id=45802
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 security/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/Kconfig b/security/Kconfig
index cd3cc7da3a55..76bcfb3eb16f 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -191,6 +191,7 @@ config HARDENED_USERCOPY_PAGESPAN
 config FORTIFY_SOURCE
 	bool "Harden common str/mem functions against buffer overflows"
 	depends on ARCH_HAS_FORTIFY_SOURCE
+	depends on !CC_IS_CLANG
 	help
 	  Detect overflows of buffers in common string and memory functions
 	  where the compiler can determine and validate the buffer sizes.
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-06  0:14             ` [PATCH v2] security: disable FORTIFY_SOURCE on clang Jason A. Donenfeld
@ 2020-05-06  0:57               ` Nick Desaulniers
  2020-05-06  2:54               ` Kees Cook
  1 sibling, 0 replies; 20+ messages in thread
From: Nick Desaulniers @ 2020-05-06  0:57 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: LKML, clang-built-linux, Arnd Bergmann, Kees Cook, George Burgess

On Tue, May 5, 2020 at 5:15 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> clang-10 has a broken optimization stage that doesn't allow the
> compiler to prove at compile time that certain memcpys are within
> bounds, and thus the outline memcpy is always called, resulting in
> horrific performance, and in some cases, excessive stack frame growth.
> Here's a simple reproducer:
>
>     typedef unsigned long size_t;
>     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
>     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
>     void blah(char *a)
>     {
>       unsigned long long b[10], c[10];
>       int i;
>
>       memcpy(b, a, sizeof(b));
>       for (i = 0; i < 10; ++i)
>         c[i] = b[i] ^ b[9 - i];
>       for (i = 0; i < 10; ++i)
>         b[i] = c[i] ^ a[i];
>       memcpy(a, b, sizeof(b));
>     }
>
> Compile this with clang-9 and clang-10 and observe:
>
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> void blah(char *a)
>      ^
> 1 warning generated.
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
>
> Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> properly optimized in the obvious way one would expect, while c10.o has
> blown up and includes extern calls to memcpy.
>
> But actually, for versions of clang earlier than 10, fortify source
> mostly does nothing. So, between being broken and doing nothing, it
> probably doesn't make sense to pretend to offer this option. So, this
> commit just disables it entirely when compiling with clang.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: George Burgess <gbiv@google.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Acked-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  security/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index cd3cc7da3a55..76bcfb3eb16f 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -191,6 +191,7 @@ config HARDENED_USERCOPY_PAGESPAN
>  config FORTIFY_SOURCE
>         bool "Harden common str/mem functions against buffer overflows"
>         depends on ARCH_HAS_FORTIFY_SOURCE
> +       depends on !CC_IS_CLANG
>         help
>           Detect overflows of buffers in common string and memory functions
>           where the compiler can determine and validate the buffer sizes.
> --
> 2.26.2
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200506001453.764332-1-Jason%40zx2c4.com.



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10
  2020-05-05 23:36               ` Nick Desaulniers
@ 2020-05-06  2:53                 ` Kees Cook
  0 siblings, 0 replies; 20+ messages in thread
From: Kees Cook @ 2020-05-06  2:53 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Jason A. Donenfeld, George Burgess, Nathan Chancellor,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, LKML,
	clang-built-linux, Arnd Bergmann

On Tue, May 05, 2020 at 04:36:49PM -0700, Nick Desaulniers wrote:
> On Tue, May 5, 2020 at 4:22 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > On Tue, May 5, 2020 at 5:19 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > (Though as was mentioned, it's likely that FORTIFY_SOURCE isn't working
> > > _at all_ under Clang, so I may still send a patch to depend on !clang
> > > just to avoid surprises until it's fixed, but I haven't had time to
> > > chase down a solution yet.)
> 
> Not good.  If it's completely broken, turn it off, and we'll prioritize fixing.

The problem is what George mentioned: it's unclear how broken it is --
it may not be all uses. I haven't had time to finish the testing for it,
but it's on the TODO list. :)

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-06  0:14             ` [PATCH v2] security: disable FORTIFY_SOURCE on clang Jason A. Donenfeld
  2020-05-06  0:57               ` Nick Desaulniers
@ 2020-05-06  2:54               ` Kees Cook
  2020-05-06  3:34                 ` Jason A. Donenfeld
  2020-05-06  3:53                 ` Nathan Chancellor
  1 sibling, 2 replies; 20+ messages in thread
From: Kees Cook @ 2020-05-06  2:54 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, clang-built-linux, arnd, George Burgess, Nick Desaulniers

On Tue, May 05, 2020 at 06:14:53PM -0600, Jason A. Donenfeld wrote:
> clang-10 has a broken optimization stage that doesn't allow the
> compiler to prove at compile time that certain memcpys are within
> bounds, and thus the outline memcpy is always called, resulting in
> horrific performance, and in some cases, excessive stack frame growth.
> Here's a simple reproducer:
> 
>     typedef unsigned long size_t;
>     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
>     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
>     void blah(char *a)
>     {
>       unsigned long long b[10], c[10];
>       int i;
> 
>       memcpy(b, a, sizeof(b));
>       for (i = 0; i < 10; ++i)
>         c[i] = b[i] ^ b[9 - i];
>       for (i = 0; i < 10; ++i)
>         b[i] = c[i] ^ a[i];
>       memcpy(a, b, sizeof(b));
>     }
> 
> Compile this with clang-9 and clang-10 and observe:
> 
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> void blah(char *a)
>      ^
> 1 warning generated.
> zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> 
> Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> properly optimized in the obvious way one would expect, while c10.o has
> blown up and includes extern calls to memcpy.
> 
> But actually, for versions of clang earlier than 10, fortify source
> mostly does nothing. So, between being broken and doing nothing, it
> probably doesn't make sense to pretend to offer this option. So, this
> commit just disables it entirely when compiling with clang.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: George Burgess <gbiv@google.com>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Grudgingly,

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-06  2:54               ` Kees Cook
@ 2020-05-06  3:34                 ` Jason A. Donenfeld
  2020-05-06  3:53                 ` Nathan Chancellor
  1 sibling, 0 replies; 20+ messages in thread
From: Jason A. Donenfeld @ 2020-05-06  3:34 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, clang-built-linux, Arnd Bergmann, George Burgess, Nick Desaulniers

On Tue, May 5, 2020 at 8:54 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, May 05, 2020 at 06:14:53PM -0600, Jason A. Donenfeld wrote:
> > clang-10 has a broken optimization stage that doesn't allow the
> > compiler to prove at compile time that certain memcpys are within
> > bounds, and thus the outline memcpy is always called, resulting in
> > horrific performance, and in some cases, excessive stack frame growth.
> > Here's a simple reproducer:
> >
> >     typedef unsigned long size_t;
> >     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
> >     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
> >     void blah(char *a)
> >     {
> >       unsigned long long b[10], c[10];
> >       int i;
> >
> >       memcpy(b, a, sizeof(b));
> >       for (i = 0; i < 10; ++i)
> >         c[i] = b[i] ^ b[9 - i];
> >       for (i = 0; i < 10; ++i)
> >         b[i] = c[i] ^ a[i];
> >       memcpy(a, b, sizeof(b));
> >     }
> >
> > Compile this with clang-9 and clang-10 and observe:
> >
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> > b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> > void blah(char *a)
> >      ^
> > 1 warning generated.
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> >
> > Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> > properly optimized in the obvious way one would expect, while c10.o has
> > blown up and includes extern calls to memcpy.
> >
> > But actually, for versions of clang earlier than 10, fortify source
> > mostly does nothing. So, between being broken and doing nothing, it
> > probably doesn't make sense to pretend to offer this option. So, this
> > commit just disables it entirely when compiling with clang.
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: George Burgess <gbiv@google.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>
> Grudgingly,
>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Do you want to take this into your tree to send to Linus? Seems like
security kconfig switches is in line with your usual submissions.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-06  2:54               ` Kees Cook
  2020-05-06  3:34                 ` Jason A. Donenfeld
@ 2020-05-06  3:53                 ` Nathan Chancellor
  2020-05-06 16:48                   ` George Burgess
  1 sibling, 1 reply; 20+ messages in thread
From: Nathan Chancellor @ 2020-05-06  3:53 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jason A. Donenfeld, linux-kernel, clang-built-linux, arnd,
	George Burgess, Nick Desaulniers

On Tue, May 05, 2020 at 07:54:09PM -0700, Kees Cook wrote:
> On Tue, May 05, 2020 at 06:14:53PM -0600, Jason A. Donenfeld wrote:
> > clang-10 has a broken optimization stage that doesn't allow the
> > compiler to prove at compile time that certain memcpys are within
> > bounds, and thus the outline memcpy is always called, resulting in
> > horrific performance, and in some cases, excessive stack frame growth.
> > Here's a simple reproducer:
> > 
> >     typedef unsigned long size_t;
> >     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
> >     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
> >     void blah(char *a)
> >     {
> >       unsigned long long b[10], c[10];
> >       int i;
> > 
> >       memcpy(b, a, sizeof(b));
> >       for (i = 0; i < 10; ++i)
> >         c[i] = b[i] ^ b[9 - i];
> >       for (i = 0; i < 10; ++i)
> >         b[i] = c[i] ^ a[i];
> >       memcpy(a, b, sizeof(b));
> >     }
> > 
> > Compile this with clang-9 and clang-10 and observe:
> > 
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> > b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> > void blah(char *a)
> >      ^
> > 1 warning generated.
> > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> > 
> > Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> > properly optimized in the obvious way one would expect, while c10.o has
> > blown up and includes extern calls to memcpy.
> > 
> > But actually, for versions of clang earlier than 10, fortify source
> > mostly does nothing. So, between being broken and doing nothing, it
> > probably doesn't make sense to pretend to offer this option. So, this
> > commit just disables it entirely when compiling with clang.
> > 
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: George Burgess <gbiv@google.com>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> 
> Grudgingly,
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> -- 
> Kees Cook
> 

I feel like you should finish your investigation into how broken this
actually is before we give it the hammer like this but if it is going
in regardless...

Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] security: disable FORTIFY_SOURCE on clang
  2020-05-06  3:53                 ` Nathan Chancellor
@ 2020-05-06 16:48                   ` George Burgess
  0 siblings, 0 replies; 20+ messages in thread
From: George Burgess @ 2020-05-06 16:48 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Kees Cook, Jason A. Donenfeld, linux-kernel, clang-built-linux,
	arnd, Nick Desaulniers

I took a bit to poke Clang here. Building an arbitrary file with
`CONFIG_FORTIFY_SOURCE=y`, none of the functions in this range
https://github.com/ClangBuiltLinux/linux/blob/0bee0cece/include/linux/string.h#L274-L468
have FORTIFY'ed definitions emitted by clang, i.e., the added FORTIFY checks
aren't helping. Happy to check other functions elsewhere if they exist,
but given that this entire block seems to be a functional nop...

Reviewed-by: George Burgess IV <gbiv@google.com>


On Tue, May 5, 2020 at 8:53 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, May 05, 2020 at 07:54:09PM -0700, Kees Cook wrote:
> > On Tue, May 05, 2020 at 06:14:53PM -0600, Jason A. Donenfeld wrote:
> > > clang-10 has a broken optimization stage that doesn't allow the
> > > compiler to prove at compile time that certain memcpys are within
> > > bounds, and thus the outline memcpy is always called, resulting in
> > > horrific performance, and in some cases, excessive stack frame growth.
> > > Here's a simple reproducer:
> > >
> > >     typedef unsigned long size_t;
> > >     void *c(void *dest, const void *src, size_t n) __asm__("memcpy");
> > >     extern inline __attribute__((gnu_inline)) void *memcpy(void *dest, const void *src, size_t n) { return c(dest, src, n); }
> > >     void blah(char *a)
> > >     {
> > >       unsigned long long b[10], c[10];
> > >       int i;
> > >
> > >       memcpy(b, a, sizeof(b));
> > >       for (i = 0; i < 10; ++i)
> > >         c[i] = b[i] ^ b[9 - i];
> > >       for (i = 0; i < 10; ++i)
> > >         b[i] = c[i] ^ a[i];
> > >       memcpy(a, b, sizeof(b));
> > >     }
> > >
> > > Compile this with clang-9 and clang-10 and observe:
> > >
> > > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-10 -Wframe-larger-than=0 -O3 -c b.c -o c10.o
> > > b.c:5:6: warning: stack frame size of 104 bytes in function 'blah' [-Wframe-larger-than=]
> > > void blah(char *a)
> > >      ^
> > > 1 warning generated.
> > > zx2c4@thinkpad /tmp/curve25519-hacl64-stack-frame-size-test $ clang-9 -Wframe-larger-than=0 -O3 -c b.c -o c9.o
> > >
> > > Looking at the disassembly of c10.o and c9.o, one can see that c9.o is
> > > properly optimized in the obvious way one would expect, while c10.o has
> > > blown up and includes extern calls to memcpy.
> > >
> > > But actually, for versions of clang earlier than 10, fortify source
> > > mostly does nothing. So, between being broken and doing nothing, it
> > > probably doesn't make sense to pretend to offer this option. So, this
> > > commit just disables it entirely when compiling with clang.
> > >
> > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > Cc: LKML <linux-kernel@vger.kernel.org>
> > > Cc: clang-built-linux <clang-built-linux@googlegroups.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: George Burgess <gbiv@google.com>
> > > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > > Link: https://bugs.llvm.org/show_bug.cgi?id=45802
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >
> > Grudgingly,
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > --
> > Kees Cook
> >
>
> I feel like you should finish your investigation into how broken this
> actually is before we give it the hammer like this but if it is going
> in regardless...
>
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-05-06 16:49 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 13:59 [PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10 Arnd Bergmann
2020-05-05 21:39 ` Jason A. Donenfeld
2020-05-05 21:48   ` Nick Desaulniers
2020-05-05 21:49     ` Jason A. Donenfeld
2020-05-05 21:55   ` [PATCH] Kbuild: disable FORTIFY_SOURCE on clang-10 Jason A. Donenfeld
2020-05-05 22:02     ` Nick Desaulniers
2020-05-05 22:25       ` Nathan Chancellor
2020-05-05 22:37         ` George Burgess IV
2020-05-05 22:37         ` Jason A. Donenfeld
2020-05-05 23:19           ` Kees Cook
2020-05-05 23:22             ` Jason A. Donenfeld
2020-05-05 23:22               ` Jason A. Donenfeld
2020-05-05 23:36               ` Nick Desaulniers
2020-05-06  2:53                 ` Kees Cook
2020-05-06  0:14             ` [PATCH v2] security: disable FORTIFY_SOURCE on clang Jason A. Donenfeld
2020-05-06  0:57               ` Nick Desaulniers
2020-05-06  2:54               ` Kees Cook
2020-05-06  3:34                 ` Jason A. Donenfeld
2020-05-06  3:53                 ` Nathan Chancellor
2020-05-06 16:48                   ` George Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).