[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 Fangrui Song changed: What|Removed |Added CC||i at maskray dot me --- Comment #13 from Fangrui Song --- I see a Clang patch that proposes -fsanitize=signed-integer-wrap, which appears to be the same as signed-integer-overflow, but performs the check in the -fwrapv mode. I feel that it's better to make -fsanitize=signed-integer-overflow work with -fwrapv https://github.com/llvm/llvm-project/pull/80089#issuecomment-1945202620 --- Copying here for folks prefer not to read github This is a UI discussion about how command line options should behave. Some folks prefer simpler rules while some prefer smart rules (guessing user intention). A [-fwrapv](https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fwrapv) user may either: * rely on the wraparound behavior * or prevent certain optimizations that would raise security concerns Our -fsanitize=signed-integer-overflow design have been assuming that -fwrapv users don't need the check. This PR suggests that an important user does want overflow checks and our guess has failed. It seems very confusing to have two options doing the same thing. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html is clear that not all checks are undefined behavior in the standards. > Issues caught by this sanitizer are not undefined behavior, but are often > unintentional. Sure -fwrapv makes wraparound defined, but it doesn't prevent us from making -fsanitize=signed-integer-overflow useful. "-fwrapv => no signed-integer-overflow" is not a solid argument. I think we can try making -fsanitize=signed-integer-overflow effective even when -fwrapv if specified. -fsanitize=signed-integer-overflow is rare in the wild, probably rarer when combined with -fwrapv. There is a precedent that -fsanitize=undefined enables different checks for different targets. We could make -fsanitize=undefined not imply -fsanitize=signed-integer-overflow when -fwrapv is specified, if we do want to guess the user intention. Personally I'd prefer moving away from such behaviors and be more orthogonal.
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #12 from qinzhao at gcc dot gnu.org --- (In reply to Kees Cook from comment #11) > The trouble with "optimize" is that it just doesn't work. The kernel has > banned its use because it results in all other optimization options being > forgotten for the function in question. How about Jacub's another suggestion in comment#10: "If you don't want to use optimize attribute, there is always the option to just do the arithmetics in unsigned types in the few selected functions where you don't want the sanitization"? is it possible to use "unsigned" integer instead of "signed" integer for the cases you want the "wrap around" behavior when overflow? if not, what's the major issue with this workaround?
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #11 from Kees Cook --- The trouble with "optimize" is that it just doesn't work. The kernel has banned its use because it results in all other optimization options being forgotten for the function in question.
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #10 from Jakub Jelinek --- The optimize attribute is how different options are represented in LTO compilation, so it grew over years from perhaps initial debugging use to something that is used everywhere. And we definitely aren't going to add further and further attributes that match just a small subset of the optimize and/or target attributes, especially when they'd need to use the same infrastructure under the hood anyway. If you don't want to use optimize attribute, there is always the option to just do the arithmetics in unsigned types in the few selected functions where you don't want the sanitization, and if you really want use -fwrapv-pointer together with -fsanitize=integer-signed-overflow and throw away -fno-strict-overflow.
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317
--- Comment #9 from Kees Cook ---
(In reply to Jakub Jelinek from comment #8)
> So, instead (when building the kernel with sanitization) build with
> -fsanitize=signed-integer-overflow and no -fno-strict-overflow, and
> the routines where you want wrapv behavior and not runtime traps build with
> optimize ("wrapv", "wrapv-pointer") attribute?
__attribute__((optimize)) is documented as not for production use ("for
debugging purposes only"):
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-optimize-function-attribute
and the kernel has hit multiple problems with it. As such, all use has been
removed, for example:
https://lore.kernel.org/lkml/20201027205723.12514-1-a...@kernel.org/
https://lore.kernel.org/lkml/20201028080433.26799-1-a...@kernel.org/
https://lore.kernel.org/lkml/20210118105557.186614-3-adrian.ra...@collabora.com/
If there were an __attribute__((wrapv)) and __attribute__((wrapv-pointer)), we
could create the wrapping helpers with those and
__attribute__((no_sanitize("signed-integer-overflow")))
FWIW, I've been trying to track this issue in the kernel here:
https://github.com/KSPP/linux/issues/26
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317
--- Comment #8 from Jakub Jelinek ---
So, instead (when building the kernel with sanitization) build with
-fsanitize=signed-integer-overflow and no -fno-strict-overflow, and
the routines where you want wrapv behavior and not runtime traps build with
optimize ("wrapv", "wrapv-pointer") attribute?
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317
--- Comment #7 from Kees Cook ---
The problem the kernel needs to solve is basically having our cake and eating
it too. :)
In _most_ situations, we want signed overflows to trap (i.e. get caught by
"-fsanitize=signed-integer-overflow").
In some limited situations, we want to be able to depend on 2s-complement
wrap-around on overflow -- and we would create a helper function to do this
work, marked with __attribute__((no_sanitize("signed-integer-overflow"))). (We
need this most for our atomic_t/refcount_t code, as well as all the open-coded
bounds checks -- see details below.)
For the latter case (wrap-around), the kernel must depend on the effects of
"-fwrapv-pointer" and "-fwrapv" because otherwise some cases end up being
elided due to being classified as undefined behavior.
(In reply to Jakub Jelinek from comment #6)
> That doesn't make sense. -fsanitize=signed-integer-overflow also removes
> that undefined behavior by defining what happens on signed integer overflow,
> one can choose whether to get a non-fatal runtime diagnostic + wrapv
> behavior, or fatal runtime diagnostic, or just abort.
There doesn't appear to be a way to remove the UB _and_ the diagnostic on a
case-by-case basis.
The matrix of behaviors:
code generation: UB, no UB (2s-complement wrap-around)
diagnostics: no diagnostic, warn or trap
-fno-strict-overflow provides "no UB", and "no diagnostic". ("warn or trap" are
not available.)
-fsanitize=signed-integer-overflow provides "no UB" and "warn or trap". ("no
diagnostic" is not available; using
__attribute__((no_sanitize("signed-integer-overflow"))) also removes "no UB")
So, on a per-function basis, we need to _either_ catch overflow _or_
deterministically perform 2s-complement wrap-around.
If there's a way to achieve this already, I would be very grateful, as I've not
been able to find the correct combination.
Background on the kernel's use of -fno-strict-overflow:
https://git.kernel.org/linus/a137802ee839ace40079bebde24cfb416f73208a
and (earlier) -fwrapv:
https://git.kernel.org/linus/68df3755e383e6fecf2354a67b08f92f18536594
An example of wanting no-UB without diagnostic:
https://git.kernel.org/linus/adb03115f4590baa280ddc440a8eff08a6be0cb7
which ultimately had to be reverted:
https://git.kernel.org/linus/a6211caa634da39d861a47437ffcda8b38ef421b
and all of UBSAN integer overflow support was removed:
https://git.kernel.org/linus/6aaa31aeb9cf260e1b7155cc11ec864f052db5ec
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #6 from Jakub Jelinek --- That doesn't make sense. -fsanitize=signed-integer-overflow also removes that undefined behavior by defining what happens on signed integer overflow, one can choose whether to get a non-fatal runtime diagnostic + wrapv behavior, or fatal runtime diagnostic, or just abort. So, when you use -fsanitize=signed-integer-overflow, you don't want -fwrapv or -fno-strict-overflow, unless you want the former to be basically a nop.
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #5 from Qing Zhao --- > On Sep 13, 2021, at 4:45 PM, pinskia at gcc dot gnu.org > wrote: > >> is it possible to make -fsanitize=signed-integer-overflow work with -fwrapv? > > Why would it? they conflict. This is a feature that is requested by Kees Cook for kernel security usage. "the kernel builds with -fno-strict-overflow which removes possible undefined behavior, but I still want the sanitizer to catch this case.”
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 Jakub Jelinek changed: What|Removed |Added Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED --- Comment #4 from Jakub Jelinek --- Well, -fwrapv says that signed integer overflow is well defined, therefore there is nothing for -fsanitize=signed-integer-overflow to diagnose. It is like if you've done all the arithmetics that could overflow in unsigned types...
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #3 from Andrew Pinski --- (In reply to qinzhao from comment #2) > (In reply to Andrew Pinski from comment #1) > > -fno-strict-overflow maps directly to -fwrapv . > > > > If you want to use -fsanitize=signed-integer-overflow, you can just remove > > both -fno-strict-overflow -fwrapv. -fwrapv is implied for code later on. > > is it possible to make -fsanitize=signed-integer-overflow work with -fwrapv? Why would it? they conflict.
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #2 from qinzhao at gcc dot gnu.org --- (In reply to Andrew Pinski from comment #1) > -fno-strict-overflow maps directly to -fwrapv . > > If you want to use -fsanitize=signed-integer-overflow, you can just remove > both -fno-strict-overflow -fwrapv. -fwrapv is implied for code later on. is it possible to make -fsanitize=signed-integer-overflow work with -fwrapv?
[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317 --- Comment #1 from Andrew Pinski --- -fno-strict-overflow maps directly to -fwrapv . If you want to use -fsanitize=signed-integer-overflow, you can just remove both -fno-strict-overflow -fwrapv. -fwrapv is implied for code later on.
