| Message ID | 20201002221527.177500-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up UBSAN Makefile | expand |
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
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.
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)
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(-)