Received-SPF: pass (google.com: domain of linux-kernel+bounces-34706-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 22 Jan 2024 20:45:22 -0800
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: =?US-ASCII?Q?Re=3A_=5BPATCH_06/82=5D_overflow=3A_Reintroduce?= =?US-ASCII?Q?_signed_and_unsigned_overflow_sanitizers?=
User-Agent: K-9 Mail for Android
In-Reply-To: <CANiq72kWTu_1YcmSP=1sB6CmrdbQcksxFvLci6VC2u8_Bb-Exw@mail.gmail.com>
References: <20240122235208.work.748-kees@kernel.org> <20240123002814.1396804-6-keescook@chromium.org> <CANiq72kWTu_1YcmSP=1sB6CmrdbQcksxFvLci6VC2u8_Bb-Exw@mail.gmail.com>
Message-ID: <14B4D24C-4CBA-401E-8111-CF74482CA956@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable


On January 22, 2024 6:24:14 PM PST, Miguel Ojeda <miguel=2Eojeda=2Esandoni=
s@gmail=2Ecom> wrote:
>On Tue, Jan 23, 2024 at 1:28=E2=80=AFAM Kees Cook <keescook@chromium=2Eor=
g> wrote:
>>
>> Because the kernel is built with -fno-strict-overflow, signed and point=
er
>> 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)=2E
>
>By elided I guess you also mean assumed to not happen and thus the
>usual chain-of-logic magic?

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

>
>> So, the config options are added back as CONFIG_UBSAN_SIGNED_WRAP and
>> CONFIG_UBSAN_UNSIGNED_WRAP=2E Since the kernel has several places that
>> explicitly depend on wrap-around behavior (e=2Eg=2E counters, atomics, =
etc),
>> also introduce the __signed_wrap and __unsigned_wrap function attribute=
s
>> for annotating functions where wrapping is expected and should not
>> be caught=2E This will allow us to distinguish in the kernel between
>> intentional and unintentional cases of arithmetic wrap-around=2E
>
>Sounds good -- it seems to go in the direction of Rust, i=2Ee=2E to have =
a
>way to mark expected wrap-arounds so that we can start catching the
>unintended ones=2E

Yup! That's the plan=2E

>
>> +       depends on !COMPILE_TEST
>> +       depends on $(cc-option,-fsanitize=3Dsigned-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=2E

>
>> +       depends on !X86_32 # avoid excessive stack usage on x86-32/clan=
g
>> +       depends on !COMPILE_TEST
>> +       help
>> +         This option enables -fsanitize=3Dunsigned-integer-overflow wh=
ich checks
>> +         for wrap-around of any arithmetic operations with unsigned in=
tegers=2E This
>> +         currently causes x86 to fail to boot=2E
>
>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=2E

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

>
>> +static void test_ubsan_sub_overflow(void)
>> +{
>> +       volatile int val =3D INT_MIN;
>> +       volatile unsigned int uval =3D 0;
>> +       volatile int val2 =3D 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 jus=
t restored them as they were=2E :)

-Kees

--=20
Kees Cook