Hi,
On Mon, Feb 05, 2024 at 01:37:29AM -0800, Kees Cook wrote:
> In order to mitigate unexpected signed wrap-around[1], bring back the
> signed integer overflow sanitizer. It was removed in commit 6aaa31aeb9cf
> ("ubsan: remove overflow checks") because it was effectively a no-op
> when combined with -fno-strict-overflow (which correctly changes signed
> overflow from being "undefined" to being explicitly "wrap around").
>
> Compilers are adjusting their sanitizers to trap wrap-around and to
> detecting common code patterns that should not be instrumented
> (e.g. "var + offset < var"). Prepare for this and explicitly rename
> the option from "OVERFLOW" to "WRAP".
>
> To annotate intentional wrap-around arithmetic, the add/sub/mul_wrap()
> helpers can be used for individual statements. At the function level,
> the __signed_wrap attribute can be used to mark an entire function as
> expecting its signed arithmetic to wrap around. For a single object file
> the Makefile can use "UBSAN_WRAP_SIGNED_target.o := n" to mark it as
> wrapping, and for an entire directory, "UBSAN_WRAP_SIGNED := n" can be
> used.
>
> Additionally keep these disabled under CONFIG_COMPILE_TEST for now.
>
> Link: https://github.com/KSPP/linux/issues/26 [1]
> Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> Cc: Marco Elver <elver@xxxxxxxxxx>
> Cc: Miguel Ojeda <ojeda@xxxxxxxxxx>
> Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Hao Luo <haoluo@xxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> v3:
> - split out signed overflow sanitizer so we can do each separately
> v2: https://lore.kernel.org/all/20240202101311.it.893-kees@xxxxxxxxxx/
> v1: https://lore.kernel.org/all/20240129175033.work.813-kees@xxxxxxxxxx/
> ---
> include/linux/compiler_types.h | 9 ++++-
> lib/Kconfig.ubsan | 14 +++++++
> lib/test_ubsan.c | 37 ++++++++++++++++++
> lib/ubsan.c | 68 ++++++++++++++++++++++++++++++++++
> lib/ubsan.h | 4 ++
> scripts/Makefile.lib | 3 ++
> scripts/Makefile.ubsan | 3 ++
> 7 files changed, 137 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 6f1ca49306d2..ee9d272008a5 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -282,11 +282,18 @@ struct ftrace_likely_data {
> #define __no_sanitize_or_inline __always_inline
> #endif
>
> +/* Do not trap 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
> +
> /* 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
>
> #define noinstr __noinstr_section(".noinstr.text")
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 56d7653f4941..129e9bc21877 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -116,6 +116,20 @@ 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.
> + This currently performs nearly no instrumentation due to the
> + kernel's use of -fno-strict-overflow which converts all would-be
> + arithmetic undefined behavior into wrap-around arithmetic. Future
> + sanitizer versions will allow for wrap-around checking (rather than
> + exclusively undefined behavior).
> +
> config UBSAN_BOOL
> bool "Perform checking for non-boolean values used as boolean"
> default UBSAN
> diff --git a/lib/test_ubsan.c b/lib/test_ubsan.c
> index f4ee2484d4b5..276c12140ee2 100644
> --- a/lib/test_ubsan.c
> +++ b/lib/test_ubsan.c
> @@ -11,6 +11,39 @@ typedef void(*test_ubsan_fp)(void);
> #config, IS_ENABLED(config) ? "y" : "n"); \
> } while (0)
>
> +static void test_ubsan_add_overflow(void)
> +{
> + volatile int val = INT_MAX;
> +
> + UBSAN_TEST(CONFIG_UBSAN_SIGNED_WRAP);
> + val += 2;
> +}
> +
> +static void test_ubsan_sub_overflow(void)
> +{
> + volatile int val = INT_MIN;
> + volatile int val2 = 2;
> +
> + UBSAN_TEST(CONFIG_UBSAN_SIGNED_WRAP);
> + val -= val2;
> +}
> +
> +static void test_ubsan_mul_overflow(void)
> +{
> + volatile int val = INT_MAX / 2;
> +
> + UBSAN_TEST(CONFIG_UBSAN_SIGNED_WRAP);
> + val *= 3;
> +}
> +
> +static void test_ubsan_negate_overflow(void)
> +{
> + volatile int val = INT_MIN;
> +
> + UBSAN_TEST(CONFIG_UBSAN_SIGNED_WRAP);
> + val = -val;
> +}
> +
> static void test_ubsan_divrem_overflow(void)
> {
> volatile int val = 16;
> @@ -90,6 +123,10 @@ static void test_ubsan_misaligned_access(void)
> }
>
> static const test_ubsan_fp test_ubsan_array[] = {
> + test_ubsan_add_overflow,
> + test_ubsan_sub_overflow,
> + test_ubsan_mul_overflow,
> + test_ubsan_negate_overflow,
I wouldn't mind also seeing a test_ubsan_div_overflow test case here.
It has some quirky behavior and it'd be nice to test that the sanitizers
properly capture it.
Check out this Godbolt: https://godbolt.org/z/qG5f1j6n1
tl;dr: with -fsanitize=signed-integer-overflow division (/) and
remainder (%) operators still instrument arithmetic even with
-fno-strict-overflow on.
This makes sense as division by 0 and INT_MIN/-1 are UBs that are not
influenced by -fno-strict-overflow.
Really though, the patch is fine and the above test case is optional and
can be shipped later -- as such:
Reviewed-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> test_ubsan_shift_out_of_bounds,
> test_ubsan_out_of_bounds,
> test_ubsan_load_invalid_value,
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index df4f8d1354bb..5fc107f61934 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -222,6 +222,74 @@ static void ubsan_epilogue(void)
> check_panic_on_warn("UBSAN");
> }
>
> +static void handle_overflow(struct overflow_data *data, void *lhs,
> + void *rhs, char op)
> +{
> +
> + struct type_descriptor *type = data->type;
> + char lhs_val_str[VALUE_LENGTH];
> + char rhs_val_str[VALUE_LENGTH];
> +
> + if (suppress_report(&data->location))
> + return;
> +
> + ubsan_prologue(&data->location, type_is_signed(type) ?
> + "signed-integer-overflow" :
> + "unsigned-integer-overflow");
> +
> + val_to_string(lhs_val_str, sizeof(lhs_val_str), type, lhs);
> + val_to_string(rhs_val_str, sizeof(rhs_val_str), type, rhs);
> + pr_err("%s %c %s cannot be represented in type %s\n",
> + lhs_val_str,
> + op,
> + rhs_val_str,
> + type->type_name);
> +
> + ubsan_epilogue();
> +}
> +
> +void __ubsan_handle_add_overflow(void *data,
> + void *lhs, void *rhs)
> +{
> +
> + handle_overflow(data, lhs, rhs, '+');
> +}
> +EXPORT_SYMBOL(__ubsan_handle_add_overflow);
> +
> +void __ubsan_handle_sub_overflow(void *data,
> + void *lhs, void *rhs)
> +{
> + handle_overflow(data, lhs, rhs, '-');
> +}
> +EXPORT_SYMBOL(__ubsan_handle_sub_overflow);
> +
> +void __ubsan_handle_mul_overflow(void *data,
> + void *lhs, void *rhs)
> +{
> + handle_overflow(data, lhs, rhs, '*');
> +}
> +EXPORT_SYMBOL(__ubsan_handle_mul_overflow);
> +
> +void __ubsan_handle_negate_overflow(void *_data, void *old_val)
> +{
> + struct overflow_data *data = _data;
> + char old_val_str[VALUE_LENGTH];
> +
> + if (suppress_report(&data->location))
> + return;
> +
> + ubsan_prologue(&data->location, "negation-overflow");
> +
> + val_to_string(old_val_str, sizeof(old_val_str), data->type, old_val);
> +
> + pr_err("negation of %s cannot be represented in type %s:\n",
> + old_val_str, data->type->type_name);
> +
> + ubsan_epilogue();
> +}
> +EXPORT_SYMBOL(__ubsan_handle_negate_overflow);
> +
> +
> void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs)
> {
> struct overflow_data *data = _data;
> diff --git a/lib/ubsan.h b/lib/ubsan.h
> index 5d99ab81913b..0abbbac8700d 100644
> --- a/lib/ubsan.h
> +++ b/lib/ubsan.h
> @@ -124,6 +124,10 @@ typedef s64 s_max;
> typedef u64 u_max;
> #endif
>
> +void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
> +void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
> +void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
> +void __ubsan_handle_negate_overflow(void *_data, void *old_val);
> void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
> void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
> void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 52efc520ae4f..7ce8ecccc65a 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -177,6 +177,9 @@ ifeq ($(CONFIG_UBSAN),y)
> _c_flags += $(if $(patsubst n%,, \
> $(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_SANITIZE)y), \
> $(CFLAGS_UBSAN))
> +_c_flags += $(if $(patsubst n%,, \
> + $(UBSAN_WRAP_SIGNED_$(basetarget).o)$(UBSAN_SANITIZE_$(basetarget).o)$(UBSAN_WRAP_SIGNED)$(UBSAN_SANITIZE)y), \
> + $(CFLAGS_UBSAN_WRAP_SIGNED))
> endif
>
> ifeq ($(CONFIG_KCOV),y)
> diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> index 7cf42231042b..bc957add0b4d 100644
> --- a/scripts/Makefile.ubsan
> +++ b/scripts/Makefile.ubsan
> @@ -13,3 +13,6 @@ ubsan-cflags-$(CONFIG_UBSAN_ENUM) += -fsanitize=enum
> ubsan-cflags-$(CONFIG_UBSAN_TRAP) += $(call cc-option,-fsanitize-trap=undefined,-fsanitize-undefined-trap-on-error)
>
> export CFLAGS_UBSAN := $(ubsan-cflags-y)
> +
> +ubsan-wrap-signed-cflags-$(CONFIG_UBSAN_SIGNED_WRAP) += -fsanitize=signed-integer-overflow
> +export CFLAGS_UBSAN_WRAP_SIGNED := $(ubsan-wrap-signed-cflags-y)
> --
> 2.34.1
>
Thanks
Justin
