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

[Kees Cook <keescook@chromium.org>]

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@google.com>
> > Cc: Miguel Ojeda <ojeda@kernel.org>
> > Cc: Nathan Chancellor <nathan@kernel.org>
> > Cc: Nick Desaulniers <ndesaulniers@google.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Hao Luo <haoluo@google.com>
> > Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > Signed-off-by: Kees Cook <keescook@chromium.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@google.com>

Thanks!

-Kees

-- 
Kees Cook