[clang] Sanitizer: Support -fwrapv with -fsanitize=signed-integer-overflow (PR #82432)

Justin Stitt via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 20 14:31:08 PST 2024


https://github.com/JustinStitt created https://github.com/llvm/llvm-project/pull/82432

**Reasoning**
Clang has a `signed-integer-overflow` sanitizer to catch arithmetic overflow; however, most of its instrumentation [fails to apply](https://godbolt.org/z/ee41rE8o6) when `-fwrapv` is enabled; this is by design.

The Linux kernel enables `-fno-strict-overflow` which implies `-fwrapv`. This means we are [currently unable to detect signed-integer wrap-around](https://github.com/KSPP/linux/issues/26). All the while, the root cause of many security vulnerabilities in the Linux kernel is [arithmetic overflow](https://cwe.mitre.org/data/definitions/190.html). 

To work around this and enhance the functionality of `-fsanitize=signed-integer-overflow`, let's instrument signed arithmetic even if the signed overflow behavior is defined.

Initially, I created a [new sanitizer @ (pr/80089)](https://github.com/llvm/llvm-project/pull/80089) but simply changing the SIO sanitizer itself may be the better approach as per @MaskRay 's review from that PR.

cc: @nickdesaulniers @kees @nathanchance @bwendling @MaskRay 


>From b02b09b9eb4f9a8ac60dd077d95c67b959db3b70 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt at google.com>
Date: Tue, 20 Feb 2024 22:21:02 +0000
Subject: [PATCH] support fwrapv with signed int overflow sanitizer

---
 clang/docs/ReleaseNotes.rst               |  3 +++
 clang/docs/UndefinedBehaviorSanitizer.rst |  9 +++++----
 clang/lib/CodeGen/CGExprScalar.cpp        | 16 ++++++++++++----
 clang/test/CodeGen/integer-overflow.c     | 12 ++++++++++++
 4 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5bca2c965c866b..685b19cabeb82c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -399,6 +399,9 @@ Moved checkers
 Sanitizers
 ----------
 
+- ``-fsanitize=signed-integer-overflow`` now instruments signed arithmetic even
+  when ``-fwrapv`` is enabled. Previously, only division checks were enabled.
+
 Python Binding Changes
 ----------------------
 
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst
index b8ad3804f18903..8f58c92bd2a163 100644
--- a/clang/docs/UndefinedBehaviorSanitizer.rst
+++ b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -190,10 +190,11 @@ Available checks are:
   -  ``-fsanitize=signed-integer-overflow``: Signed integer overflow, where the
      result of a signed integer computation cannot be represented in its type.
      This includes all the checks covered by ``-ftrapv``, as well as checks for
-     signed division overflow (``INT_MIN/-1``), but not checks for
-     lossy implicit conversions performed before the computation
-     (see ``-fsanitize=implicit-conversion``). Both of these two issues are
-     handled by ``-fsanitize=implicit-conversion`` group of checks.
+     signed division overflow (``INT_MIN/-1``). Note that checks are still
+     added even when ``-fwrapv`` is enabled. This sanitizer does not check for
+     lossy implicit conversions performed before the computation (see
+     ``-fsanitize=implicit-conversion``). Both of these two issues are handled
+     by ``-fsanitize=implicit-conversion`` group of checks.
   -  ``-fsanitize=unreachable``: If control flow reaches an unreachable
      program point.
   -  ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 576734e460b9c1..7621d9bcdec991 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -723,7 +723,9 @@ class ScalarExprEmitter
     if (Ops.Ty->isSignedIntegerOrEnumerationType()) {
       switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
       case LangOptions::SOB_Defined:
-        return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
+        if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
+              return Builder.CreateMul(Ops.LHS, Ops.RHS, "mul");
+        [[fallthrough]];
       case LangOptions::SOB_Undefined:
         if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
           return Builder.CreateNSWMul(Ops.LHS, Ops.RHS, "mul");
@@ -2568,7 +2570,9 @@ llvm::Value *ScalarExprEmitter::EmitIncDecConsiderOverflowBehavior(
   StringRef Name = IsInc ? "inc" : "dec";
   switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
   case LangOptions::SOB_Defined:
-    return Builder.CreateAdd(InVal, Amount, Name);
+    if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
+        return Builder.CreateAdd(InVal, Amount, Name);
+    [[fallthrough]];
   case LangOptions::SOB_Undefined:
     if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
       return Builder.CreateNSWAdd(InVal, Amount, Name);
@@ -3913,7 +3917,9 @@ Value *ScalarExprEmitter::EmitAdd(const BinOpInfo &op) {
   if (op.Ty->isSignedIntegerOrEnumerationType()) {
     switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
     case LangOptions::SOB_Defined:
-      return Builder.CreateAdd(op.LHS, op.RHS, "add");
+      if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
+        return Builder.CreateAdd(op.LHS, op.RHS, "add");
+      [[fallthrough]];
     case LangOptions::SOB_Undefined:
       if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
         return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
@@ -4067,7 +4073,9 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
     if (op.Ty->isSignedIntegerOrEnumerationType()) {
       switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
       case LangOptions::SOB_Defined:
-        return Builder.CreateSub(op.LHS, op.RHS, "sub");
+        if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
+          return Builder.CreateSub(op.LHS, op.RHS, "sub");
+        [[fallthrough]];
       case LangOptions::SOB_Undefined:
         if (!CGF.SanOpts.has(SanitizerKind::SignedIntegerOverflow))
           return Builder.CreateNSWSub(op.LHS, op.RHS, "sub");
diff --git a/clang/test/CodeGen/integer-overflow.c b/clang/test/CodeGen/integer-overflow.c
index 9a3107c0b52926..b6d1a9fc4cf744 100644
--- a/clang/test/CodeGen/integer-overflow.c
+++ b/clang/test/CodeGen/integer-overflow.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -fwrapv | FileCheck %s --check-prefix=WRAPV
 // RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -ftrapv | FileCheck %s --check-prefix=TRAPV
 // RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=CATCH_UB
+// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -fsanitize=signed-integer-overflow -fwrapv | FileCheck %s --check-prefix=CATCH_WRAP
 // RUN: %clang_cc1 -triple x86_64-apple-darwin %s -emit-llvm -o - -ftrapv -ftrapv-handler foo | FileCheck %s --check-prefix=TRAPV_HANDLER
 
 
@@ -16,6 +17,7 @@ void test1(void) {
   // WRAPV: add i32
   // TRAPV: llvm.sadd.with.overflow.i32
   // CATCH_UB: llvm.sadd.with.overflow.i32
+  // CATCH_WRAP: llvm.sadd.with.overflow.i32
   // TRAPV_HANDLER: foo(
   f11G = a + b;
   
@@ -23,6 +25,7 @@ void test1(void) {
   // WRAPV: sub i32
   // TRAPV: llvm.ssub.with.overflow.i32
   // CATCH_UB: llvm.ssub.with.overflow.i32
+  // CATCH_WRAP: llvm.ssub.with.overflow.i32
   // TRAPV_HANDLER: foo(
   f11G = a - b;
   
@@ -30,6 +33,7 @@ void test1(void) {
   // WRAPV: mul i32
   // TRAPV: llvm.smul.with.overflow.i32
   // CATCH_UB: llvm.smul.with.overflow.i32
+  // CATCH_WRAP: llvm.smul.with.overflow.i32
   // TRAPV_HANDLER: foo(
   f11G = a * b;
 
@@ -37,6 +41,7 @@ void test1(void) {
   // WRAPV: sub i32 0, 
   // TRAPV: llvm.ssub.with.overflow.i32(i32 0
   // CATCH_UB: llvm.ssub.with.overflow.i32(i32 0
+  // CATCH_WRAP: llvm.ssub.with.overflow.i32(i32 0
   // TRAPV_HANDLER: foo(
   f11G = -a;
   
@@ -46,6 +51,7 @@ void test1(void) {
   // WRAPV: add i32 {{.*}}, 1
   // TRAPV: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
   // CATCH_UB: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
+  // CATCH_WRAP: llvm.sadd.with.overflow.i32({{.*}}, i32 1)
   // TRAPV_HANDLER: foo(
   ++a;
   
@@ -53,6 +59,7 @@ void test1(void) {
   // WRAPV: add i32 {{.*}}, -1
   // TRAPV: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
   // CATCH_UB: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
+  // CATCH_WRAP: llvm.ssub.with.overflow.i32({{.*}}, i32 1)
   // TRAPV_HANDLER: foo(
   --a;
   
@@ -70,6 +77,7 @@ void test1(void) {
   // WRAPV: add i8 {{.*}}, 1
   // TRAPV: add i8 {{.*}}, 1
   // CATCH_UB: add i8 {{.*}}, 1
+  // CATCH_WRAP: add i8 {{.*}}, 1
   ++PR9350_char_inc;
 
   // PR9350: char pre-decrement never overflows.
@@ -78,6 +86,7 @@ void test1(void) {
   // WRAPV: add i8 {{.*}}, -1
   // TRAPV: add i8 {{.*}}, -1
   // CATCH_UB: add i8 {{.*}}, -1
+  // CATCH_WRAP: add i8 {{.*}}, -1
   --PR9350_char_dec;
 
   // PR9350: short pre-increment never overflows.
@@ -86,6 +95,7 @@ void test1(void) {
   // WRAPV: add i16 {{.*}}, 1
   // TRAPV: add i16 {{.*}}, 1
   // CATCH_UB: add i16 {{.*}}, 1
+  // CATCH_WRAP: add i16 {{.*}}, 1
   ++PR9350_short_inc;
 
   // PR9350: short pre-decrement never overflows.
@@ -94,6 +104,7 @@ void test1(void) {
   // WRAPV: add i16 {{.*}}, -1
   // TRAPV: add i16 {{.*}}, -1
   // CATCH_UB: add i16 {{.*}}, -1
+  // CATCH_WRAP: add i16 {{.*}}, -1
   --PR9350_short_dec;
 
   // PR24256: don't instrument __builtin_frame_address.
@@ -102,4 +113,5 @@ void test1(void) {
   // WRAPV:    call ptr @llvm.frameaddress.p0(i32 0)
   // TRAPV:    call ptr @llvm.frameaddress.p0(i32 0)
   // CATCH_UB: call ptr @llvm.frameaddress.p0(i32 0)
+  // CATCH_WRAP: call ptr @llvm.frameaddress.p0(i32 0)
 }



More information about the cfe-commits mailing list