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

From: Kees Cook <kees@kernel.org>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
	Kees Cook <keescook@chromium.org>
Cc: linux-hardening@vger.kernel.org,
	Justin Stitt <justinstitt@google.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Marco Elver <elver@google.com>, Hao Luo <haoluo@google.com>,
	Przemek Kitszel <przemyslaw.kitszel@intel.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Bill Wendling <morbo@google.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/82] overflow: Reintroduce signed and unsigned overflow sanitizers
Date: Mon, 22 Jan 2024 20:45:22 -0800	[thread overview]
Message-ID: <14B4D24C-4CBA-401E-8111-CF74482CA956@kernel.org> (raw)
In-Reply-To: <CANiq72kWTu_1YcmSP=1sB6CmrdbQcksxFvLci6VC2u8_Bb-Exw@mail.gmail.com>

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