[PATCH v2 0/5] -ffreestanding/-fno-builtin-* patches

[PATCH v2 0/5] -ffreestanding/-fno-builtin-* patches

[Nick Desaulniers]

A recent "libcall optimization" addition to LLVM will emit libcalls to
stpcpy, which the kernel doesn't provide an implementation, breaking
almost all kernel builds with ToT Clang. Disable it for clang.

We discussed providing an implementation, but the interface is generally
unsafe as it provides no bounds checking.

-fno-builtin-foo doesn't prevent GCC from emitting calls to foo, and GCC
doesn't currently do the same libcall optimizations. If it ever does,
then we can resurrect these implementations, but right now, YAGNI. So we
only add these flags to CLANG_FLAGS to solve a Clang specific issue.

The first patch is critical, I'm hoping Masahiro will pick it for the
Kbuild tree and help us to get the fix in 5.9.

The rest are cleanups; sending them for feedback/review/testing.  Once
the first hits mainline, I plan to resend the rest to the x86
maintainers for inclusion in tip.

Nick Desaulniers (5):
  Makefile: add -fno-builtin-stpcpy
  Makefile: add -fno-builtin-bcmp
  Revert "lib/string.c: implement a basic bcmp"
  x86/boot: use -fno-builtin-bcmp
  x86: don't build CONFIG_X86_32 as -ffreestanding

 Makefile               |  2 ++
 arch/x86/Makefile      |  3 ---
 arch/x86/boot/Makefile |  1 +
 arch/x86/boot/string.c |  8 --------
 include/linux/string.h |  3 ---
 lib/string.c           | 20 --------------------
 6 files changed, 3 insertions(+), 34 deletions(-)

-- 
2.28.0.297.g1956fa8f8d-goog

[PATCH v2 1/5] Makefile: add -fno-builtin-stpcpy

[Nick Desaulniers]

LLVM implemented a recent "libcall optimization" that lowers calls to
`sprintf(dest, "%s", str)` where the return value is used to
`stpcpy(dest, str) - dest`. This generally avoids the machinery involved
in parsing format strings. This optimization was introduced into
clang-12. Because the kernel does not provide an implementation of
stpcpy, we observe linkage failures for almost all targets when building
with ToT clang.

The interface is unsafe as it does not perform any bounds checking.
Disable this "libcall optimization" via `-fno-builtin-stpcpy`.

Cc: stable@vger.kernel.org # 4.4
Link: https://bugs.llvm.org/show_bug.cgi?id=47162
Link: https://github.com/ClangBuiltLinux/linux/issues/1126
Link: https://reviews.llvm.org/D85963
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Suggested-by: Dávid Bolvanský <david.bolvansky@gmail.com>
Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 9cac6fde3479..e523dc8d30e0 100644
--- a/Makefile
+++ b/Makefile
@@ -578,6 +578,7 @@ ifneq ($(LLVM_IAS),1)
 CLANG_FLAGS	+= -no-integrated-as
 endif
 CLANG_FLAGS	+= -Werror=unknown-warning-option
+CLANG_FLAGS	+= -fno-builtin-stpcpy
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH v2 1/5] Makefile: add -fno-builtin-stpcpy

[Nathan Chancellor]

On Wed, Aug 19, 2020 at 12:16:50PM -0700, Nick Desaulniers wrote:
> LLVM implemented a recent "libcall optimization" that lowers calls to
> `sprintf(dest, "%s", str)` where the return value is used to
> `stpcpy(dest, str) - dest`. This generally avoids the machinery involved
> in parsing format strings. This optimization was introduced into
> clang-12. Because the kernel does not provide an implementation of
> stpcpy, we observe linkage failures for almost all targets when building
> with ToT clang.
> 
> The interface is unsafe as it does not perform any bounds checking.
> Disable this "libcall optimization" via `-fno-builtin-stpcpy`.
> 
> Cc: stable@vger.kernel.org # 4.4
> Link: https://bugs.llvm.org/show_bug.cgi?id=47162
> Link: https://github.com/ClangBuiltLinux/linux/issues/1126
> Link: https://reviews.llvm.org/D85963
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Suggested-by: Dávid Bolvanský <david.bolvansky@gmail.com>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

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

> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index 9cac6fde3479..e523dc8d30e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -578,6 +578,7 @@ ifneq ($(LLVM_IAS),1)
>  CLANG_FLAGS	+= -no-integrated-as
>  endif
>  CLANG_FLAGS	+= -Werror=unknown-warning-option
> +CLANG_FLAGS	+= -fno-builtin-stpcpy
>  KBUILD_CFLAGS	+= $(CLANG_FLAGS)
>  KBUILD_AFLAGS	+= $(CLANG_FLAGS)
>  export CLANG_FLAGS
> -- 
> 2.28.0.297.g1956fa8f8d-goog
> 

[PATCH v2 2/5] Makefile: add -fno-builtin-bcmp

[Nick Desaulniers]

The issue with using `-fno-builtin-*` flags was that they were not
retained during an LTO link with LLVM.  This was fixed in clang-11 by
https://reviews.llvm.org/D71193
(0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum
supported version of clang for LTO.  Use `-fno-builtin-bcmp` instead.

With this applid, we can cleanly revert
commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")

Reviewed-by: Kees Cook <keescook@chromium.org>
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index e523dc8d30e0..def590b743a9 100644
--- a/Makefile
+++ b/Makefile
@@ -579,6 +579,7 @@ CLANG_FLAGS	+= -no-integrated-as
 endif
 CLANG_FLAGS	+= -Werror=unknown-warning-option
 CLANG_FLAGS	+= -fno-builtin-stpcpy
+CLANG_FLAGS	+= -fno-builtin-bcmp
 KBUILD_CFLAGS	+= $(CLANG_FLAGS)
 KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH v2 2/5] Makefile: add -fno-builtin-bcmp

[Nathan Chancellor]

On Wed, Aug 19, 2020 at 12:16:51PM -0700, Nick Desaulniers wrote:
> The issue with using `-fno-builtin-*` flags was that they were not
> retained during an LTO link with LLVM.  This was fixed in clang-11 by
> https://reviews.llvm.org/D71193
> (0508c994f0b14144041f2cfd3ba9f9a80f03de08), which is also the minimum
> supported version of clang for LTO.  Use `-fno-builtin-bcmp` instead.
> 
> With this applid, we can cleanly revert
> commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

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

> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index e523dc8d30e0..def590b743a9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -579,6 +579,7 @@ CLANG_FLAGS	+= -no-integrated-as
>  endif
>  CLANG_FLAGS	+= -Werror=unknown-warning-option
>  CLANG_FLAGS	+= -fno-builtin-stpcpy
> +CLANG_FLAGS	+= -fno-builtin-bcmp
>  KBUILD_CFLAGS	+= $(CLANG_FLAGS)
>  KBUILD_AFLAGS	+= $(CLANG_FLAGS)
>  export CLANG_FLAGS
> -- 
> 2.28.0.297.g1956fa8f8d-goog
> 

[PATCH v2 3/5] Revert "lib/string.c: implement a basic bcmp"

[Nick Desaulniers]

This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342.

An earlier commit in the series prevents the compiler from emitting
calls to bcmp as part of "libcall optimization," and there are no
explicit callers, so we can now safely remove this interface.

Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 include/linux/string.h |  3 ---
 lib/string.c           | 20 --------------------
 2 files changed, 23 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b1f3894a0a3e..f3bdb74bc230 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t);
 #ifndef __HAVE_ARCH_MEMCMP
 extern int memcmp(const void *,const void *,__kernel_size_t);
 #endif
-#ifndef __HAVE_ARCH_BCMP
-extern int bcmp(const void *,const void *,__kernel_size_t);
-#endif
 #ifndef __HAVE_ARCH_MEMCHR
 extern void * memchr(const void *,int,__kernel_size_t);
 #endif
diff --git a/lib/string.c b/lib/string.c
index 6012c385fb31..69328b8353e1 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count)
 EXPORT_SYMBOL(memcmp);
 #endif
 
-#ifndef __HAVE_ARCH_BCMP
-/**
- * bcmp - returns 0 if and only if the buffers have identical contents.
- * @a: pointer to first buffer.
- * @b: pointer to second buffer.
- * @len: size of buffers.
- *
- * The sign or magnitude of a non-zero return value has no particular
- * meaning, and architectures may implement their own more efficient bcmp(). So
- * while this particular implementation is a simple (tail) call to memcmp, do
- * not rely on anything but whether the return value is zero or non-zero.
- */
-#undef bcmp
-int bcmp(const void *a, const void *b, size_t len)
-{
-	return memcmp(a, b, len);
-}
-EXPORT_SYMBOL(bcmp);
-#endif
-
 #ifndef __HAVE_ARCH_MEMSCAN
 /**
  * memscan - Find a character in an area of memory.
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH v2 3/5] Revert "lib/string.c: implement a basic bcmp"

[Nathan Chancellor]

On Wed, Aug 19, 2020 at 12:16:52PM -0700, Nick Desaulniers wrote:
> This reverts commit 5f074f3e192f10c9fade898b9b3b8812e3d83342.
> 
> An earlier commit in the series prevents the compiler from emitting
> calls to bcmp as part of "libcall optimization," and there are no
> explicit callers, so we can now safely remove this interface.
> 
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

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

> ---
>  include/linux/string.h |  3 ---
>  lib/string.c           | 20 --------------------
>  2 files changed, 23 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b1f3894a0a3e..f3bdb74bc230 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -155,9 +155,6 @@ extern void * memscan(void *,int,__kernel_size_t);
>  #ifndef __HAVE_ARCH_MEMCMP
>  extern int memcmp(const void *,const void *,__kernel_size_t);
>  #endif
> -#ifndef __HAVE_ARCH_BCMP
> -extern int bcmp(const void *,const void *,__kernel_size_t);
> -#endif
>  #ifndef __HAVE_ARCH_MEMCHR
>  extern void * memchr(const void *,int,__kernel_size_t);
>  #endif
> diff --git a/lib/string.c b/lib/string.c
> index 6012c385fb31..69328b8353e1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -922,26 +922,6 @@ __visible int memcmp(const void *cs, const void *ct, size_t count)
>  EXPORT_SYMBOL(memcmp);
>  #endif
>  
> -#ifndef __HAVE_ARCH_BCMP
> -/**
> - * bcmp - returns 0 if and only if the buffers have identical contents.
> - * @a: pointer to first buffer.
> - * @b: pointer to second buffer.
> - * @len: size of buffers.
> - *
> - * The sign or magnitude of a non-zero return value has no particular
> - * meaning, and architectures may implement their own more efficient bcmp(). So
> - * while this particular implementation is a simple (tail) call to memcmp, do
> - * not rely on anything but whether the return value is zero or non-zero.
> - */
> -#undef bcmp
> -int bcmp(const void *a, const void *b, size_t len)
> -{
> -	return memcmp(a, b, len);
> -}
> -EXPORT_SYMBOL(bcmp);
> -#endif
> -
>  #ifndef __HAVE_ARCH_MEMSCAN
>  /**
>   * memscan - Find a character in an area of memory.
> -- 
> 2.28.0.297.g1956fa8f8d-goog
> 

[PATCH v2 4/5] x86/boot: use -fno-builtin-bcmp

[Nick Desaulniers]

We're reverting
commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
in favor of -fno-builtin-bcmp. Remove the additional definition here,
too.

arch/x86/purgatory/Makefile uses -ffreestanding, so there's no risk of
this libcall optimization occurring for arch/x86/boot/purgatory.ro.

arch/x86/boot/Makefile resets KBUILD_CFLAGS, so make sure to reset this
flag that was set for the top level Makefile.

Fixes: 4ce97317f41d ("x86/purgatory: Do not use __builtin_memcpy and __builtin_memset")
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/boot/Makefile | 1 +
 arch/x86/boot/string.c | 8 --------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index fe605205b4ce..ef7f15bfceab 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -70,6 +70,7 @@ KBUILD_CFLAGS	:= $(REALMODE_CFLAGS) -D_SETUP
 KBUILD_AFLAGS	:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
 KBUILD_CFLAGS	+= $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
 KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
+KBUILD_CFLAGS	+= -fno-builtin-bcmp
 GCOV_PROFILE := n
 UBSAN_SANITIZE := n
 
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 8a3fff9128bb..23d91aa7691e 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -37,14 +37,6 @@ int memcmp(const void *s1, const void *s2, size_t len)
 	return diff;
 }
 
-/*
- * Clang may lower `memcmp == 0` to `bcmp == 0`.
- */
-int bcmp(const void *s1, const void *s2, size_t len)
-{
-	return memcmp(s1, s2, len);
-}
-
 int strcmp(const char *str1, const char *str2)
 {
 	const unsigned char *s1 = (const unsigned char *)str1;
-- 
2.28.0.297.g1956fa8f8d-goog

Re: [PATCH v2 4/5] x86/boot: use -fno-builtin-bcmp

[Arvind Sankar]

On Wed, Aug 19, 2020 at 12:16:53PM -0700, Nick Desaulniers wrote:
> We're reverting
> commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
> in favor of -fno-builtin-bcmp. Remove the additional definition here,
> too.
> 
> arch/x86/purgatory/Makefile uses -ffreestanding, so there's no risk of
> this libcall optimization occurring for arch/x86/boot/purgatory.ro.
> 
> arch/x86/boot/Makefile resets KBUILD_CFLAGS, so make sure to reset this
> flag that was set for the top level Makefile.
> 
> Fixes: 4ce97317f41d ("x86/purgatory: Do not use __builtin_memcpy and __builtin_memset")
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  arch/x86/boot/Makefile | 1 +
>  arch/x86/boot/string.c | 8 --------
>  2 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> index fe605205b4ce..ef7f15bfceab 100644
> --- a/arch/x86/boot/Makefile
> +++ b/arch/x86/boot/Makefile
> @@ -70,6 +70,7 @@ KBUILD_CFLAGS	:= $(REALMODE_CFLAGS) -D_SETUP
>  KBUILD_AFLAGS	:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
>  KBUILD_CFLAGS	+= $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
>  KBUILD_CFLAGS	+= -fno-asynchronous-unwind-tables
> +KBUILD_CFLAGS	+= -fno-builtin-bcmp
>  GCOV_PROFILE := n
>  UBSAN_SANITIZE := n
>  

This should be unnecessary: KBUILD_CFLAGS in arch/x86/boot/Makefile is
set to REALMODE_CFLAGS (defined in arch/x86/Makefile), which includes
-ffreestanding.

Re: [PATCH v2 4/5] x86/boot: use -fno-builtin-bcmp

[Nick Desaulniers]

On Wed, Aug 19, 2020 at 12:29 PM Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Aug 19, 2020 at 12:16:53PM -0700, Nick Desaulniers wrote:
> > We're reverting
> > commit 5f074f3e192f ("lib/string.c: implement a basic bcmp")
> > in favor of -fno-builtin-bcmp. Remove the additional definition here,
> > too.
> >
> > arch/x86/purgatory/Makefile uses -ffreestanding, so there's no risk of
> > this libcall optimization occurring for arch/x86/boot/purgatory.ro.
> >
> > arch/x86/boot/Makefile resets KBUILD_CFLAGS, so make sure to reset this
> > flag that was set for the top level Makefile.
> >
> > Fixes: 4ce97317f41d ("x86/purgatory: Do not use __builtin_memcpy and __builtin_memset")
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  arch/x86/boot/Makefile | 1 +
> >  arch/x86/boot/string.c | 8 --------
> >  2 files changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
> > index fe605205b4ce..ef7f15bfceab 100644
> > --- a/arch/x86/boot/Makefile
> > +++ b/arch/x86/boot/Makefile
> > @@ -70,6 +70,7 @@ KBUILD_CFLAGS       := $(REALMODE_CFLAGS) -D_SETUP
> >  KBUILD_AFLAGS        := $(KBUILD_CFLAGS) -D__ASSEMBLY__
> >  KBUILD_CFLAGS        += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
> >  KBUILD_CFLAGS        += -fno-asynchronous-unwind-tables
> > +KBUILD_CFLAGS        += -fno-builtin-bcmp
> >  GCOV_PROFILE := n
> >  UBSAN_SANITIZE := n
> >
>
> This should be unnecessary: KBUILD_CFLAGS in arch/x86/boot/Makefile is
> set to REALMODE_CFLAGS (defined in arch/x86/Makefile), which includes
> -ffreestanding.

I triple checked by grepping the disassemblies of
./arch/x86/purgatory/purgatory.ro, arch/x86/purgatory/*.o, and
arch/x86/boot/*.o for bcmp; should be fine to drop that hunk.  Will
wait a bit to see if there's other feedback, and whether folks are on
board with the plan to merge the series proposed in the cover letter
or not.

--
Thanks,
~Nick Desaulniers

[PATCH v2 5/5] x86: don't build CONFIG_X86_32 as -ffreestanding

[Nick Desaulniers]

-ffreestanding typically inhibits "libcall optimizations" where calls to
certain library functions can be replaced by the compiler in certain
cases to calls to other library functions that may be more efficient.
This can be problematic for embedded targets that don't provide full
libc implementations.

-ffreestanding inhibits all optimizations, which
is the safe choice, but generally we want the optimizations that are
performed. The Linux kernel does implement a fair amount of libc
routines. Instead of -ffreestanding (which makes more sense in smaller
images like kexec's purgatory image), prefer -fno-builtin-* flags to
disable the compiler from emitting calls to functions which may not be
defined.

If you see a linkage failure due to a missing symbol that's typically
defined in a libc, and not explicitly called from the source code, then
the compiler may have done such a transform.  You can either implement
such a function (ie. in lib/string.c) or disable the transform outright
via -fno-builtin-* flag (where * is the name of the library routine, ie.
-fno-builtin-bcmp).

i386_defconfig build+boot tested with GCC and Clang. Removes a pretty
old TODO from the codebase.

Fixes: 6edfba1b33c7 ("x86_64: Don't define string functions to builtin")
Suggested-by: Arvind Sankar <nivedita@alum.mit.edu>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Might be nice to have
https://lore.kernel.org/lkml/CAKwvOdn-mv1D1GEk3pWnPYsyzQRRk5qZFhSi0CYn6tRDo1O_iw@mail.gmail.com/T/#u
first.

 arch/x86/Makefile | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 4346ffb2e39f..2383a96cf4fd 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -80,9 +80,6 @@ ifeq ($(CONFIG_X86_32),y)
         # CPU-specific tuning. Anything which can be shared with UML should go here.
         include arch/x86/Makefile_32.cpu
         KBUILD_CFLAGS += $(cflags-y)
-
-        # temporary until string.h is fixed
-        KBUILD_CFLAGS += -ffreestanding
 else
         BITS := 64
         UTS_MACHINE := x86_64
-- 
2.28.0.297.g1956fa8f8d-goog