Index: test/tools/llvm-cfi-verify/X86/blacklist-expected-unprotected.s =================================================================== --- test/tools/llvm-cfi-verify/X86/blacklist-expected-unprotected.s +++ test/tools/llvm-cfi-verify/X86/blacklist-expected-unprotected.s @@ -5,7 +5,7 @@ # CHECK-LABEL: U # CHECK-NEXT: tiny.cc:11 -# CHECK-NEXT: BLACKLIST MATCH, 'src' +# CHECK-NEXT: {{^Blacklist Match:.*blacklist\.txt:1$}} # CHECK-NEXT: ====> Expected Unprotected # CHECK: Expected Protected: 0 (0.00%) Index: test/tools/llvm-cfi-verify/X86/blacklist-match-fun.s =================================================================== --- test/tools/llvm-cfi-verify/X86/blacklist-match-fun.s +++ test/tools/llvm-cfi-verify/X86/blacklist-match-fun.s @@ -5,7 +5,7 @@ # CHECK-LABEL: U # CHECK-NEXT: tiny.cc:11 -# CHECK-NEXT: BLACKLIST MATCH, 'fun' +# CHECK-NEXT: {{^Blacklist Match:.*blacklist\.txt:1$}} # CHECK-NEXT: ====> Expected Unprotected # CHECK: Expected Protected: 0 (0.00%) Index: test/tools/llvm-cfi-verify/X86/blacklist-unexpected-protected.s =================================================================== --- test/tools/llvm-cfi-verify/X86/blacklist-unexpected-protected.s +++ test/tools/llvm-cfi-verify/X86/blacklist-unexpected-protected.s @@ -5,7 +5,7 @@ # CHECK-LABEL: P # CHECK-NEXT: tiny.cc:11 -# CHECK-NEXT: BLACKLIST MATCH, 'src' +# CHECK-NEXT: {{^Blacklist Match:.*blacklist\.txt:1$}} # CHECK-NEXT: ====> Unexpected Protected # CHECK: Expected Protected: 0 (0.00%) Index: tools/llvm-cfi-verify/lib/FileAnalysis.h =================================================================== --- tools/llvm-cfi-verify/lib/FileAnalysis.h +++ tools/llvm-cfi-verify/lib/FileAnalysis.h @@ -44,8 +44,24 @@ namespace llvm { namespace cfi_verify { +struct GraphResult; + extern bool IgnoreDWARFFlag; +enum CFIProtectionStatus { + PROTECTED, // This instruction is protected by CFI. + FAIL_NOT_INDIRECT_CF, // The instruction is not an indirect control flow + // instruction, and thus shouldn't be protected. + FAIL_ORPHANS, // There is a path to the instruction that was unexpected. + FAIL_BAD_CONDITIONAL_BRANCH, // There is a path to the instruction from a + // conditional branch that does not properly + // check the destination for this vcall/icall. + FAIL_UNKNOWN, // Something strange happened, e.g. the instruction was not in + // this file. +}; + +StringRef stringCFIProtectionStatus(CFIProtectionStatus Status); + // Disassembler and analysis tool for machine code files. Keeps track of non- // sequential control flows, including indirect control flow instructions. class FileAnalysis { @@ -69,12 +85,6 @@ FileAnalysis(const FileAnalysis &) = delete; FileAnalysis(FileAnalysis &&Other) = default; - // Check whether the provided instruction is CFI protected in this file. - // Returns false if this instruction doesn't exist in this file, if it's not - // an indirect control flow instruction, or isn't CFI protected. Returns true - // otherwise. - bool isIndirectInstructionCFIProtected(uint64_t Address) const; - // Returns the instruction at the provided address. Returns nullptr if there // is no instruction at the provided address. const Instr *getInstruction(uint64_t Address) const; @@ -122,19 +132,13 @@ const MCRegisterInfo *getRegisterInfo() const; const MCInstrInfo *getMCInstrInfo() const; const MCInstrAnalysis *getMCInstrAnalysis() const; - symbolize::LLVMSymbolizer &getSymbolizer(); - - // Returns true if this class is using DWARF line tables for elimination. - bool hasLineTableInfo() const; - // Returns the line table information for the range {Address +- - // DWARFSearchRange}. Returns an empty table if the address has no valid line - // table information, or this analysis object has DWARF handling disabled. - DILineInfoTable getLineInfoForAddressRange(uint64_t Address); + // Returns the inlining information for the provided address. + Expected symbolizeInlinedCode(uint64_t Address); - // Returns whether the provided address has valid line information for - // instructions in the range of Address +- DWARFSearchRange. - bool hasValidLineInfoForAddressRange(uint64_t Address); + // Returns whether the provided Graph represents a protected indirect control + // flow instruction in this file. + CFIProtectionStatus validateCFIProtection(const GraphResult &Graph) const; protected: // Construct a blank object with the provided triple and features. Used in Index: tools/llvm-cfi-verify/lib/FileAnalysis.cpp =================================================================== --- tools/llvm-cfi-verify/lib/FileAnalysis.cpp +++ tools/llvm-cfi-verify/lib/FileAnalysis.cpp @@ -54,6 +54,22 @@ "will result in false positives for 'CFI unprotected' instructions."), cl::location(IgnoreDWARFFlag), cl::init(false)); +StringRef stringCFIProtectionStatus(CFIProtectionStatus Status) { + switch (Status) { + case PROTECTED: + return "PROTECTED"; + case FAIL_NOT_INDIRECT_CF: + return "FAIL_NOT_INDIRECT_CF"; + case FAIL_ORPHANS: + return "FAIL_ORPHANS"; + case FAIL_BAD_CONDITIONAL_BRANCH: + return "FAIL_BAD_CONDITIONAL_BRANCH"; + case FAIL_UNKNOWN: + return "FAIL_UNKNOWN"; + } + return "FAIL_UNKNOWN"; +} + Expected FileAnalysis::Create(StringRef Filename) { // Open the filename provided. Expected> BinaryOrErr = @@ -89,32 +105,6 @@ const SubtargetFeatures &Features) : ObjectTriple(ObjectTriple), Features(Features) {} -bool FileAnalysis::isIndirectInstructionCFIProtected(uint64_t Address) const { - const Instr *InstrMetaPtr = getInstruction(Address); - if (!InstrMetaPtr) - return false; - - const auto &InstrDesc = MII->get(InstrMetaPtr->Instruction.getOpcode()); - - if (!InstrDesc.mayAffectControlFlow(InstrMetaPtr->Instruction, *RegisterInfo)) - return false; - - if (!usesRegisterOperand(*InstrMetaPtr)) - return false; - - auto Flows = GraphBuilder::buildFlowGraph(*this, Address); - - if (!Flows.OrphanedNodes.empty()) - return false; - - for (const auto &BranchNode : Flows.ConditionalBranchNodes) { - if (!BranchNode.CFIProtection) - return false; - } - - return true; -} - const Instr * FileAnalysis::getPrevInstructionSequential(const Instr &InstrMeta) const { std::map::const_iterator KV = @@ -254,7 +244,34 @@ return MIA.get(); } -LLVMSymbolizer &FileAnalysis::getSymbolizer() { return *Symbolizer; } +Expected FileAnalysis::symbolizeInlinedCode(uint64_t Address) { + assert(Symbolizer != nullptr && "Symbolizer is invalid."); + return Symbolizer->symbolizeInlinedCode(Object->getFileName(), Address); +} + +CFIProtectionStatus +FileAnalysis::validateCFIProtection(const GraphResult &Graph) const { + const Instr *InstrMetaPtr = getInstruction(Graph.BaseAddress); + if (!InstrMetaPtr) + return FAIL_UNKNOWN; + + const auto &InstrDesc = MII->get(InstrMetaPtr->Instruction.getOpcode()); + if (!InstrDesc.mayAffectControlFlow(InstrMetaPtr->Instruction, *RegisterInfo)) + return FAIL_NOT_INDIRECT_CF; + + if (!usesRegisterOperand(*InstrMetaPtr)) + return FAIL_NOT_INDIRECT_CF; + + if (!Graph.OrphanedNodes.empty()) + return FAIL_ORPHANS; + + for (const auto &BranchNode : Graph.ConditionalBranchNodes) { + if (!BranchNode.CFIProtection) + return FAIL_BAD_CONDITIONAL_BRANCH; + } + + return PROTECTED; +} Error FileAnalysis::initialiseDisassemblyMembers() { std::string TripleName = ObjectTriple.getTriple(); @@ -370,21 +387,6 @@ InstrMeta.InstructionSize = InstructionSize; InstrMeta.Valid = ValidInstruction; - // Check if this instruction exists in the range of the DWARF metadata. - if (!IgnoreDWARFFlag) { - auto LineInfo = - Symbolizer->symbolizeCode(Object->getFileName(), VMAddress); - if (!LineInfo) { - handleAllErrors(LineInfo.takeError(), [](const ErrorInfoBase &E) { - errs() << "Symbolizer failed to get line: " << E.message() << "\n"; - }); - continue; - } - - if (LineInfo->FileName == "") - continue; - } - addInstruction(InstrMeta); if (!ValidInstruction) @@ -406,6 +408,21 @@ if (!usesRegisterOperand(InstrMeta)) continue; + // Check if this instruction exists in the range of the DWARF metadata. + if (!IgnoreDWARFFlag) { + auto LineInfo = + Symbolizer->symbolizeCode(Object->getFileName(), VMAddress); + if (!LineInfo) { + handleAllErrors(LineInfo.takeError(), [](const ErrorInfoBase &E) { + errs() << "Symbolizer failed to get line: " << E.message() << "\n"; + }); + continue; + } + + if (LineInfo->FileName == "") + continue; + } + IndirectInstructions.insert(VMAddress); } } Index: tools/llvm-cfi-verify/llvm-cfi-verify.cpp =================================================================== --- tools/llvm-cfi-verify/llvm-cfi-verify.cpp +++ tools/llvm-cfi-verify/llvm-cfi-verify.cpp @@ -18,6 +18,7 @@ //===----------------------------------------------------------------------===// #include "lib/FileAnalysis.h" +#include "lib/GraphBuilder.h" #include "llvm/BinaryFormat/ELF.h" #include "llvm/Support/CommandLine.h" @@ -46,12 +47,15 @@ uint64_t ExpectedUnprotected = 0; uint64_t UnexpectedUnprotected = 0; - symbolize::LLVMSymbolizer &Symbolizer = Analysis.getSymbolizer(); + std::map BlameCounter; for (uint64_t Address : Analysis.getIndirectInstructions()) { const auto &InstrMeta = Analysis.getInstructionOrDie(Address); + GraphResult Graph = GraphBuilder::buildFlowGraph(Analysis, Address); - bool CFIProtected = Analysis.isIndirectInstructionCFIProtected(Address); + CFIProtectionStatus ProtectionStatus = + Analysis.validateCFIProtection(Graph); + bool CFIProtected = (ProtectionStatus == PROTECTED); if (CFIProtected) outs() << "P "; @@ -71,7 +75,7 @@ continue; } - auto InliningInfo = Symbolizer.symbolizeInlinedCode(InputFilename, Address); + auto InliningInfo = Analysis.symbolizeInlinedCode(Address); if (!InliningInfo || InliningInfo->getNumberOfFrames() == 0) { errs() << "Failed to symbolise " << format_hex(Address, 2) << " with line tables from " << InputFilename << "\n"; @@ -97,20 +101,20 @@ continue; } - bool MatchesBlacklistRule = false; - if (SpecialCaseList->inSection("cfi-icall", "src", LineInfo.FileName) || - SpecialCaseList->inSection("cfi-vcall", "src", LineInfo.FileName)) { - outs() << "BLACKLIST MATCH, 'src'\n"; - MatchesBlacklistRule = true; - } - - if (SpecialCaseList->inSection("cfi-icall", "fun", LineInfo.FunctionName) || - SpecialCaseList->inSection("cfi-vcall", "fun", LineInfo.FunctionName)) { - outs() << "BLACKLIST MATCH, 'fun'\n"; - MatchesBlacklistRule = true; + unsigned BlameLine = 0; + for (auto &K : {"cfi-icall", "cfi-vcall"}) { + for (auto &V : + {std::make_pair("src", StringRef(LineInfo.FileName)), + std::make_pair("fun", StringRef(LineInfo.FunctionName))}) { + if (!BlameLine) + BlameLine = SpecialCaseList->inSectionBlame(K, V.first, V.second); + } } - if (MatchesBlacklistRule) { + if (BlameLine) { + outs() << "Blacklist Match: " << BlacklistFilename << ":" << BlameLine + << "\n"; + BlameCounter[BlameLine]++; if (CFIProtected) { UnexpectedProtected++; outs() << "====> Unexpected Protected\n"; @@ -149,6 +153,15 @@ ((double)ExpectedUnprotected) / IndirectCFInstructions, UnexpectedUnprotected, ((double)UnexpectedUnprotected) / IndirectCFInstructions); + + if (!SpecialCaseList) + return; + + outs() << "Blacklist Results:\n"; + for (const auto &KV : BlameCounter) { + outs() << " " << BlacklistFilename << ":" << KV.first << " affects " + << KV.second << " indirect CF instructions.\n"; + } } int main(int argc, char **argv) { Index: unittests/tools/llvm-cfi-verify/FileAnalysis.cpp =================================================================== --- unittests/tools/llvm-cfi-verify/FileAnalysis.cpp +++ unittests/tools/llvm-cfi-verify/FileAnalysis.cpp @@ -493,10 +493,14 @@ 0x75, 0x00, // 3: jne 5 [+0] }, 0xDEADBEEF); - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF)); - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 1)); - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 3)); - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADC0DE)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF); + EXPECT_EQ(FAIL_NOT_INDIRECT_CF, Analysis.validateCFIProtection(Result)); + Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 1); + EXPECT_EQ(FAIL_NOT_INDIRECT_CF, Analysis.validateCFIProtection(Result)); + Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 3); + EXPECT_EQ(FAIL_NOT_INDIRECT_CF, Analysis.validateCFIProtection(Result)); + Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADC0DE); + EXPECT_EQ(FAIL_UNKNOWN, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionBasicFallthroughToUd2) { @@ -509,7 +513,8 @@ 0xff, 0x10, // 4: callq *(%rax) }, 0xDEADBEEF); - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 4)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 4); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionBasicJumpToUd2) { @@ -522,7 +527,8 @@ 0x0f, 0x0b, // 4: ud2 }, 0xDEADBEEF); - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 2)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 2); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionDualPathUd2) { @@ -538,7 +544,8 @@ 0x0f, 0x0b, // 9: ud2 }, 0xDEADBEEF); - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 3)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 3); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionDualPathSingleUd2) { @@ -553,7 +560,8 @@ 0x0f, 0x0b, // 7: ud2 }, 0xDEADBEEF); - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 3)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 3); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionDualFailLimitUpwards) { @@ -574,7 +582,8 @@ SearchLengthForConditionalBranch; SearchLengthForConditionalBranch = 2; - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 6)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 6); + EXPECT_EQ(FAIL_ORPHANS, Analysis.validateCFIProtection(Result)); SearchLengthForConditionalBranch = PrevSearchLengthForConditionalBranch; } @@ -596,7 +605,8 @@ uint64_t PrevSearchLengthForUndef = SearchLengthForUndef; SearchLengthForUndef = 2; - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 2)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 2); + EXPECT_EQ(FAIL_BAD_CONDITIONAL_BRANCH, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = PrevSearchLengthForUndef; } @@ -612,7 +622,8 @@ 0x0f, 0x0b, // 6: ud2 }, 0xDEADBEEF); - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 4)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 4); + EXPECT_EQ(FAIL_ORPHANS, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionWithUnconditionalJumpInFallthrough) { @@ -626,7 +637,8 @@ 0x0f, 0x0b, // 6: ud2 }, 0xDEADBEEF); - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 4)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 4); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); } TEST_F(BasicFileAnalysisTest, CFIProtectionComplexExample) { @@ -653,7 +665,8 @@ 0xDEADBEEF); uint64_t PrevSearchLengthForUndef = SearchLengthForUndef; SearchLengthForUndef = 5; - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0xDEADBEEF + 9)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 9); + EXPECT_EQ(FAIL_ORPHANS, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = PrevSearchLengthForUndef; } @@ -670,7 +683,8 @@ 0x688118); uint64_t PrevSearchLengthForUndef = SearchLengthForUndef; SearchLengthForUndef = 1; - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0x68811d)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0x68811d); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = PrevSearchLengthForUndef; } @@ -699,11 +713,14 @@ 0x775e0e); uint64_t PrevSearchLengthForUndef = SearchLengthForUndef; SearchLengthForUndef = 1; - EXPECT_FALSE(Analysis.isIndirectInstructionCFIProtected(0x775a68)); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0x775a68); + EXPECT_EQ(FAIL_BAD_CONDITIONAL_BRANCH, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = 2; - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0x775a68)); + Result = GraphBuilder::buildFlowGraph(Analysis, 0x775a68); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = 3; - EXPECT_TRUE(Analysis.isIndirectInstructionCFIProtected(0x775a68)); + Result = GraphBuilder::buildFlowGraph(Analysis, 0x775a68); + EXPECT_EQ(PROTECTED, Analysis.validateCFIProtection(Result)); SearchLengthForUndef = PrevSearchLengthForUndef; }