linux-kbuild.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Clean up UBSAN Makefile
@ 2020-10-02 22:15 Kees Cook
  2020-10-02 22:15 ` [PATCH 1/4] ubsan: Move cc-option tests into Kconfig Kees Cook
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Kees Cook @ 2020-10-02 22:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, Marco Elver, Randy Dunlap, Dmitry Vyukov,
	George Popescu, Herbert Xu, Peter Oberparleiter, Andrey Ryabinin,
	clang-built-linux, linux-kbuild, linux-kernel

Hi,

This series attempts to address the issues seen with UBSAN's object-size
sanitizer causing problems under GCC. In the process, the Kconfig and
Makefile are refactored to do all the cc-option calls in the Kconfig.
Additionally start to detangle -Wno-maybe-uninitialized, and disable
UBSAN_TRAP under COMPILE_TEST for wider build coverage.

Thanks!

-Kees

Kees Cook (4):
  ubsan: Move cc-option tests into Kconfig
  ubsan: Disable object-size sanitizer under GCC
  ubsan: Force -Wno-maybe-uninitialized only for GCC
  ubsan: Disable UBSAN_TRAP for all*config

 lib/Kconfig.ubsan      | 58 +++++++++++++++++++++++++++++++++++++++++-
 scripts/Makefile.ubsan | 50 +++++++++++++-----------------------
 2 files changed, 74 insertions(+), 34 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH 1/4] ubsan: Move cc-option tests into Kconfig
  2020-10-02 22:15 [PATCH 0/4] Clean up UBSAN Makefile Kees Cook
@ 2020-10-02 22:15 ` Kees Cook
  2020-10-04  7:08   ` Nathan Chancellor
  2020-10-02 22:15 ` [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC Kees Cook
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-10-02 22:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, Marco Elver, Randy Dunlap, Dmitry Vyukov,
	George Popescu, Herbert Xu, Peter Oberparleiter, Andrey Ryabinin,
	clang-built-linux, linux-kbuild, linux-kernel

Instead of doing if/endif blocks with cc-option calls in the UBSAN
Makefile, move all the tests into Kconfig and use the Makefile to
collect the results.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.ubsan      | 48 +++++++++++++++++++++++++++++++++++++++-
 scripts/Makefile.ubsan | 50 ++++++++++++++----------------------------
 2 files changed, 64 insertions(+), 34 deletions(-)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 58f8d03d037b..c0b801871e0b 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -36,10 +36,17 @@ config UBSAN_KCOV_BROKEN
 	  See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
 	  in newer releases.
 
+config CC_HAS_UBSAN_BOUNDS
+	def_bool $(cc-option,-fsanitize=bounds)
+
+config CC_HAS_UBSAN_ARRAY_BOUNDS
+	def_bool $(cc-option,-fsanitize=array-bounds)
+
 config UBSAN_BOUNDS
 	bool "Perform array index bounds checking"
 	default UBSAN
 	depends on !UBSAN_KCOV_BROKEN
+	depends on CC_HAS_UBSAN_ARRAY_BOUNDS || CC_HAS_UBSAN_BOUNDS
 	help
 	  This option enables detection of directly indexed out of bounds
 	  array accesses, where the array size is known at compile time.
@@ -47,11 +54,17 @@ config UBSAN_BOUNDS
 	  to the {str,mem}*cpy() family of functions (that is addressed
 	  by CONFIG_FORTIFY_SOURCE).
 
+config CC_ARG_UBSAN_BOUNDS
+	string
+	default "-fsanitize=array-bounds" if CC_HAS_UBSAN_ARRAY_BOUNDS
+	default "-fsanitize=bounds"
+	depends on UBSAN_BOUNDS
+
 config UBSAN_LOCAL_BOUNDS
 	bool "Perform array local bounds checking"
 	depends on UBSAN_TRAP
-	depends on CC_IS_CLANG
 	depends on !UBSAN_KCOV_BROKEN
+	depends on $(cc-option,-fsanitize=local-bounds)
 	help
 	  This option enables -fsanitize=local-bounds which traps when an
 	  exception/error is detected. Therefore, it should be enabled only
@@ -69,6 +82,38 @@ config UBSAN_MISC
 	  own Kconfig options. Disable this if you only want to have
 	  individually selected checks.
 
+config UBSAN_SHIFT
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=shift)
+
+config UBSAN_DIV_ZERO
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=integer-divide-by-zero)
+
+config UBSAN_UNREACHABLE
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=unreachable)
+
+config UBSAN_SIGNED_OVERFLOW
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=signed-integer-overflow)
+
+config UBSAN_UNSIGNED_OVERFLOW
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
+
+config UBSAN_OBJECT_SIZE
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=object-size)
+
+config UBSAN_BOOL
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=bool)
+
+config UBSAN_ENUM
+	def_bool UBSAN_MISC
+	depends on $(cc-option,-fsanitize=enum)
+
 config UBSAN_SANITIZE_ALL
 	bool "Enable instrumentation for the entire kernel"
 	depends on ARCH_HAS_UBSAN_SANITIZE_ALL
@@ -89,6 +134,7 @@ config UBSAN_ALIGNMENT
 	bool "Enable checks for pointers alignment"
 	default !HAVE_EFFICIENT_UNALIGNED_ACCESS
 	depends on !UBSAN_TRAP
+	depends on $(cc-option,-fsanitize=alignment)
 	help
 	  This option enables the check of unaligned memory accesses.
 	  Enabling this option on architectures that support unaligned
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 9716dab06bc7..72862da47baf 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -1,37 +1,21 @@
 # SPDX-License-Identifier: GPL-2.0
 
-export CFLAGS_UBSAN :=
+# -fsanitize=* options makes GCC less smart than usual and
+# increases the number of 'maybe-uninitialized' false-positives.
+ubsan-cflags-$(CONFIG_UBSAN) += $(call cc-disable-warning, maybe-uninitialized)
 
-ifdef CONFIG_UBSAN_ALIGNMENT
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
-endif
+# Enable available and selected UBSAN features.
+ubsan-cflags-$(CONFIG_UBSAN_ALIGNMENT)		+= -fsanitize=alignment
+ubsan-cflags-$(CONFIG_UBSAN_BOUNDS)		+= $(CONFIG_CC_ARG_UBSAN_BOUNDS)
+ubsan-cflags-$(CONFIG_UBSAN_LOCAL_BOUNDS)	+= -fsanitize=local-bounds
+ubsan-cflags-$(CONFIG_UBSAN_SHIFT)		+= -fsanitize=shift
+ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO)		+= -fsanitize=integer-divide-by-zero
+ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE)	+= -fsanitize=unreachable
+ubsan-cflags-$(CONFIG_UBSAN_SIGNED_OVERFLOW)	+= -fsanitize=signed-integer-overflow
+ubsan-cflags-$(CONFIG_UBSAN_UNSIGNED_OVERFLOW)	+= -fsanitize=unsigned-integer-overflow
+ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE)	+= -fsanitize=object-size
+ubsan-cflags-$(CONFIG_UBSAN_BOOL)		+= -fsanitize=bool
+ubsan-cflags-$(CONFIG_UBSAN_ENUM)		+= -fsanitize=enum
+ubsan-cflags-$(CONFIG_UBSAN_TRAP)		+= -fsanitize-undefined-trap-on-error
 
-ifdef CONFIG_UBSAN_BOUNDS
-      ifdef CONFIG_CC_IS_CLANG
-            CFLAGS_UBSAN += -fsanitize=array-bounds
-      else
-            CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
-      endif
-endif
-
-ifdef CONFIG_UBSAN_LOCAL_BOUNDS
-      CFLAGS_UBSAN += -fsanitize=local-bounds
-endif
-
-ifdef CONFIG_UBSAN_MISC
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
-endif
-
-ifdef CONFIG_UBSAN_TRAP
-      CFLAGS_UBSAN += $(call cc-option, -fsanitize-undefined-trap-on-error)
-endif
-
-      # -fsanitize=* options makes GCC less smart than usual and
-      # increase number of 'maybe-uninitialized false-positives
-      CFLAGS_UBSAN += $(call cc-option, -Wno-maybe-uninitialized)
+export CFLAGS_UBSAN := $(ubsan-cflags-y)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC
  2020-10-02 22:15 [PATCH 0/4] Clean up UBSAN Makefile Kees Cook
  2020-10-02 22:15 ` [PATCH 1/4] ubsan: Move cc-option tests into Kconfig Kees Cook
@ 2020-10-02 22:15 ` Kees Cook
  2020-10-04  7:10   ` Nathan Chancellor
  2020-10-02 22:15 ` [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC Kees Cook
  2020-10-02 22:15 ` [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config Kees Cook
  3 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-10-02 22:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, Marco Elver, Randy Dunlap, Dmitry Vyukov,
	George Popescu, Herbert Xu, Peter Oberparleiter, Andrey Ryabinin,
	clang-built-linux, linux-kbuild, linux-kernel

GCC's -fsanitize=object-size (as part of CONFIG_UBSAN_MISC) greatly
increases stack utilization. Do not allow this under GCC.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.ubsan | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index c0b801871e0b..aeb2cdea0b94 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -104,6 +104,9 @@ config UBSAN_UNSIGNED_OVERFLOW
 
 config UBSAN_OBJECT_SIZE
 	def_bool UBSAN_MISC
+	# gcc hugely expands stack usage with -fsanitize=object-size
+	# https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
+	depends on !CC_IS_GCC
 	depends on $(cc-option,-fsanitize=object-size)
 
 config UBSAN_BOOL
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC
  2020-10-02 22:15 [PATCH 0/4] Clean up UBSAN Makefile Kees Cook
  2020-10-02 22:15 ` [PATCH 1/4] ubsan: Move cc-option tests into Kconfig Kees Cook
  2020-10-02 22:15 ` [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC Kees Cook
@ 2020-10-02 22:15 ` Kees Cook
  2020-10-04  7:16   ` Nathan Chancellor
  2020-10-02 22:15 ` [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config Kees Cook
  3 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-10-02 22:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, Marco Elver, Randy Dunlap, Dmitry Vyukov,
	George Popescu, Herbert Xu, Peter Oberparleiter, Andrey Ryabinin,
	clang-built-linux, linux-kbuild, linux-kernel

Clang handles 'maybe-uninitialized' better in the face of using UBSAN,
so do not make this universally disabled for UBSAN builds.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.ubsan      | 6 ++++++
 scripts/Makefile.ubsan | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index aeb2cdea0b94..1fc07f936e06 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -36,6 +36,12 @@ config UBSAN_KCOV_BROKEN
 	  See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
 	  in newer releases.
 
+config UBSAN_DISABLE_MAYBE_UNINITIALIZED
+	def_bool CC_IS_GCC
+	help
+	  -fsanitize=* options makes GCC less smart than usual and
+	  increases the number of 'maybe-uninitialized' false-positives.
+
 config CC_HAS_UBSAN_BOUNDS
 	def_bool $(cc-option,-fsanitize=bounds)
 
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 72862da47baf..c5ef6bac09d4 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -1,8 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
-# -fsanitize=* options makes GCC less smart than usual and
-# increases the number of 'maybe-uninitialized' false-positives.
-ubsan-cflags-$(CONFIG_UBSAN) += $(call cc-disable-warning, maybe-uninitialized)
+# The "maybe-uninitialized" warning can be very noisy.
+ubsan-cflags-$(CONFIG_UBSAN_DISABLE_MAYBE_UNINITIALIZED) += \
+						$(call cc-disable-warning, maybe-uninitialized)
 
 # Enable available and selected UBSAN features.
 ubsan-cflags-$(CONFIG_UBSAN_ALIGNMENT)		+= -fsanitize=alignment
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config
  2020-10-02 22:15 [PATCH 0/4] Clean up UBSAN Makefile Kees Cook
                   ` (2 preceding siblings ...)
  2020-10-02 22:15 ` [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC Kees Cook
@ 2020-10-02 22:15 ` Kees Cook
  2020-10-04  7:16   ` Nathan Chancellor
  3 siblings, 1 reply; 11+ messages in thread
From: Kees Cook @ 2020-10-02 22:15 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, Marco Elver, Randy Dunlap, Dmitry Vyukov,
	George Popescu, Herbert Xu, Peter Oberparleiter, Andrey Ryabinin,
	clang-built-linux, linux-kbuild, linux-kernel

Doing all*config builds attempts build as much as possible. UBSAN_TRAP
effectively short-circuits lib/usban.c, so it should be disabled for
COMPILE_TEST so that the lib/ubsan.c code gets built.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.ubsan | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 1fc07f936e06..b5b9da0b635a 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -14,6 +14,7 @@ if UBSAN
 
 config UBSAN_TRAP
 	bool "On Sanitizer warnings, abort the running kernel code"
+	depends on !COMPILE_TEST
 	depends on $(cc-option, -fsanitize-undefined-trap-on-error)
 	help
 	  Building kernels with Sanitizer features enabled tends to grow
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] ubsan: Move cc-option tests into Kconfig
  2020-10-02 22:15 ` [PATCH 1/4] ubsan: Move cc-option tests into Kconfig Kees Cook
@ 2020-10-04  7:08   ` Nathan Chancellor
  2020-10-06  6:01     ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Nathan Chancellor @ 2020-10-04  7:08 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Fri, Oct 02, 2020 at 03:15:24PM -0700, Kees Cook wrote:
> Instead of doing if/endif blocks with cc-option calls in the UBSAN
> Makefile, move all the tests into Kconfig and use the Makefile to
> collect the results.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/

I tested menuconfig to make sure all the flags when CONFIG_UBSAN_MISC is
flipped.

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

One comment below.

> ---
>  lib/Kconfig.ubsan      | 48 +++++++++++++++++++++++++++++++++++++++-
>  scripts/Makefile.ubsan | 50 ++++++++++++++----------------------------
>  2 files changed, 64 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 58f8d03d037b..c0b801871e0b 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -36,10 +36,17 @@ config UBSAN_KCOV_BROKEN
>  	  See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
>  	  in newer releases.
>  
> +config CC_HAS_UBSAN_BOUNDS
> +	def_bool $(cc-option,-fsanitize=bounds)
> +
> +config CC_HAS_UBSAN_ARRAY_BOUNDS
> +	def_bool $(cc-option,-fsanitize=array-bounds)
> +
>  config UBSAN_BOUNDS
>  	bool "Perform array index bounds checking"
>  	default UBSAN
>  	depends on !UBSAN_KCOV_BROKEN
> +	depends on CC_HAS_UBSAN_ARRAY_BOUNDS || CC_HAS_UBSAN_BOUNDS
>  	help
>  	  This option enables detection of directly indexed out of bounds
>  	  array accesses, where the array size is known at compile time.
> @@ -47,11 +54,17 @@ config UBSAN_BOUNDS
>  	  to the {str,mem}*cpy() family of functions (that is addressed
>  	  by CONFIG_FORTIFY_SOURCE).
>  
> +config CC_ARG_UBSAN_BOUNDS
> +	string
> +	default "-fsanitize=array-bounds" if CC_HAS_UBSAN_ARRAY_BOUNDS
> +	default "-fsanitize=bounds"
> +	depends on UBSAN_BOUNDS
> +
>  config UBSAN_LOCAL_BOUNDS
>  	bool "Perform array local bounds checking"
>  	depends on UBSAN_TRAP
> -	depends on CC_IS_CLANG
>  	depends on !UBSAN_KCOV_BROKEN
> +	depends on $(cc-option,-fsanitize=local-bounds)
>  	help
>  	  This option enables -fsanitize=local-bounds which traps when an
>  	  exception/error is detected. Therefore, it should be enabled only
> @@ -69,6 +82,38 @@ config UBSAN_MISC
>  	  own Kconfig options. Disable this if you only want to have
>  	  individually selected checks.
>  
> +config UBSAN_SHIFT
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=shift)
> +
> +config UBSAN_DIV_ZERO
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=integer-divide-by-zero)
> +
> +config UBSAN_UNREACHABLE
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=unreachable)
> +
> +config UBSAN_SIGNED_OVERFLOW
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=signed-integer-overflow)
> +
> +config UBSAN_UNSIGNED_OVERFLOW
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> +
> +config UBSAN_OBJECT_SIZE
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=object-size)
> +
> +config UBSAN_BOOL
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=bool)
> +
> +config UBSAN_ENUM
> +	def_bool UBSAN_MISC
> +	depends on $(cc-option,-fsanitize=enum)
> +
>  config UBSAN_SANITIZE_ALL
>  	bool "Enable instrumentation for the entire kernel"
>  	depends on ARCH_HAS_UBSAN_SANITIZE_ALL
> @@ -89,6 +134,7 @@ config UBSAN_ALIGNMENT
>  	bool "Enable checks for pointers alignment"
>  	default !HAVE_EFFICIENT_UNALIGNED_ACCESS
>  	depends on !UBSAN_TRAP
> +	depends on $(cc-option,-fsanitize=alignment)
>  	help
>  	  This option enables the check of unaligned memory accesses.
>  	  Enabling this option on architectures that support unaligned
> diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> index 9716dab06bc7..72862da47baf 100644
> --- a/scripts/Makefile.ubsan
> +++ b/scripts/Makefile.ubsan
> @@ -1,37 +1,21 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -export CFLAGS_UBSAN :=
> +# -fsanitize=* options makes GCC less smart than usual and
> +# increases the number of 'maybe-uninitialized' false-positives.
> +ubsan-cflags-$(CONFIG_UBSAN) += $(call cc-disable-warning, maybe-uninitialized)

Is this just to force -Wno-maybe-uninitialized even when W=2?
-Wmaybe-uninitialized is already disabled globally after
commit 78a5255ffb6a ("Stop the ad-hoc games with
-Wno-maybe-initialized"). I feel like it might be worth a comment in
case that changes in the future but maybe that is a bit much.

> -ifdef CONFIG_UBSAN_ALIGNMENT
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
> -endif
> +# Enable available and selected UBSAN features.
> +ubsan-cflags-$(CONFIG_UBSAN_ALIGNMENT)		+= -fsanitize=alignment
> +ubsan-cflags-$(CONFIG_UBSAN_BOUNDS)		+= $(CONFIG_CC_ARG_UBSAN_BOUNDS)
> +ubsan-cflags-$(CONFIG_UBSAN_LOCAL_BOUNDS)	+= -fsanitize=local-bounds
> +ubsan-cflags-$(CONFIG_UBSAN_SHIFT)		+= -fsanitize=shift
> +ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO)		+= -fsanitize=integer-divide-by-zero
> +ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE)	+= -fsanitize=unreachable
> +ubsan-cflags-$(CONFIG_UBSAN_SIGNED_OVERFLOW)	+= -fsanitize=signed-integer-overflow
> +ubsan-cflags-$(CONFIG_UBSAN_UNSIGNED_OVERFLOW)	+= -fsanitize=unsigned-integer-overflow
> +ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE)	+= -fsanitize=object-size
> +ubsan-cflags-$(CONFIG_UBSAN_BOOL)		+= -fsanitize=bool
> +ubsan-cflags-$(CONFIG_UBSAN_ENUM)		+= -fsanitize=enum
> +ubsan-cflags-$(CONFIG_UBSAN_TRAP)		+= -fsanitize-undefined-trap-on-error
>  
> -ifdef CONFIG_UBSAN_BOUNDS
> -      ifdef CONFIG_CC_IS_CLANG
> -            CFLAGS_UBSAN += -fsanitize=array-bounds
> -      else
> -            CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
> -      endif
> -endif
> -
> -ifdef CONFIG_UBSAN_LOCAL_BOUNDS
> -      CFLAGS_UBSAN += -fsanitize=local-bounds
> -endif
> -
> -ifdef CONFIG_UBSAN_MISC
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
> -endif
> -
> -ifdef CONFIG_UBSAN_TRAP
> -      CFLAGS_UBSAN += $(call cc-option, -fsanitize-undefined-trap-on-error)
> -endif
> -
> -      # -fsanitize=* options makes GCC less smart than usual and
> -      # increase number of 'maybe-uninitialized false-positives
> -      CFLAGS_UBSAN += $(call cc-option, -Wno-maybe-uninitialized)
> +export CFLAGS_UBSAN := $(ubsan-cflags-y)
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC
  2020-10-02 22:15 ` [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC Kees Cook
@ 2020-10-04  7:10   ` Nathan Chancellor
  0 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2020-10-04  7:10 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Fri, Oct 02, 2020 at 03:15:25PM -0700, Kees Cook wrote:
> GCC's -fsanitize=object-size (as part of CONFIG_UBSAN_MISC) greatly
> increases stack utilization. Do not allow this under GCC.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Link: https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/

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

> ---
>  lib/Kconfig.ubsan | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index c0b801871e0b..aeb2cdea0b94 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -104,6 +104,9 @@ config UBSAN_UNSIGNED_OVERFLOW
>  
>  config UBSAN_OBJECT_SIZE
>  	def_bool UBSAN_MISC
> +	# gcc hugely expands stack usage with -fsanitize=object-size
> +	# https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
> +	depends on !CC_IS_GCC
>  	depends on $(cc-option,-fsanitize=object-size)
>  
>  config UBSAN_BOOL
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC
  2020-10-02 22:15 ` [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC Kees Cook
@ 2020-10-04  7:16   ` Nathan Chancellor
  2020-10-06  6:03     ` Kees Cook
  0 siblings, 1 reply; 11+ messages in thread
From: Nathan Chancellor @ 2020-10-04  7:16 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Fri, Oct 02, 2020 at 03:15:26PM -0700, Kees Cook wrote:
> Clang handles 'maybe-uninitialized' better in the face of using UBSAN,
> so do not make this universally disabled for UBSAN builds.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Well this patch is not strictly necessary because Clang does not support
-Wmaybe-uninitialized anyways :) its flags are -Wuninitialized and
-Wsometimes-uninitialized so the warning stays enabled for UBSAN as it
stands.

However, something like this could still worthwhile because it would
save us one call to cc-disable-warning (yay micro optimizations).

Maybe it just does not need to have a whole new symbol, just make it

ubsan-cflags-$(CONFIG_CC_IS_GCC)

instead of

ubsan-cflags-$(CONFIG_UBSAN)

No strong opinions either way though.

> ---
>  lib/Kconfig.ubsan      | 6 ++++++
>  scripts/Makefile.ubsan | 6 +++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index aeb2cdea0b94..1fc07f936e06 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -36,6 +36,12 @@ config UBSAN_KCOV_BROKEN
>  	  See https://bugs.llvm.org/show_bug.cgi?id=45831 for the status
>  	  in newer releases.
>  
> +config UBSAN_DISABLE_MAYBE_UNINITIALIZED
> +	def_bool CC_IS_GCC
> +	help
> +	  -fsanitize=* options makes GCC less smart than usual and
> +	  increases the number of 'maybe-uninitialized' false-positives.
> +
>  config CC_HAS_UBSAN_BOUNDS
>  	def_bool $(cc-option,-fsanitize=bounds)
>  
> diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> index 72862da47baf..c5ef6bac09d4 100644
> --- a/scripts/Makefile.ubsan
> +++ b/scripts/Makefile.ubsan
> @@ -1,8 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
> -# -fsanitize=* options makes GCC less smart than usual and
> -# increases the number of 'maybe-uninitialized' false-positives.
> -ubsan-cflags-$(CONFIG_UBSAN) += $(call cc-disable-warning, maybe-uninitialized)
> +# The "maybe-uninitialized" warning can be very noisy.
> +ubsan-cflags-$(CONFIG_UBSAN_DISABLE_MAYBE_UNINITIALIZED) += \
> +						$(call cc-disable-warning, maybe-uninitialized)
>  
>  # Enable available and selected UBSAN features.
>  ubsan-cflags-$(CONFIG_UBSAN_ALIGNMENT)		+= -fsanitize=alignment
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config
  2020-10-02 22:15 ` [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config Kees Cook
@ 2020-10-04  7:16   ` Nathan Chancellor
  0 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2020-10-04  7:16 UTC (permalink / raw)
  To: Kees Cook
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Fri, Oct 02, 2020 at 03:15:27PM -0700, Kees Cook wrote:
> Doing all*config builds attempts build as much as possible. UBSAN_TRAP
> effectively short-circuits lib/usban.c, so it should be disabled for
> COMPILE_TEST so that the lib/ubsan.c code gets built.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

> ---
>  lib/Kconfig.ubsan | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 1fc07f936e06..b5b9da0b635a 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -14,6 +14,7 @@ if UBSAN
>  
>  config UBSAN_TRAP
>  	bool "On Sanitizer warnings, abort the running kernel code"
> +	depends on !COMPILE_TEST
>  	depends on $(cc-option, -fsanitize-undefined-trap-on-error)
>  	help
>  	  Building kernels with Sanitizer features enabled tends to grow
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] ubsan: Move cc-option tests into Kconfig
  2020-10-04  7:08   ` Nathan Chancellor
@ 2020-10-06  6:01     ` Kees Cook
  0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2020-10-06  6:01 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Sun, Oct 04, 2020 at 12:08:47AM -0700, Nathan Chancellor wrote:
> On Fri, Oct 02, 2020 at 03:15:24PM -0700, Kees Cook wrote:
> > Instead of doing if/endif blocks with cc-option calls in the UBSAN
> > Makefile, move all the tests into Kconfig and use the Makefile to
> > collect the results.
> > 
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Link: https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
> 
> I tested menuconfig to make sure all the flags when CONFIG_UBSAN_MISC is
> flipped.
> 
> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>

Awesome, thank you!

> One comment below.
> 
> > [...]
> > diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> > index 9716dab06bc7..72862da47baf 100644
> > --- a/scripts/Makefile.ubsan
> > +++ b/scripts/Makefile.ubsan
> > @@ -1,37 +1,21 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > -export CFLAGS_UBSAN :=
> > +# -fsanitize=* options makes GCC less smart than usual and
> > +# increases the number of 'maybe-uninitialized' false-positives.
> > +ubsan-cflags-$(CONFIG_UBSAN) += $(call cc-disable-warning, maybe-uninitialized)
> 
> Is this just to force -Wno-maybe-uninitialized even when W=2?
> -Wmaybe-uninitialized is already disabled globally after
> commit 78a5255ffb6a ("Stop the ad-hoc games with
> -Wno-maybe-initialized"). I feel like it might be worth a comment in
> case that changes in the future but maybe that is a bit much.
> 
> > [...]
> > -      # -fsanitize=* options makes GCC less smart than usual and
> > -      # increase number of 'maybe-uninitialized false-positives
> > -      CFLAGS_UBSAN += $(call cc-option, -Wno-maybe-uninitialized)

It's just a direct copying of the existing logic, but into the new
"ubsan-cflags-y" style. But yes, AFAICT, that was the intent when it was
added in commit a76bcf557ef4 ("Kbuild: enable -Wmaybe-uninitialized
warning for "make W=1"").

So for this patch, I kept the logic as it was.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC
  2020-10-04  7:16   ` Nathan Chancellor
@ 2020-10-06  6:03     ` Kees Cook
  0 siblings, 0 replies; 11+ messages in thread
From: Kees Cook @ 2020-10-06  6:03 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Andrew Morton, Linus Torvalds, Ard Biesheuvel, Arnd Bergmann,
	Masahiro Yamada, Michal Marek, Nick Desaulniers, Marco Elver,
	Randy Dunlap, Dmitry Vyukov, George Popescu, Herbert Xu,
	Peter Oberparleiter, Andrey Ryabinin, clang-built-linux,
	linux-kbuild, linux-kernel

On Sun, Oct 04, 2020 at 12:16:14AM -0700, Nathan Chancellor wrote:
> On Fri, Oct 02, 2020 at 03:15:26PM -0700, Kees Cook wrote:
> > Clang handles 'maybe-uninitialized' better in the face of using UBSAN,
> > so do not make this universally disabled for UBSAN builds.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Well this patch is not strictly necessary because Clang does not support
> -Wmaybe-uninitialized anyways :) its flags are -Wuninitialized and
> -Wsometimes-uninitialized so the warning stays enabled for UBSAN as it
> stands.

Ah, yes. Heh. Well... perhaps I can just drop this patch.

> However, something like this could still worthwhile because it would
> save us one call to cc-disable-warning (yay micro optimizations).
> 
> Maybe it just does not need to have a whole new symbol, just make it
> 
> ubsan-cflags-$(CONFIG_CC_IS_GCC)
> 
> instead of
> 
> ubsan-cflags-$(CONFIG_UBSAN)

If it gets kept, I'd still like it gated on CONFIG_UBSAN in some way
(e.g. the patch has an implicit depends due to the "if UBSAN" section).

But yes, this patch is rather a no-op.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-06  6:03 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-02 22:15 [PATCH 0/4] Clean up UBSAN Makefile Kees Cook
2020-10-02 22:15 ` [PATCH 1/4] ubsan: Move cc-option tests into Kconfig Kees Cook
2020-10-04  7:08   ` Nathan Chancellor
2020-10-06  6:01     ` Kees Cook
2020-10-02 22:15 ` [PATCH 2/4] ubsan: Disable object-size sanitizer under GCC Kees Cook
2020-10-04  7:10   ` Nathan Chancellor
2020-10-02 22:15 ` [PATCH 3/4] ubsan: Force -Wno-maybe-uninitialized only for GCC Kees Cook
2020-10-04  7:16   ` Nathan Chancellor
2020-10-06  6:03     ` Kees Cook
2020-10-02 22:15 ` [PATCH 4/4] ubsan: Disable UBSAN_TRAP for all*config Kees Cook
2020-10-04  7:16   ` Nathan Chancellor

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