From 52c4fcf58da932fec911f275c1d4591dca2f69fc Mon Sep 17 00:00:00 2001 From: Kostya Serebryany Date: Thu, 16 Oct 2014 20:54:52 +0000 Subject: [PATCH] Insert poisoned paddings between fields in C++ classes so that AddressSanitizer can find intra-object-overflow bugs Summary: The general approach is to add extra paddings after every field in AST/RecordLayoutBuilder.cpp, then add code to CTORs/DTORs that poisons the paddings (CodeGen/CGClass.cpp). Everything is done under the flag -fsanitize-address-field-padding. The blacklist file (-fsanitize-blacklist) allows to avoid the transformation for given classes or source files. See also https://code.google.com/p/address-sanitizer/wiki/IntraObjectOverflow Test Plan: run SPEC2006 and some of the Chromium tests with -fsanitize-address-field-padding Reviewers: samsonov, rnk, rsmith Reviewed By: rsmith Subscribers: majnemer, cfe-commits Differential Revision: http://reviews.llvm.org/D5687 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@219961 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 5 + .../clang/Basic/DiagnosticFrontendKinds.td | 10 ++ include/clang/Basic/DiagnosticGroups.td | 3 + include/clang/Basic/SanitizerBlacklist.h | 3 +- lib/AST/Decl.cpp | 44 +++++ lib/AST/RecordLayoutBuilder.cpp | 19 ++- lib/Basic/SanitizerBlacklist.cpp | 5 +- lib/CodeGen/CGClass.cpp | 74 ++++++++- lib/CodeGen/CodeGenFunction.h | 1 + .../sanitize-address-field-padding.cpp | 152 ++++++++++++++++++ 10 files changed, 308 insertions(+), 8 deletions(-) create mode 100644 test/CodeGen/sanitize-address-field-padding.cpp diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index 48e9ed2ccc..e2f3bdaffd 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -3263,6 +3263,11 @@ public: /// commandline option. bool isMsStruct(const ASTContext &C) const; + /// \brief Whether we are allowed to insert extra padding between fields. + /// These padding are added to help AddressSanitizer detect + /// intra-object-overflow bugs. + bool mayInsertExtraPadding(bool EmitRemark = false) const; + private: /// \brief Deserialize just the fields. void LoadFieldsFromExternalStorage() const; diff --git a/include/clang/Basic/DiagnosticFrontendKinds.td b/include/clang/Basic/DiagnosticFrontendKinds.td index fa12eee399..d58552b345 100644 --- a/include/clang/Basic/DiagnosticFrontendKinds.td +++ b/include/clang/Basic/DiagnosticFrontendKinds.td @@ -46,6 +46,16 @@ def warn_fe_backend_optimization_failure : Warning<"%0">, BackendInfo, def note_fe_backend_optimization_remark_invalid_loc : Note<"could " "not determine the original source location for %0:%1:%2">; +def remark_sanitize_address_insert_extra_padding_accepted : Remark< + "-fsanitize-address-field-padding applied to %0">, ShowInSystemHeader, + InGroup; +def remark_sanitize_address_insert_extra_padding_rejected : Remark< + "-fsanitize-address-field-padding ignored for %0 because it " + "%select{is not C++|is packed|is a union|is trivially copyable|" + "has trivial destructor|is standard layout|is in a blacklisted file|" + "is blacklisted}1">, ShowInSystemHeader, + InGroup; + def err_fe_invalid_code_complete_file : Error< "cannot locate code-completion file %0">, DefaultFatal; def err_fe_stdout_binary : Error<"unable to change standard output to binary">, diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 78908e2846..0095590093 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -727,6 +727,9 @@ def BackendOptimizationFailure : DiagGroup<"pass-failed">; def ProfileInstrOutOfDate : DiagGroup<"profile-instr-out-of-date">; def ProfileInstrUnprofiled : DiagGroup<"profile-instr-unprofiled">; +// AddressSanitizer frontent instrumentation remarks. +def SanitizeAddressRemarks : DiagGroup<"sanitize-address">; + // A warning group for warnings about code that clang accepts when // compiling CUDA C/C++ but which is not compatible with the CUDA spec. def CudaCompat : DiagGroup<"cuda-compat">; diff --git a/include/clang/Basic/SanitizerBlacklist.h b/include/clang/Basic/SanitizerBlacklist.h index 7ea778e02c..8fa4357b75 100644 --- a/include/clang/Basic/SanitizerBlacklist.h +++ b/include/clang/Basic/SanitizerBlacklist.h @@ -35,7 +35,8 @@ public: bool isIn(const llvm::Function &F) const; bool isIn(const llvm::GlobalVariable &G, StringRef Category = StringRef()) const; - bool isBlacklistedType(StringRef MangledTypeName) const; + bool isBlacklistedType(StringRef MangledTypeName, + StringRef Category = StringRef()) const; bool isBlacklistedFunction(StringRef FunctionName) const; bool isBlacklistedFile(StringRef FileName, StringRef Category = StringRef()) const; diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index e3e365ff0f..c01272ec13 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -29,6 +29,7 @@ #include "clang/Basic/Module.h" #include "clang/Basic/Specifiers.h" #include "clang/Basic/TargetInfo.h" +#include "clang/Frontend/FrontendDiagnostic.h" #include "llvm/Support/ErrorHandling.h" #include @@ -3615,6 +3616,49 @@ void RecordDecl::LoadFieldsFromExternalStorage() const { /*FieldsAlreadyLoaded=*/false); } +bool RecordDecl::mayInsertExtraPadding(bool EmitRemark) const { + ASTContext &Context = getASTContext(); + if (!Context.getLangOpts().Sanitize.Address || + !Context.getLangOpts().Sanitize.SanitizeAddressFieldPadding) + return false; + auto &Blacklist = Context.getSanitizerBlacklist(); + const CXXRecordDecl *CXXRD = dyn_cast(this); + StringRef Filename = Context.getSourceManager().getFilename(getLocation()); + // We may be able to relax some of these requirements. + int ReasonToReject = -1; + if (!CXXRD || CXXRD->isExternCContext()) + ReasonToReject = 0; // is not C++. + else if (CXXRD->hasAttr()) + ReasonToReject = 1; // is packed. + else if (CXXRD->isUnion()) + ReasonToReject = 2; // is a union. + else if (CXXRD->isTriviallyCopyable()) + ReasonToReject = 3; // is trivially copyable. + else if (CXXRD->hasTrivialDestructor()) + ReasonToReject = 4; // has trivial destructor. + else if (CXXRD->isStandardLayout()) + ReasonToReject = 5; // is standard layout. + else if (Blacklist.isBlacklistedFile(Filename, "field-padding")) + ReasonToReject = 6; // is in a blacklisted file. + else if (Blacklist.isBlacklistedType(getQualifiedNameAsString(), + "field-padding")) + ReasonToReject = 7; // is blacklisted. + + if (EmitRemark) { + if (ReasonToReject >= 0) + Context.getDiagnostics().Report( + getLocation(), + diag::remark_sanitize_address_insert_extra_padding_rejected) + << getQualifiedNameAsString() << ReasonToReject; + else + Context.getDiagnostics().Report( + getLocation(), + diag::remark_sanitize_address_insert_extra_padding_accepted) + << getQualifiedNameAsString(); + } + return ReasonToReject < 0; +} + //===----------------------------------------------------------------------===// // BlockDecl Implementation //===----------------------------------------------------------------------===// diff --git a/lib/AST/RecordLayoutBuilder.cpp b/lib/AST/RecordLayoutBuilder.cpp index be326320d0..323d3d87e0 100644 --- a/lib/AST/RecordLayoutBuilder.cpp +++ b/lib/AST/RecordLayoutBuilder.cpp @@ -652,7 +652,7 @@ protected: void Layout(const ObjCInterfaceDecl *D); void LayoutFields(const RecordDecl *D); - void LayoutField(const FieldDecl *D); + void LayoutField(const FieldDecl *D, bool InsertExtraPadding); void LayoutWideBitField(uint64_t FieldSize, uint64_t TypeSize, bool FieldPacked, const FieldDecl *D); void LayoutBitField(const FieldDecl *D); @@ -1331,7 +1331,7 @@ void RecordLayoutBuilder::Layout(const ObjCInterfaceDecl *D) { // Layout each ivar sequentially. for (const ObjCIvarDecl *IVD = D->all_declared_ivar_begin(); IVD; IVD = IVD->getNextIvar()) - LayoutField(IVD); + LayoutField(IVD, false); // Finally, round the size of the total struct up to the alignment of the // struct itself. @@ -1341,8 +1341,9 @@ void RecordLayoutBuilder::Layout(const ObjCInterfaceDecl *D) { void RecordLayoutBuilder::LayoutFields(const RecordDecl *D) { // Layout each field, for now, just sequentially, respecting alignment. In // the future, this will need to be tweakable by targets. + bool InsertExtraPadding = D->mayInsertExtraPadding(/*EmitRemark=*/true); for (const auto *Field : D->fields()) - LayoutField(Field); + LayoutField(Field, InsertExtraPadding); } void RecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize, @@ -1645,7 +1646,8 @@ void RecordLayoutBuilder::LayoutBitField(const FieldDecl *D) { Context.toCharUnitsFromBits(UnpackedFieldAlign)); } -void RecordLayoutBuilder::LayoutField(const FieldDecl *D) { +void RecordLayoutBuilder::LayoutField(const FieldDecl *D, + bool InsertExtraPadding) { if (D->isBitField()) { LayoutBitField(D); return; @@ -1749,6 +1751,15 @@ void RecordLayoutBuilder::LayoutField(const FieldDecl *D) { Context.toBits(UnpackedFieldOffset), Context.toBits(UnpackedFieldAlign), FieldPacked, D); + if (InsertExtraPadding && !FieldSize.isZero()) { + CharUnits ASanAlignment = CharUnits::fromQuantity(8); + CharUnits ExtraSizeForAsan = ASanAlignment; + if (FieldSize % ASanAlignment) + ExtraSizeForAsan += + ASanAlignment - CharUnits::fromQuantity(FieldSize % ASanAlignment); + FieldSize += ExtraSizeForAsan; + } + // Reserve space for this field. uint64_t FieldSizeInBits = Context.toBits(FieldSize); if (IsUnion) diff --git a/lib/Basic/SanitizerBlacklist.cpp b/lib/Basic/SanitizerBlacklist.cpp index 7627bd0299..84ec2100b6 100644 --- a/lib/Basic/SanitizerBlacklist.cpp +++ b/lib/Basic/SanitizerBlacklist.cpp @@ -44,8 +44,9 @@ bool SanitizerBlacklist::isIn(const llvm::GlobalVariable &G, SCL->inSection("type", GetGlobalTypeString(G), Category); } -bool SanitizerBlacklist::isBlacklistedType(StringRef MangledTypeName) const { - return SCL->inSection("type", MangledTypeName); +bool SanitizerBlacklist::isBlacklistedType(StringRef MangledTypeName, + StringRef Category) const { + return SCL->inSection("type", MangledTypeName, Category); } bool SanitizerBlacklist::isBlacklistedFunction(StringRef FunctionName) const { diff --git a/lib/CodeGen/CGClass.cpp b/lib/CodeGen/CGClass.cpp index 1bba4a70a1..d31d50bb4b 100644 --- a/lib/CodeGen/CGClass.cpp +++ b/lib/CodeGen/CGClass.cpp @@ -702,8 +702,76 @@ static bool IsConstructorDelegationValid(const CXXConstructorDecl *Ctor) { return true; } +// Emit code in ctor (Prologue==true) or dtor (Prologue==false) +// to poison the extra field paddings inserted under +// -fsanitize-address-field-padding=1|2. +void CodeGenFunction::EmitAsanPrologueOrEpilogue(bool Prologue) { + ASTContext &Context = getContext(); + const CXXRecordDecl *ClassDecl = + Prologue ? cast(CurGD.getDecl())->getParent() + : cast(CurGD.getDecl())->getParent(); + if (!ClassDecl->mayInsertExtraPadding()) return; + + struct SizeAndOffset { + uint64_t Size; + uint64_t Offset; + }; + + unsigned PtrSize = CGM.getDataLayout().getPointerSizeInBits(); + const ASTRecordLayout &Info = Context.getASTRecordLayout(ClassDecl); + + // Populate sizes and offsets of fields. + SmallVector SSV(Info.getFieldCount()); + for (unsigned i = 0, e = Info.getFieldCount(); i != e; ++i) + SSV[i].Offset = + Context.toCharUnitsFromBits(Info.getFieldOffset(i)).getQuantity(); + + size_t NumFields = 0; + for (const auto *Field : ClassDecl->fields()) { + const FieldDecl *D = Field; + std::pair FieldInfo = + Context.getTypeInfoInChars(D->getType()); + CharUnits FieldSize = FieldInfo.first; + assert(NumFields < SSV.size()); + SSV[NumFields].Size = D->isBitField() ? 0 : FieldSize.getQuantity(); + NumFields++; + } + assert(NumFields == SSV.size()); + if (SSV.size() <= 1) return; + + // We will insert calls to __asan_* run-time functions. + // LLVM AddressSanitizer pass may decide to inline them later. + llvm::Type *Args[2] = {IntPtrTy, IntPtrTy}; + llvm::FunctionType *FTy = + llvm::FunctionType::get(CGM.VoidTy, Args, false); + llvm::Constant *F = CGM.CreateRuntimeFunction( + FTy, Prologue ? "__asan_poison_intra_object_redzone" + : "__asan_unpoison_intra_object_redzone"); + + llvm::Value *ThisPtr = LoadCXXThis(); + ThisPtr = Builder.CreatePtrToInt(ThisPtr, IntPtrTy); + QualType RecordTy = Context.getTypeDeclType(ClassDecl); + uint64_t TypeSize = Context.getTypeSizeInChars(RecordTy).getQuantity(); + + // For each field check if it has sufficient padding, + // if so (un)poison it with a call. + for (size_t i = 0; i < SSV.size(); i++) { + uint64_t AsanAlignment = 8; + uint64_t NextField = i == SSV.size() - 1 ? TypeSize : SSV[i + 1].Offset; + uint64_t PoisonSize = NextField - SSV[i].Offset - SSV[i].Size; + uint64_t EndOffset = SSV[i].Offset + SSV[i].Size; + if (PoisonSize < AsanAlignment || !SSV[i].Size || + (NextField % AsanAlignment) != 0) + continue; + Builder.CreateCall2( + F, Builder.CreateAdd(ThisPtr, Builder.getIntN(PtrSize, EndOffset)), + Builder.getIntN(PtrSize, PoisonSize)); + } +} + /// EmitConstructorBody - Emits the body of the current constructor. void CodeGenFunction::EmitConstructorBody(FunctionArgList &Args) { + EmitAsanPrologueOrEpilogue(true); const CXXConstructorDecl *Ctor = cast(CurGD.getDecl()); CXXCtorType CtorType = CurGD.getCtorType(); @@ -792,7 +860,10 @@ namespace { FirstField(nullptr), LastField(nullptr), FirstFieldOffset(0), LastFieldOffset(0), LastAddedFieldIndex(0) {} - static bool isMemcpyableField(FieldDecl *F) { + bool isMemcpyableField(FieldDecl *F) const { + // Never memcpy fields when we are adding poisoned paddings. + if (CGF.getContext().getLangOpts().Sanitize.SanitizeAddressFieldPadding) + return false; Qualifiers Qual = F->getType().getQualifiers(); if (Qual.hasVolatile() || Qual.hasObjCLifetime()) return false; @@ -1304,6 +1375,7 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) { bool isTryBody = (Body && isa(Body)); if (isTryBody) EnterCXXTryStmt(*cast(Body), true); + EmitAsanPrologueOrEpilogue(false); // Enter the epilogue cleanups. RunCleanupsScope DtorEpilogue(*this); diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index 09b12713a8..49810a8700 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -1267,6 +1267,7 @@ public: void EmitLambdaBlockInvokeBody(); void EmitLambdaDelegatingInvokeBody(const CXXMethodDecl *MD); void EmitLambdaStaticInvokeFunction(const CXXMethodDecl *MD); + void EmitAsanPrologueOrEpilogue(bool Prologue); /// EmitReturnBlock - Emit the unified return block, trying to avoid its /// emission when possible. diff --git a/test/CodeGen/sanitize-address-field-padding.cpp b/test/CodeGen/sanitize-address-field-padding.cpp new file mode 100644 index 0000000000..f02268dff8 --- /dev/null +++ b/test/CodeGen/sanitize-address-field-padding.cpp @@ -0,0 +1,152 @@ +// Test -fsanitize-address-field-padding +// RUN: echo 'type:SomeNamespace::BlacklistedByName=field-padding' > %t.type.blacklist +// RUN: echo 'src:*sanitize-address-field-padding.cpp=field-padding' > %t.file.blacklist +// RUN: %clang_cc1 -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.type.blacklist -Rsanitize-address -emit-llvm -o - %s 2>&1 | FileCheck %s +// RUN: %clang_cc1 -fsanitize=address -fsanitize-address-field-padding=1 -fsanitize-blacklist=%t.file.blacklist -Rsanitize-address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=FILE_BLACKLIST +// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - %s 2>&1 | FileCheck %s --check-prefix=NO_PADDING +// REQUIRES: shell +// + +// The reasons to ignore a particular class are not set in stone and will change. +// +// CHECK: -fsanitize-address-field-padding applied to Positive1 +// CHECK: -fsanitize-address-field-padding ignored for Negative1 because it is trivially copyable +// CHECK: -fsanitize-address-field-padding ignored for Negative2 because it is trivially copyable +// CHECK: -fsanitize-address-field-padding ignored for Negative3 because it is a union +// CHECK: -fsanitize-address-field-padding ignored for Negative4 because it is trivially copyable +// CHECK: -fsanitize-address-field-padding ignored for Negative5 because it is packed +// CHECK: -fsanitize-address-field-padding ignored for SomeNamespace::BlacklistedByName because it is blacklisted +// CHECK: -fsanitize-address-field-padding ignored for ExternCStruct because it is not C++ +// +// FILE_BLACKLIST: -fsanitize-address-field-padding ignored for Positive1 because it is in a blacklisted file +// FILE_BLACKLIST-NOT: __asan_poison_intra_object_redzone +// NO_PADDING-NOT: __asan_poison_intra_object_redzone + + +class Positive1 { + public: + Positive1() {} + ~Positive1() {} + int make_it_non_standard_layout; + private: + char private1; + int private2; + short private_array[6]; + long long private3; +}; + +Positive1 positive1; +// Positive1 with extra paddings +// CHECK: type { i32, [12 x i8], i8, [15 x i8], i32, [12 x i8], [6 x i16], [12 x i8], i64, [8 x i8] } + +class Negative1 { + public: + Negative1() {} + int public1, public2; +}; +Negative1 negative1; +// CHECK: type { i32, i32 } + +class Negative2 { + public: + Negative2() {} + private: + int private1, private2; +}; +Negative2 negative2; +// CHECK: type { i32, i32 } + +union Negative3 { + char m1[8]; + long long m2; +}; + +Negative3 negative3; +// CHECK: type { i64 } + +class Negative4 { + public: + Negative4() {} + // No DTOR + int make_it_non_standard_layout; + private: + char private1; + int private2; +}; + +Negative4 negative4; +// CHECK: type { i32, i8, i32 } + +class __attribute__((packed)) Negative5 { + public: + Negative5() {} + ~Negative5() {} + int make_it_non_standard_layout; + private: + char private1; + int private2; +}; + +Negative5 negative5; +// CHECK: type <{ i32, i8, i32 }> + + +namespace SomeNamespace { +class BlacklistedByName { + public: + BlacklistedByName() {} + ~BlacklistedByName() {} + int make_it_non_standard_layout; + private: + char private1; + int private2; +}; +} // SomeNamespace + +SomeNamespace::BlacklistedByName blacklisted_by_name; + +extern "C" { +class ExternCStruct { + public: + ExternCStruct() {} + ~ExternCStruct() {} + int make_it_non_standard_layout; + private: + char private1; + int private2; +}; +} // extern "C" + +ExternCStruct extern_C_struct; + +// CTOR +// CHECK-LABEL: define linkonce_odr void @_ZN9Positive1C1Ev +// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 15) +// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_poison_intra_object_redzone({{.*}}, i64 8) +// CHECK-NOT: __asan_poison_intra_object_redzone +// CHECK: ret void +// DTOR +// CHECK-LABEL: define linkonce_odr void @_ZN9Positive1D1Ev +// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 15) +// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 12) +// CHECK: call void @__asan_unpoison_intra_object_redzone({{.*}}, i64 8) +// CHECK-NOT: __asan_unpoison_intra_object_redzone +// CHECK: ret void +// +// CHECK-LABEL: define linkonce_odr void @_ZN9Negative1C1Ev +// CHECK-NOT: call void @__asan_poison_intra_object_redzone +// CHECK: ret void +// +// CHECK-LABEL: define linkonce_odr void @_ZN9Negative2C1Ev +// CHECK-NOT: call void @__asan_poison_intra_object_redzone +// CHECK: ret void +// +// CHECK-LABEL: define linkonce_odr void @_ZN9Negative4C1Ev +// CHECK-NOT: call void @__asan_poison_intra_object_redzone +// CHECK: ret void + -- 2.40.0