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

[Kees Cook]

On January 22, 2024 6:24:14 PM PST, Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>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?

Yes. We removed this bad behavior by using -fno-strict-overflow, and we will want to keep it enabled.

>
>> 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.

Yup! That's the plan.

>
>> +       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)

Sure, I can move it 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.

The stack usage is separate. (This may even be fixed in modern Clang; this comes from the original version of this Kconfig.) The not booting part is separate and has not been tracked down yet.

>
>> +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?

I wondered the same -- they were this way when they were removed, so I just restored them as they were. :)

-Kees

-- 
Kees Cook