Message-ID: <14B4D24C-4CBA-401E-8111-CF74482CA956@kernel.org>
Date: Mon, 22 Jan 2024 20:45:22 -0800
From: Kees Cook <kees@...nel.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
 Kees Cook <keescook@...omium.org>
CC: linux-hardening@...r.kernel.org, Justin Stitt <justinstitt@...gle.com>,
 Miguel Ojeda <ojeda@...nel.org>, Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <ndesaulniers@...gle.com>,
 Peter Zijlstra <peterz@...radead.org>, Marco Elver <elver@...gle.com>,
 Hao Luo <haoluo@...gle.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Bill Wendling <morbo@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/82] overflow: Reintroduce signed and unsigned overflow sanitizers



On January 22, 2024 6:24:14 PM PST, Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:
>On Tue, Jan 23, 2024 at 1:28 AM Kees Cook <keescook@...omium.org> 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

Powered by blists - more mailing lists