[Bug sanitizer/102317] New: signed integer overflow sanitizer cannot work well with -fno-strict-overflow

[Bug sanitizer/102317] New: signed integer overflow sanitizer cannot work well with -fno-strict-overflow

[qinzhao at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

            Bug ID: 102317
           Summary: signed integer overflow sanitizer cannot work well
                    with -fno-strict-overflow
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: qinzhao at gcc dot gnu.org
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

with the latest gcc12:
$ cat wrap.c
#include <stdio.h>
#include <limits.h>

/* volatile to avoid optimization */
volatile int val;

int main(void)
{
   val = INT_MAX;
   val += 1;
   printf("%d\n", val);
   return 0;
}
$ gcc -Wall -O2 -fsanitize=signed-integer-overflow -o wrap wrap.c
$ ./wrap
wrap.c:10:9: runtime error: signed integer overflow: 2147483647 + 1 cannot be
represented in type 'int'
-2147483648

Things work as expected: the overflow is detected and in this warning
mode, the result is a wrap-around.

However, the kernel builds with -fno-strict-overflow which removes
possible undefined behavior, but I still want the sanitizer to catch
this case. Currently it doesn't:

$ gcc -Wall -O2 -fsanitize=signed-integer-overflow -fno-strict-overflow -o wrap
wrap.c
$ ./wrap
-2147483648

[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
-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.

[Bug sanitizer/102317] signed integer overflow sanitizer cannot work well with -fno-strict-overflow

[qinzhao at gcc dot gnu.org]

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

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[qing.zhao at oracle dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #5 from Qing Zhao <qing.zhao at oracle dot com> ---
> On Sep 13, 2021, at 4:45 PM, pinskia at gcc dot gnu.org <gcc-bugzilla@gcc.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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #7 from Kees Cook <kees at outflux dot net> ---
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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #9 from Kees Cook <kees at outflux dot net> ---
(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-ardb@kernel.org/
https://lore.kernel.org/lkml/20201028080433.26799-1-ardb@kernel.org/
https://lore.kernel.org/lkml/20210118105557.186614-3-adrian.ratiu@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

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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

[kees at outflux dot net]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

--- Comment #11 from Kees Cook <kees at outflux dot net> ---
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

[qinzhao at gcc dot gnu.org]

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

[i at maskray dot me]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317

Fangrui Song <i at maskray dot me> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |i at maskray dot me

--- Comment #13 from Fangrui Song <i at maskray dot me> ---
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.