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(-)
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,
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
On Mon, 5 Feb 2024 at 10:37, Kees Cook <[email protected]> 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 <[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?
> ---
> v3:
> - split out signed overflow sanitizer so we can do each separately
Thanks for splitting.
> 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(-)
>
> 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,
> 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
>
On Mon, Feb 05, 2024 at 12:29:21PM +0100, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 10:37, Kees Cook <[email protected]> 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 <[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]>
Thanks!
> 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).
--
Kees Cook
On 2/5/24 10:37, Kees Cook wrote:
> ---
> 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
>
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.
On Mon, Feb 05, 2024 at 01:54:24PM +0100, Andrey Ryabinin wrote:
>
>
> On 2/5/24 10:37, Kees Cook wrote:
>
> > ---
> > 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
> >
>
> Given this disables all kinds of code instrumentations,
> shouldn't we just add __no_sanitize_undefined here?
Yeah, that's a very good point.
> 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
--
Kees Cook
On Mon, 5 Feb 2024 at 13:59, Kees Cook <[email protected]> wrote:
>
> On Mon, Feb 05, 2024 at 01:54:24PM +0100, Andrey Ryabinin wrote:
> >
> >
> > On 2/5/24 10:37, Kees Cook wrote:
> >
> > > ---
> > > 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
> > >
> >
> > Given this disables all kinds of code instrumentations,
> > shouldn't we just add __no_sanitize_undefined here?
>
> Yeah, that's a very good point.
>
> > 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?
It already does, see CONFIG_NOINSTR_VALIDATION (yes by default on x86).
On Mon, Feb 05, 2024 at 02:10:26PM +0100, Marco Elver wrote:
> On Mon, 5 Feb 2024 at 13:59, Kees Cook <[email protected]> wrote:
> >
> > On Mon, Feb 05, 2024 at 01:54:24PM +0100, Andrey Ryabinin wrote:
> > >
> > >
> > > On 2/5/24 10:37, Kees Cook wrote:
> > >
> > > > ---
> > > > 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
> > > >
> > >
> > > Given this disables all kinds of code instrumentations,
> > > shouldn't we just add __no_sanitize_undefined here?
> >
> > Yeah, that's a very good point.
> >
> > > 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?
>
> 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....
--
Kees Cook
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 <[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(-)
>
> 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 <[email protected]>
> 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
On Wed, Feb 07, 2024 at 01:45:28AM +0000, Justin Stitt wrote:
> 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.
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...
>
> 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
--
Kees Cook