2021-12-03 23:53:52

by Kees Cook

[permalink] [raw]
Subject: [PATCH] ubsan: Remove CONFIG_UBSAN_OBJECT_SIZE

The object-size sanitizer is redundant to -Warray-bounds, and
inappropriately performs its checks at run-time when all information
needed for the evaluation is available at compile-time, making it quite
difficult to use:

https://bugzilla.kernel.org/show_bug.cgi?id=214861

With -Warray-bounds almost enabled globally, it doesn't make sense to
keep this around.

Signed-off-by: Kees Cook <[email protected]>
---
lib/Kconfig.ubsan | 13 -------------
lib/test_ubsan.c | 22 ----------------------
scripts/Makefile.ubsan | 1 -
3 files changed, 36 deletions(-)

diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index e5372a13511d..236c5cefc4cc 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -112,19 +112,6 @@ config UBSAN_UNREACHABLE
This option enables -fsanitize=unreachable which checks for control
flow reaching an expected-to-be-unreachable position.

-config UBSAN_OBJECT_SIZE
- bool "Perform checking for accesses beyond the end of objects"
- default UBSAN
- # gcc hugely expands stack usage with -fsanitize=object-size
- # https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
- depends on !CC_IS_GCC
- depends on $(cc-option,-fsanitize=object-size)
- help
- This option enables -fsanitize=object-size which checks for accesses
- beyond the end of objects where the optimizer can determine both the
- object being operated on and its size, usually seen with bad downcasts,
- or access to struct members from NULL pointers.
-
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 7e7bbd0f3fd2..2062be1f2e80 100644
--- a/lib/test_ubsan.c
+++ b/lib/test_ubsan.c
@@ -79,15 +79,6 @@ static void test_ubsan_load_invalid_value(void)
eval2 = eval;
}

-static void test_ubsan_null_ptr_deref(void)
-{
- volatile int *ptr = NULL;
- int val;
-
- UBSAN_TEST(CONFIG_UBSAN_OBJECT_SIZE);
- val = *ptr;
-}
-
static void test_ubsan_misaligned_access(void)
{
volatile char arr[5] __aligned(4) = {1, 2, 3, 4, 5};
@@ -98,29 +89,16 @@ static void test_ubsan_misaligned_access(void)
*ptr = val;
}

-static void test_ubsan_object_size_mismatch(void)
-{
- /* "((aligned(8)))" helps this not into be misaligned for ptr-access. */
- volatile int val __aligned(8) = 4;
- volatile long long *ptr, val2;
-
- UBSAN_TEST(CONFIG_UBSAN_OBJECT_SIZE);
- ptr = (long long *)&val;
- val2 = *ptr;
-}
-
static const test_ubsan_fp test_ubsan_array[] = {
test_ubsan_shift_out_of_bounds,
test_ubsan_out_of_bounds,
test_ubsan_load_invalid_value,
test_ubsan_misaligned_access,
- test_ubsan_object_size_mismatch,
};

/* Excluded because they Oops the module. */
static const test_ubsan_fp skip_ubsan_array[] = {
test_ubsan_divrem_overflow,
- test_ubsan_null_ptr_deref,
};

static int __init test_ubsan_init(void)
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 9e2092fd5206..7099c603ff0a 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -8,7 +8,6 @@ ubsan-cflags-$(CONFIG_UBSAN_LOCAL_BOUNDS) += -fsanitize=local-bounds
ubsan-cflags-$(CONFIG_UBSAN_SHIFT) += -fsanitize=shift
ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO) += -fsanitize=integer-divide-by-zero
ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE) += -fsanitize=unreachable
-ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE) += -fsanitize=object-size
ubsan-cflags-$(CONFIG_UBSAN_BOOL) += -fsanitize=bool
ubsan-cflags-$(CONFIG_UBSAN_ENUM) += -fsanitize=enum
ubsan-cflags-$(CONFIG_UBSAN_TRAP) += -fsanitize-undefined-trap-on-error
--
2.30.2



2021-12-06 07:01:38

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH] ubsan: Remove CONFIG_UBSAN_OBJECT_SIZE

On Sat, 4 Dec 2021 at 00:53, Kees Cook <[email protected]> wrote:
> The object-size sanitizer is redundant to -Warray-bounds, and
> inappropriately performs its checks at run-time when all information
> needed for the evaluation is available at compile-time, making it quite
> difficult to use:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=214861
>
> With -Warray-bounds almost enabled globally, it doesn't make sense to
> keep this around.
>
> Signed-off-by: Kees Cook <[email protected]>

Reviewed-by: Marco Elver <[email protected]>

Thank you!

> ---
> lib/Kconfig.ubsan | 13 -------------
> lib/test_ubsan.c | 22 ----------------------
> scripts/Makefile.ubsan | 1 -
> 3 files changed, 36 deletions(-)
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index e5372a13511d..236c5cefc4cc 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -112,19 +112,6 @@ config UBSAN_UNREACHABLE
> This option enables -fsanitize=unreachable which checks for control
> flow reaching an expected-to-be-unreachable position.
>
> -config UBSAN_OBJECT_SIZE
> - bool "Perform checking for accesses beyond the end of objects"
> - default UBSAN
> - # gcc hugely expands stack usage with -fsanitize=object-size
> - # https://lore.kernel.org/lkml/CAHk-=wjPasyJrDuwDnpHJS2TuQfExwe=px-SzLeN8GFMAQJPmQ@mail.gmail.com/
> - depends on !CC_IS_GCC
> - depends on $(cc-option,-fsanitize=object-size)
> - help
> - This option enables -fsanitize=object-size which checks for accesses
> - beyond the end of objects where the optimizer can determine both the
> - object being operated on and its size, usually seen with bad downcasts,
> - or access to struct members from NULL pointers.
> -
> 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 7e7bbd0f3fd2..2062be1f2e80 100644
> --- a/lib/test_ubsan.c
> +++ b/lib/test_ubsan.c
> @@ -79,15 +79,6 @@ static void test_ubsan_load_invalid_value(void)
> eval2 = eval;
> }
>
> -static void test_ubsan_null_ptr_deref(void)
> -{
> - volatile int *ptr = NULL;
> - int val;
> -
> - UBSAN_TEST(CONFIG_UBSAN_OBJECT_SIZE);
> - val = *ptr;
> -}
> -
> static void test_ubsan_misaligned_access(void)
> {
> volatile char arr[5] __aligned(4) = {1, 2, 3, 4, 5};
> @@ -98,29 +89,16 @@ static void test_ubsan_misaligned_access(void)
> *ptr = val;
> }
>
> -static void test_ubsan_object_size_mismatch(void)
> -{
> - /* "((aligned(8)))" helps this not into be misaligned for ptr-access. */
> - volatile int val __aligned(8) = 4;
> - volatile long long *ptr, val2;
> -
> - UBSAN_TEST(CONFIG_UBSAN_OBJECT_SIZE);
> - ptr = (long long *)&val;
> - val2 = *ptr;
> -}
> -
> static const test_ubsan_fp test_ubsan_array[] = {
> test_ubsan_shift_out_of_bounds,
> test_ubsan_out_of_bounds,
> test_ubsan_load_invalid_value,
> test_ubsan_misaligned_access,
> - test_ubsan_object_size_mismatch,
> };
>
> /* Excluded because they Oops the module. */
> static const test_ubsan_fp skip_ubsan_array[] = {
> test_ubsan_divrem_overflow,
> - test_ubsan_null_ptr_deref,
> };
>
> static int __init test_ubsan_init(void)
> diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
> index 9e2092fd5206..7099c603ff0a 100644
> --- a/scripts/Makefile.ubsan
> +++ b/scripts/Makefile.ubsan
> @@ -8,7 +8,6 @@ ubsan-cflags-$(CONFIG_UBSAN_LOCAL_BOUNDS) += -fsanitize=local-bounds
> ubsan-cflags-$(CONFIG_UBSAN_SHIFT) += -fsanitize=shift
> ubsan-cflags-$(CONFIG_UBSAN_DIV_ZERO) += -fsanitize=integer-divide-by-zero
> ubsan-cflags-$(CONFIG_UBSAN_UNREACHABLE) += -fsanitize=unreachable
> -ubsan-cflags-$(CONFIG_UBSAN_OBJECT_SIZE) += -fsanitize=object-size
> ubsan-cflags-$(CONFIG_UBSAN_BOOL) += -fsanitize=bool
> ubsan-cflags-$(CONFIG_UBSAN_ENUM) += -fsanitize=enum
> ubsan-cflags-$(CONFIG_UBSAN_TRAP) += -fsanitize-undefined-trap-on-error
> --
> 2.30.2
>