Re: [PATCH 06/82] overflow: Reintroduce signed and unsigned overflow sanitizers

[Miguel Ojeda]

On Tue, Jan 23, 2024 at 1:28 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> Because the kernel is built with -fno-strict-overflow, signed and pointer
> arithmetic is defined to always wrap around instead of "overflowing"
> (which would either be elided due to being undefined behavior or would
> wrap around, which led to very weird bugs in the kernel).

By elided I guess you also mean assumed to not happen and thus the
usual chain-of-logic magic?

> So, the config options are added back as CONFIG_UBSAN_SIGNED_WRAP and
> CONFIG_UBSAN_UNSIGNED_WRAP. Since the kernel has several places that
> explicitly depend on wrap-around behavior (e.g. counters, atomics, etc),
> also introduce the __signed_wrap and __unsigned_wrap function attributes
> for annotating functions where wrapping is expected and should not
> be caught. This will allow us to distinguish in the kernel between
> intentional and unintentional cases of arithmetic wrap-around.

Sounds good -- it seems to go in the direction of Rust, i.e. to have a
way to mark expected wrap-arounds so that we can start catching the
unintended ones.

> +       depends on !COMPILE_TEST
> +       depends on $(cc-option,-fsanitize=signed-integer-overflow)

Maybe this line goes above the other, to be consistent with the
unsigned case? (or the other way around)

> +       depends on !X86_32 # avoid excessive stack usage on x86-32/clang
> +       depends on !COMPILE_TEST
> +       help
> +         This option enables -fsanitize=unsigned-integer-overflow which checks
> +         for wrap-around of any arithmetic operations with unsigned integers. This
> +         currently causes x86 to fail to boot.

Is it related to the excessive stack usage? In that case, users would
not reach the point to see this description, right? If so, I guess it
could be removed from the `help` and moved into the comment above or
similar.

> +static void test_ubsan_sub_overflow(void)
> +{
> +       volatile int val = INT_MIN;
> +       volatile unsigned int uval = 0;
> +       volatile int val2 = 2;

In the other tests you use a constant instead of `val2`, I am curious
if there is a reason for it?

Thanks!

Cheers,
Miguel