jfb updated this revision to Diff 288383.
jfb marked 6 inline comments as done.
jfb added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86000/new/
https://reviews.llvm.org/D86000
Files:
clang/docs/UndefinedBehaviorSanitizer.rst
clang/include/clang/Basic/Sanitizers.def
clang/lib/CodeGen/CGExprScalar.cpp
clang/lib/Driver/ToolChain.cpp
clang/test/CodeGen/unsigned-shift-base.c
clang/test/Driver/fsanitize.c
compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
llvm/docs/ReleaseNotes.rst
Index: llvm/docs/ReleaseNotes.rst
===================================================================
--- llvm/docs/ReleaseNotes.rst
+++ llvm/docs/ReleaseNotes.rst
@@ -140,6 +140,18 @@
Changes to LLDB
---------------------------------
+Changes to Sanitizers
+---------------------
+
+The integer sanitizer `-fsanitize=integer` now has a new sanitizer:
+`-fsanitize=unsigned-shift-base`. It's not undefined behavior for an unsigned
+left shift to overflow (i.e. to shift bits out), but it has been the source of
+bugs and exploits in certain codebases in the past.
+
+One can silence the check with masking, for example:
+
+ `(lhs & ~(~1U << ((sizeof(lhs)*CHAR_BIT) - rhs))) << rhs`
+
External Open Source Projects Using LLVM 12
===========================================
Index: compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
===================================================================
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Integer/unsigned-shift.cpp
@@ -0,0 +1,54 @@
+// RUN: %clangxx -fsanitize=unsigned-shift-base %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+// RUN: %clangxx -fsanitize=unsigned-shift-base,shift-exponent %s -o %t1 && not %run %t1 2>&1 | FileCheck %s
+
+#define shift(val, amount) ({ \
+ volatile unsigned _v = (val); \
+ volatile unsigned _a = (amount); \
+ unsigned res = _v << _a; \
+ res; \
+})
+
+int main() {
+
+ shift(0b00000000'00000000'00000000'00000000, 31);
+ shift(0b00000000'00000000'00000000'00000001, 31);
+ shift(0b00000000'00000000'00000000'00000010, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'00000100, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'00001000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'00010000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'00100000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'01000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 64 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000000'10000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 128 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000001'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 256 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000010'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 512 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00000100'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1024 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00001000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2048 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00010000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4096 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'00100000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8192 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'01000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16384 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000000'10000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 32768 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000001'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 65536 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000010'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 131072 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00000100'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 262144 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00001000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 524288 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00010000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1048576 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'00100000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2097152 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'01000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4194304 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000000'10000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 8388608 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000001'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 16777216 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000010'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 33554432 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00000100'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 67108864 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00001000'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 134217728 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00010000'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 268435456 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b00100000'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 536870912 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b01000000'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 1073741824 by 31 places cannot be represented in type 'unsigned int'
+ shift(0b10000000'00000000'00000000'00000000, 31); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2147483648 by 31 places cannot be represented in type 'unsigned int'
+
+ shift(0b10000000'00000000'00000000'00000000, 00);
+ shift(0b10000000'00000000'00000000'00000000, 01); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 2147483648 by 1 places cannot be represented in type 'unsigned int'
+
+ shift(0xffff'ffff, 0);
+ shift(0xffff'ffff, 1); // CHECK: unsigned-shift.cpp:[[@LINE]]:3: runtime error: left shift of 4294967295 by 1 places cannot be represented in type 'unsigned int'
+
+ return 1;
+}
Index: clang/test/Driver/fsanitize.c
===================================================================
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -32,7 +32,7 @@
// CHECK-COVERAGE-WIN64: "--dependent-lib={{[^"]*}}ubsan_standalone-x86_64.lib"
// RUN: %clang -target %itanium_abi_triple -fsanitize=integer %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-INTEGER -implicit-check-not="-fsanitize-address-use-after-scope"
-// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change),?){8}"}}
+// CHECK-INTEGER: "-fsanitize={{((signed-integer-overflow|unsigned-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|implicit-unsigned-integer-truncation|implicit-signed-integer-truncation|implicit-integer-sign-change|unsigned-shift-base),?){9}"}}
// RUN: %clang -fsanitize=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
// RUN: %clang -fsanitize=implicit-conversion -fsanitize-recover=implicit-conversion %s -### 2>&1 | FileCheck %s --check-prefixes=CHECK-implicit-conversion,CHECK-implicit-conversion-RECOVER
Index: clang/test/CodeGen/unsigned-shift-base.c
===================================================================
--- /dev/null
+++ clang/test/CodeGen/unsigned-shift-base.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsanitize=unsigned-shift-base,shift-exponent %s -emit-llvm -o - | FileCheck %s
+
+// CHECK-LABEL: lsh_overflow(
+unsigned lsh_overflow(unsigned a, unsigned b) {
+ // CHECK: %[[RHS_INBOUNDS:.*]] = icmp ule i32 %[[RHS:.*]], 31
+ // CHECK-NEXT: br i1 %[[RHS_INBOUNDS]], label %[[CHECK_BB:.*]], label %[[CONT_BB:.*]],
+
+ // CHECK: [[CHECK_BB]]:
+ // CHECK-NEXT: %[[SHIFTED_OUT_WIDTH:.*]] = sub nuw nsw i32 31, %[[RHS]]
+ // CHECK-NEXT: %[[SHIFTED_OUT:.*]] = lshr i32 %[[LHS:.*]], %[[SHIFTED_OUT_WIDTH]]
+
+ // CHECK-NEXT: %[[SHIFTED_OUT_NOT_SIGN:.*]] = lshr i32 %[[SHIFTED_OUT]], 1
+
+ // CHECK-NEXT: %[[NO_OVERFLOW:.*]] = icmp eq i32 %[[SHIFTED_OUT_NOT_SIGN]], 0
+ // CHECK-NEXT: br label %[[CONT_BB]]
+
+ // CHECK: [[CONT_BB]]:
+ // CHECK-NEXT: %[[VALID_BASE:.*]] = phi i1 [ true, {{.*}} ], [ %[[NO_OVERFLOW]], %[[CHECK_BB]] ]
+ // CHECK-NEXT: %[[VALID:.*]] = and i1 %[[RHS_INBOUNDS]], %[[VALID_BASE]]
+ // CHECK-NEXT: br i1 %[[VALID]]
+
+ // CHECK: call void @__ubsan_handle_shift_out_of_bounds
+ // CHECK-NOT: call void @__ubsan_handle_shift_out_of_bounds
+
+ // CHECK: %[[RET:.*]] = shl i32 %[[LHS]], %[[RHS]]
+ // CHECK-NEXT: ret i32 %[[RET]]
+ return a << b;
+}
Index: clang/lib/Driver/ToolChain.cpp
===================================================================
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -1016,14 +1016,14 @@
// Return sanitizers which don't require runtime support and are not
// platform dependent.
- SanitizerMask Res = (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
- ~SanitizerKind::Function) |
- (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
- SanitizerKind::CFICastStrict |
- SanitizerKind::FloatDivideByZero |
- SanitizerKind::UnsignedIntegerOverflow |
- SanitizerKind::ImplicitConversion |
- SanitizerKind::Nullability | SanitizerKind::LocalBounds;
+ SanitizerMask Res =
+ (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
+ ~SanitizerKind::Function) |
+ (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
+ SanitizerKind::CFICastStrict | SanitizerKind::FloatDivideByZero |
+ SanitizerKind::UnsignedIntegerOverflow |
+ SanitizerKind::UnsignedShiftBase | SanitizerKind::ImplicitConversion |
+ SanitizerKind::Nullability | SanitizerKind::LocalBounds;
if (getTriple().getArch() == llvm::Triple::x86 ||
getTriple().getArch() == llvm::Triple::x86_64 ||
getTriple().getArch() == llvm::Triple::arm || getTriple().isWasm() ||
Index: clang/lib/CodeGen/CGExprScalar.cpp
===================================================================
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3833,10 +3833,14 @@
if (Ops.LHS->getType() != RHS->getType())
RHS = Builder.CreateIntCast(RHS, Ops.LHS->getType(), false, "sh_prom");
- bool SanitizeBase = CGF.SanOpts.has(SanitizerKind::ShiftBase) &&
- Ops.Ty->hasSignedIntegerRepresentation() &&
- !CGF.getLangOpts().isSignedOverflowDefined() &&
- !CGF.getLangOpts().CPlusPlus20;
+ bool SanitizeSignedBase = CGF.SanOpts.has(SanitizerKind::ShiftBase) &&
+ Ops.Ty->hasSignedIntegerRepresentation() &&
+ !CGF.getLangOpts().isSignedOverflowDefined() &&
+ !CGF.getLangOpts().CPlusPlus20;
+ bool SanitizeUnsignedBase =
+ CGF.SanOpts.has(SanitizerKind::UnsignedShiftBase) &&
+ Ops.Ty->hasUnsignedIntegerRepresentation();
+ bool SanitizeBase = SanitizeSignedBase || SanitizeUnsignedBase;
bool SanitizeExponent = CGF.SanOpts.has(SanitizerKind::ShiftExponent);
// OpenCL 6.3j: shift values are effectively % word size of LHS.
if (CGF.getLangOpts().OpenCL)
@@ -3869,11 +3873,12 @@
Ops.LHS, Builder.CreateSub(PromotedWidthMinusOne, RHS, "shl.zeros",
/*NUW*/ true, /*NSW*/ true),
"shl.check");
- if (CGF.getLangOpts().CPlusPlus) {
+ if (SanitizeUnsignedBase || CGF.getLangOpts().CPlusPlus) {
// In C99, we are not permitted to shift a 1 bit into the sign bit.
// Under C++11's rules, shifting a 1 bit into the sign bit is
// OK, but shifting a 1 bit out of it is not. (C89 and C++03 don't
// define signed left shifts, so we use the C99 and C++11 rules there).
+ // Unsigned shifts can always shift into the top bit.
llvm::Value *One = llvm::ConstantInt::get(BitsShiftedOff->getType(), 1);
BitsShiftedOff = Builder.CreateLShr(BitsShiftedOff, One);
}
@@ -3883,7 +3888,9 @@
llvm::PHINode *BaseCheck = Builder.CreatePHI(ValidBase->getType(), 2);
BaseCheck->addIncoming(Builder.getTrue(), Orig);
BaseCheck->addIncoming(ValidBase, CheckShiftBase);
- Checks.push_back(std::make_pair(BaseCheck, SanitizerKind::ShiftBase));
+ Checks.push_back(std::make_pair(
+ BaseCheck, SanitizeSignedBase ? SanitizerKind::ShiftBase
+ : SanitizerKind::UnsignedShiftBase));
}
assert(!Checks.empty());
Index: clang/include/clang/Basic/Sanitizers.def
===================================================================
--- clang/include/clang/Basic/Sanitizers.def
+++ clang/include/clang/Basic/Sanitizers.def
@@ -107,6 +107,7 @@
// IntegerSanitizer
SANITIZER("unsigned-integer-overflow", UnsignedIntegerOverflow)
+SANITIZER("unsigned-shift-base", UnsignedShiftBase)
// DataFlowSanitizer
SANITIZER("dataflow", DataFlow)
@@ -171,7 +172,8 @@
SANITIZER_GROUP("integer", Integer,
ImplicitConversion | IntegerDivideByZero | Shift |
- SignedIntegerOverflow | UnsignedIntegerOverflow)
+ SignedIntegerOverflow | UnsignedIntegerOverflow |
+ UnsignedShiftBase)
SANITIZER("local-bounds", LocalBounds)
SANITIZER_GROUP("bounds", Bounds, ArrayBounds | LocalBounds)
Index: clang/docs/UndefinedBehaviorSanitizer.rst
===================================================================
--- clang/docs/UndefinedBehaviorSanitizer.rst
+++ clang/docs/UndefinedBehaviorSanitizer.rst
@@ -153,6 +153,8 @@
unsigned overflow in C++. You can use ``-fsanitize=shift-base`` or
``-fsanitize=shift-exponent`` to check only left-hand side or
right-hand side of shift operation, respectively.
+ - ``-fsanitize=unsigned-shift-base``: check that an unsigned left-hand side of
+ a left shift operation doesn't overflow.
- ``-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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
