linux-hardening - Re: [PATCH 2/6] ubsan: Reintroduce signed and unsigned overflow sanitizers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 29 Jan 2024 12:22:40 -0800
From: Kees Cook <keescook@...omium.org>
To: Justin Stitt <justinstitt@...gle.com>
Cc: Marco Elver <elver@...gle.com>, Miguel Ojeda <ojeda@...nel.org>,
	Nathan Chancellor <nathan@...nel.org>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Peter Zijlstra <peterz@...radead.org>, Hao Luo <haoluo@...gle.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Fangrui Song <maskray@...gle.com>,
	Masahiro Yamada <masahiroy@...nel.org>,
	Bill Wendling <morbo@...gle.com>,
	Nicolas Schier <nicolas@...sle.eu>,
	Andrey Konovalov <andreyknvl@...il.com>,
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
	llvm@...ts.linux.dev, linux-hardening@...r.kernel.org
Subject: Re: [PATCH 2/6] ubsan: Reintroduce signed and unsigned overflow
 sanitizers

On Mon, Jan 29, 2024 at 07:54:18PM +0000, Justin Stitt wrote:
> Hi,
> 
> On Mon, Jan 29, 2024 at 10:00:39AM -0800, Kees Cook wrote:
> > Effectively revert commit 6aaa31aeb9cf ("ubsan: remove overflow
> > checks"), to allow the kernel to be built with the "overflow"
> > sanitizers again. This gives developers a chance to experiment[1][2][3]
> > with the instrumentation again, while compilers adjust their sanitizers
> > to deal with the impact of -fno-strict-oveflow (i.e. moving from
> > "overflow" checking to "wrap-around" checking).
> >
> > Notably, the naming of the options is adjusted to use the name "WRAP"
> > instead of "OVERFLOW". In the strictest sense, arithmetic "overflow"
> > happens when a result exceeds the storage of the type, and is considered
> > by the C standard and compilers to be undefined behavior for signed
> > and pointer types (without -fno-strict-overflow). Unsigned arithmetic
> > overflow is defined as always wrapping around.
> >
> > Because the kernel is built with -fno-strict-overflow, signed and pointer
> > arithmetic is defined to always wrap around instead of "overflowing"
> > (which could either be elided due to being undefined behavior or would
> > wrap around, which led to very weird bugs in the kernel).
> >
> > 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, crypto,
> > etc), also introduce the __signed_wrap and __unsigned_wrap function
> > attributes for annotating functions where wrapping is expected and should
> > not be instrumented. This will allow us to distinguish in the kernel
> > between intentional and unintentional cases of arithmetic wrap-around.
> >
> > Additionally keep these disabled under CONFIG_COMPILE_TEST for now.
> 
> This is present in the patch but perhaps its worth noting here that x86
> has trouble booting with the unsigned-integer-overflow sanitizer on.

Yeah, though this is fixed later in the series.

> 
> >
> > Link: https://github.com/KSPP/linux/issues/26 [1]
> > Link: https://github.com/KSPP/linux/issues/27 [2]
> > Link: https://github.com/KSPP/linux/issues/344 [3]
> > Cc: Justin Stitt <justinstitt@...gle.com>
> > Cc: Miguel Ojeda <ojeda@...nel.org>
> > Cc: Nathan Chancellor <nathan@...nel.org>
> > Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Marco Elver <elver@...gle.com>
> > Cc: Hao Luo <haoluo@...gle.com>
> > Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> 
> 
> This patch adheres to the language semantics as I understand them.
> Moreover, we would've had to send a patch similar to this once we land
> some better sanitizer + -fno-strict-oveflow support in the compilers.
> 
> Currently, though, -fsanitize=signed-integer-overflow instruments very
> little (if anything at all) due to compiler optimizations in conjunction
> with -fno-strict-oveflow. I am working on a new
> -fsanitize=signed-integer-wrap in Clang which will instrument more
> arithmetic even under -fno-strict-oveflow.

Agreed -- I'm mainly getting these back into the kernel so folks working
on this have a common base to work from.

> Reviewed-by: Justin Stitt <justinstitt@...gle.com>

Thanks!

-Kees

-- 
Kees Cook