Skip to contentSkip to conversation

ubsan: Reintroduce signed overflow sanitizer

PATCH Applied Kees Cook 4 participants 9 messages Feb 5, 2024 – Feb 7, 2024

Subsystems (3)
Kbuild Library code Ubsan
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH v3 1/1 ubsan: Reintroduce signed overflow sanitizer

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 <[email protected]>
Cc Marco Elver <[email protected]>
Cc Miguel Ojeda <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Hao Luo <[email protected]>
Signed-off-by Kees Cook <[email protected]>

---
v3:
- split out signed overflow sanitizer so we can do each separately
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

---
 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(-)
include/linux/compiler_types.h +8 -1
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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")
lib/Kconfig.ubsan +14 -0
index 56d7653f4941..129e9bc21877 100644
@@ -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
lib/test_ubsan.c +37 -0
index f4ee2484d4b5..276c12140ee2 100644
@@ -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,
 	test_ubsan_shift_out_of_bounds,
 	test_ubsan_out_of_bounds,
 	test_ubsan_load_invalid_value,
lib/ubsan.c +68 -0

74 lines not on this page · raw message

lib/ubsan.h +4 -0
index 5d99ab81913b..0abbbac8700d 100644
@@ -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);
scripts/Makefile.lib +3 -0
index 52efc520ae4f..7ce8ecccc65a 100644
@@ -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)
scripts/Makefile.ubsan +3 -0
index 7cf42231042b..bc957add0b4d 100644
@@ -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)
Signature · 1 line
2.34.1
reviewer Reviewed-by on v3 1/1
#1 lore raw
Quoted 22 lines · Kees Cook

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.

Quoted 8 lines
Link https://github.com/KSPP/linux/issues/26 [1]
Cc Justin Stitt <[email protected]>
Cc Marco Elver <[email protected]>
Cc Miguel Ojeda <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Hao Luo <[email protected]>
Signed-off-by Kees Cook <[email protected]>

Looks good.

Reviewed-by Marco Elver <[email protected]>

And just to double check, you don't think we need 'depends on EXPERT' (or DEBUG_KERNEL) to keep the noise down initially?

Quoted 3 lines

---
v3:
- split out signed overflow sanitizer so we can do each separately

Thanks for splitting.

Quoted 238 lines

v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

---
 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(-)
include/linux/compiler_types.h +8 -1
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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")
lib/Kconfig.ubsan +14 -0
index 56d7653f4941..129e9bc21877 100644
@@ -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
lib/test_ubsan.c +37 -0
index f4ee2484d4b5..276c12140ee2 100644
@@ -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,
        test_ubsan_shift_out_of_bounds,
        test_ubsan_out_of_bounds,
        test_ubsan_load_invalid_value,
lib/ubsan.c +68 -0
index df4f8d1354bb..5fc107f61934 100644
@@ -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;
lib/ubsan.h +4 -0
index 5d99ab81913b..0abbbac8700d 100644
@@ -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);
scripts/Makefile.lib +3 -0
index 52efc520ae4f..7ce8ecccc65a 100644
@@ -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)
scripts/Makefile.ubsan +3 -1
index 7cf42231042b..bc957add0b4d 100644
@@ -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

author
#2 lore raw
Quoted 32 lines · Marco Elver
Quoted 22 lines · Kees Cook

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.

Quoted 8 lines
Link https://github.com/KSPP/linux/issues/26 [1]
Cc Justin Stitt <[email protected]>
Cc Marco Elver <[email protected]>
Cc Miguel Ojeda <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Hao Luo <[email protected]>
Signed-off-by Kees Cook <[email protected]>
Quoted 3 lines

Looks good.

Reviewed-by Marco Elver <[email protected]>

Thanks!

Quoted 2 lines

And just to double check, you don't think we need 'depends on EXPERT' (or DEBUG_KERNEL) to keep the noise down initially?

Not for signed, no. It's almost a no-op like this. Once Clang and GCC support the wrap version (which will likely require changing the command line argument), we can re-evaluate. So far in my testing, I've not been able to trip it. I'm planning to get a local syzbot running with the wrap sanitizer later this week to see how noisy it gets (if at all).

Signature · 1 line
Kees Cook
reviewer
#3 lore raw
In reply to Kees Cook
Quoted 22 lines · Kees Cook
---
 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(-)
include/linux/compiler_types.h +4 -0
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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

include/linux/compiler_types.h struct ftrace_likely_data { · line 297

Quoted 10 lines
include/linux/compiler_types.h +4 -1
+# 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

Given this disables all kinds of code instrumentations, shouldn't we just add __no_sanitize_undefined here?

I suspect that ubsan's instrumentation usually doesn't cause problems because it calls __ubsan_* functions with all heavy stuff (printk, locks etc) only if code has an UB. So the answer to the question above depends on whether we want to ignore UBs in "noinstr" code or to get some weird side effect, possibly without proper UBSAN report in dmesg.

author
#4 lore raw
Quoted 31 lines · Andrey Ryabinin
Quoted 27 lines · Kees Cook
---
 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(-)
include/linux/compiler_types.h +7 -0
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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)					\
Quoted 8 lines

include/linux/compiler_types.h struct ftrace_likely_data { · line 297

Quoted 5 lines
include/linux/compiler_types.h +1 -1
 	noinline notrace __attribute((__section__(section)))		\
 	__no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
-	__no_sanitize_memory
+	__no_sanitize_memory __signed_wrap

Given this disables all kinds of code instrumentations, shouldn't we just add __no_sanitize_undefined here?

Yeah, that's a very good point.

Quoted 5 lines

I suspect that ubsan's instrumentation usually doesn't cause problems because it calls __ubsan_* functions with all heavy stuff (printk, locks etc) only if code has an UB. So the answer to the question above depends on whether we want to ignore UBs in "noinstr" code or to get some weird side effect, possibly without proper UBSAN report in dmesg.

I think my preference would be to fail safe (i.e. leave in the instrumentation), but the intent of noinstr is pretty clear. :P I wonder if, instead, we could adjust objtool to yell about cases where calls are made in noinstr functions (like it does for UACCESS)... maybe it already does?

-Kees

Signature · 1 line
Kees Cook
reviewer
#5 lore raw
Quoted 50 lines · Kees Cook
Quoted 31 lines · Andrey Ryabinin
Quoted 27 lines · Kees Cook
---
 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(-)
include/linux/compiler_types.h +7 -0
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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)                                 \
Quoted 8 lines

include/linux/compiler_types.h struct ftrace_likely_data { · line 297

Quoted 5 lines
include/linux/compiler_types.h +1 -1
    noinline notrace __attribute((__section__(section)))            \
    __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
-   __no_sanitize_memory
+   __no_sanitize_memory __signed_wrap

Given this disables all kinds of code instrumentations, shouldn't we just add __no_sanitize_undefined here?

Yeah, that's a very good point.

Quoted 5 lines

I suspect that ubsan's instrumentation usually doesn't cause problems because it calls __ubsan_* functions with all heavy stuff (printk, locks etc) only if code has an UB. So the answer to the question above depends on whether we want to ignore UBs in "noinstr" code or to get some weird side effect, possibly without proper UBSAN report in dmesg.

Quoted 5 lines

I think my preference would be to fail safe (i.e. leave in the instrumentation), but the intent of noinstr is pretty clear. :P I wonder if, instead, we could adjust objtool to yell about cases where calls are made in noinstr functions (like it does for UACCESS)... maybe it already does?

It already does, see CONFIG_NOINSTR_VALIDATION (yes by default on x86).

author
#6 lore raw
Quoted 51 lines · Marco Elver
Quoted 50 lines · Kees Cook
Quoted 31 lines · Andrey Ryabinin
Quoted 27 lines · Kees Cook
---
 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(-)
include/linux/compiler_types.h +7 -0
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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)                                 \
Quoted 8 lines

include/linux/compiler_types.h struct ftrace_likely_data { · line 297

Quoted 5 lines
include/linux/compiler_types.h +1 -1
    noinline notrace __attribute((__section__(section)))            \
    __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage \
-   __no_sanitize_memory
+   __no_sanitize_memory __signed_wrap

Given this disables all kinds of code instrumentations, shouldn't we just add __no_sanitize_undefined here?

Yeah, that's a very good point.

Quoted 5 lines

I suspect that ubsan's instrumentation usually doesn't cause problems because it calls __ubsan_* functions with all heavy stuff (printk, locks etc) only if code has an UB. So the answer to the question above depends on whether we want to ignore UBs in "noinstr" code or to get some weird side effect, possibly without proper UBSAN report in dmesg.

Quoted 7 lines
Quoted 5 lines

I think my preference would be to fail safe (i.e. leave in the instrumentation), but the intent of noinstr is pretty clear. :P I wonder if, instead, we could adjust objtool to yell about cases where calls are made in noinstr functions (like it does for UACCESS)... maybe it already does?

It already does, see CONFIG_NOINSTR_VALIDATION (yes by default on x86).

This is actually a reason to not include the ubsan disabling in __noinstr_section just to see what ends up in there so we can fix it immediately....

Signature · 1 line
Kees Cook
Reviewed-by on v3 1/1
#7 lore raw
In reply to Kees Cook

Hi,

Quoted 138 lines · Kees Cook

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 <[email protected]>
Cc Marco Elver <[email protected]>
Cc Miguel Ojeda <[email protected]>
Cc Nathan Chancellor <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Hao Luo <[email protected]>
Signed-off-by Kees Cook <[email protected]>

---
v3:
- split out signed overflow sanitizer so we can do each separately
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

---
 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(-)
include/linux/compiler_types.h +8 -1
index 6f1ca49306d2..ee9d272008a5 100644
@@ -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")
lib/Kconfig.ubsan +14 -0
index 56d7653f4941..129e9bc21877 100644
@@ -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
lib/test_ubsan.c +33 -0
index f4ee2484d4b5..276c12140ee2 100644
@@ -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;

lib/test_ubsan.c static void test_ubsan_misaligned_access(void) · line 129

Quoted 7 lines
lib/test_ubsan.c +4 -0
@@ -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 <[email protected]>
Quoted 123 lines
lib/test_ubsan.c +0 -0
 	test_ubsan_shift_out_of_bounds,
 	test_ubsan_out_of_bounds,
 	test_ubsan_load_invalid_value,
lib/ubsan.c +68 -0
index df4f8d1354bb..5fc107f61934 100644
@@ -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;
lib/ubsan.h +4 -0
index 5d99ab81913b..0abbbac8700d 100644
@@ -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);
scripts/Makefile.lib +3 -0
index 52efc520ae4f..7ce8ecccc65a 100644
@@ -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)
scripts/Makefile.ubsan +3 -1
index 7cf42231042b..bc957add0b4d 100644
@@ -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)
--
Quoted 2 lines

2.34.1

Thanks
Justin

author
#8 lore raw
Quoted 5 lines · Justin Stitt

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.

Quoted 8 lines

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.

There is actually already a test_ubsan_divrem_overflow, but because the failure modes result in a trap even without the sanitizer, it's disabled in the test. For testing a crashing mode, it might be interesting to add it to LKDTM, which is the crash tester...

Quoted 5 lines

Really though, the patch is fine and the above test case is optional and can be shipped later -- as such:

Reviewed-by Justin Stitt <[email protected]>

Thanks!

-Kees

Signature · 1 line
Kees Cook