On Fri, 2 Feb 2024 at 11:16, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> Effectively revert commit 6aaa31aeb9cf ("ubsan: remove overflow
> checks"), to allow the kernel to be built with the "overflow"
> sanitizers again. This gives developers a chance to experiment[1][2][3]
> with the instrumentation again, while compilers adjust their sanitizers
> to deal with the impact of -fno-strict-oveflow (i.e. moving from
> "overflow" checking to "wrap-around" checking).
>
> Notably, the naming of the options is adjusted to use the name "WRAP"
> instead of "OVERFLOW". In the strictest sense, arithmetic "overflow"
> happens when a result exceeds the storage of the type, and is considered
> by the C standard and compilers to be undefined behavior for signed
> and pointer types (without -fno-strict-overflow). Unsigned arithmetic
> overflow is defined as always wrapping around.
>
> Because the kernel is built with -fno-strict-overflow, signed and pointer
> arithmetic is defined to always wrap around instead of "overflowing"
> (which could either be elided due to being undefined behavior or would
> wrap around, which led to very weird bugs in the kernel).
>
> So, the config options are added back as CONFIG_UBSAN_SIGNED_WRAP and
> CONFIG_UBSAN_UNSIGNED_WRAP. Since the kernel has several places that
> explicitly depend on wrap-around behavior (e.g. counters, atomics, crypto,
> etc), also introduce the __signed_wrap and __unsigned_wrap function
> attributes for annotating functions where wrapping is expected and should
> not be instrumented. This will allow us to distinguish in the kernel
> between intentional and unintentional cases of arithmetic wrap-around.
>
> Additionally keep these disabled under CONFIG_COMPILE_TEST for now.
>
> Link: https://github.com/KSPP/linux/issues/26 [1]
> Link: https://github.com/KSPP/linux/issues/27 [2]
> Link: https://github.com/KSPP/linux/issues/344 [3]
> Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> Cc: Miguel Ojeda <ojeda@xxxxxxxxxx>
> Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> Cc: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Marco Elver <elver@xxxxxxxxxx>
> Cc: Hao Luo <haoluo@xxxxxxxxxx>
> Cc: Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> include/linux/compiler_types.h | 14 ++++++-
> lib/Kconfig.ubsan | 19 ++++++++++
> lib/test_ubsan.c | 49 ++++++++++++++++++++++++
> lib/ubsan.c | 68 ++++++++++++++++++++++++++++++++++
> lib/ubsan.h | 4 ++
> scripts/Makefile.ubsan | 2 +
> 6 files changed, 155 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6f1ca49306d2..e585614f3152 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -282,11 +282,23 @@ struct ftrace_likely_data {
> #define __no_sanitize_or_inline __always_inline
> #endif
>
> +/* Allow wrapping arithmetic within an annotated function. */
> +#ifdef CONFIG_UBSAN_SIGNED_WRAP
> +# define __signed_wrap __attribute__((no_sanitize("signed-integer-overflow")))
> +#else
> +# define __signed_wrap
> +#endif
> +#ifdef CONFIG_UBSAN_UNSIGNED_WRAP
> +# define __unsigned_wrap __attribute__((no_sanitize("unsigned-integer-overflow")))
> +#else
> +# define __unsigned_wrap
> +#endif
> +
> /* Section for code which can't be instrumented at all */
> #define __noinstr_section(section) \
> noinline notrace __attribute((__section__(section))) \
> __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
> - __no_sanitize_memory
> + __no_sanitize_memory __signed_wrap __unsigned_wrap
>
> #define noinstr __noinstr_section(".noinstr.text")
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 59e21bfec188..a7003e5bd2a1 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -116,6 +116,25 @@ config UBSAN_UNREACHABLE
> This option enables -fsanitize=unreachable which checks for control
> flow reaching an expected-to-be-unreachable position.
>
> +config UBSAN_SIGNED_WRAP
> + bool "Perform checking for signed arithmetic wrap-around"
> + default UBSAN
> + depends on !COMPILE_TEST
> + depends on $(cc-option,-fsanitize=signed-integer-overflow)
> + help
> + This option enables -fsanitize=signed-integer-overflow which checks
> + for wrap-around of any arithmetic operations with signed integers.
> +
> +config UBSAN_UNSIGNED_WRAP
> + bool "Perform checking for unsigned arithmetic wrap-around"
> + depends on $(cc-option,-fsanitize=unsigned-integer-overflow)
> + depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> + depends on !COMPILE_TEST
> + help
> + This option enables -fsanitize=unsigned-integer-overflow which checks
> + for wrap-around of any arithmetic operations with unsigned integers. This
> + currently causes x86 to fail to boot.
My hypothesis is that these options will quickly be enabled by various
test and fuzzing setups, to the detriment of kernel developers. While
the commit message states that these are for experimentation, I do not
think it is at all clear from the Kconfig options.
Unsigned integer wrap-around is relatively common (it is _not_ UB
after all). While I can appreciate that in some cases wrap around is a
genuine semantic bug, and that's what we want to find with these
changes, ultimately marking all semantically valid wrap arounds to
catch the unmarked ones. Given these patterns are so common, and C
programmers are used to them, it will take a lot of effort to mark all
the intentional cases. But I fear that even if we get to that place,
_unmarked_ but semantically valid unsigned wrap around will keep
popping up again and again.
What is the long-term vision to minimize the additional churn this may
introduce?
I think the problem reminds me a little of the data race problem,
although I suspect unsigned integer wraparound is much more common
than data races (which unlike unsigned wrap around is actually UB) -
so chasing all intentional unsigned integer wrap arounds and marking
will take even more effort than marking all intentional data races
(which we're still slowly, but steadily, making progress towards).
At the very least, these options should 'depends on EXPERT' or even
'depends on BROKEN' while the story is still being worked out.
Thanks,
-- Marco
