[PATCH] crypto: curve25519-hacl64 - Disable fortify-source for clang-10

[Arnd Bergmann]

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 <Ja...@zx2c4.com>
Signed-off-by: Arnd Bergmann <ar...@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

[Jason A. Donenfeld]

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.

[Nick Desaulniers]

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

> --
> 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-li...@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

[Jason A. Donenfeld]

On Tue, May 5, 2020 at 3:48 PM Nick Desaulniers <ndesau...@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.

[Jason A. Donenfeld]

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 <ar...@arndb.de>
Cc: LKML <linux-...@vger.kernel.org>
Cc: clang-built-linux <clang-bu...@googlegroups.com>
Cc: Kees Cook <kees...@chromium.org>
Cc: George Burgess <gb...@google.com>
Cc: Nick Desaulniers <ndesau...@google.com>
Link: https://bugs.llvm.org/show_bug.cgi?id=45802
Signed-off-by: Jason A. Donenfeld <Ja...@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

[Nick Desaulniers]

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

[Nathan Chancellor]

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

> --
> 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-li...@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.

[Jason A. Donenfeld]

On Tue, May 5, 2020 at 4:25 PM Nathan Chancellor
<natecha...@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.

[George Burgess IV]

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 :)

> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200505222540.GA230458%40ubuntu-s3-xlarge-x86.

[Kees Cook]

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

[Jason A. Donenfeld]

On Tue, May 5, 2020 at 5:19 PM Kees Cook <kees...@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
> > <natecha...@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

[Jason A. Donenfeld]

On Tue, May 5, 2020 at 5:22 PM Jason A. Donenfeld <Ja...@zx2c4.com> wrote:
>
> On Tue, May 5, 2020 at 5:19 PM Kees Cook <kees...@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
> > > <natecha...@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.

[Nick Desaulniers]

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

[Jason A. Donenfeld]

clang-10 has a broken optimization stage that doesn't allow the

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 <ar...@arndb.de>
Cc: LKML <linux-...@vger.kernel.org>
Cc: clang-built-linux <clang-bu...@googlegroups.com>
Cc: Kees Cook <kees...@chromium.org>
Cc: George Burgess <gb...@google.com>
Cc: Nick Desaulniers <ndesau...@google.com>
Link: https://bugs.llvm.org/show_bug.cgi?id=45802
Signed-off-by: Jason A. Donenfeld <Ja...@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

[Nick Desaulniers]

Acked-by: Nick Desaulniers <ndesau...@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-li...@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

[Kees Cook]

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 <Ja...@zx2c4.com> wrote:
> >
> > On Tue, May 5, 2020 at 5:19 PM Kees Cook <kees...@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

[Kees Cook]

Grudgingly,

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

--
Kees Cook

[Jason A. Donenfeld]

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.

[Nathan Chancellor]

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 <natecha...@gmail.com>

[George Burgess]

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 <gb...@google.com>