From 2e7be2a65af2ffdd7ce378b54d16b9e3859ff3b5 Mon Sep 17 00:00:00 2001 From: Mitch Phillips Date: Wed, 15 Nov 2017 00:35:26 +0000 Subject: [PATCH] [cfi-verify] Validate there are no register clobbers between CFI-check and instruction execution. Summary: This patch adds another failure mode for `validateCFIProtection(..)`, wherein any register that affects the indirect control flow instruction is clobbered to between the CFI-check and the instruction's execution. Also includes a modification to make MCInstrDesc::hasDefOfPhysReg public. Reviewers: vlad.tsyrklevich Reviewed By: vlad.tsyrklevich Subscribers: llvm-commits, pcc, kcc Differential Revision: https://reviews.llvm.org/D39820 llvm-svn: 318238 --- llvm/include/llvm/MC/MCInstrDesc.h | 4 +- llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp | 45 +++++++++++++++ llvm/tools/llvm-cfi-verify/lib/FileAnalysis.h | 10 ++++ llvm/tools/llvm-cfi-verify/lib/GraphBuilder.cpp | 1 + llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h | 1 + .../tools/llvm-cfi-verify/FileAnalysis.cpp | 66 ++++++++++++++++++++++ 6 files changed, 124 insertions(+), 3 deletions(-) diff --git a/llvm/include/llvm/MC/MCInstrDesc.h b/llvm/include/llvm/MC/MCInstrDesc.h index 9150a8b..ff4c756 100644 --- a/llvm/include/llvm/MC/MCInstrDesc.h +++ b/llvm/include/llvm/MC/MCInstrDesc.h @@ -173,7 +173,7 @@ public: const MCPhysReg *ImplicitDefs; // Registers implicitly defined by this instr const MCOperandInfo *OpInfo; // 'NumOperands' entries about operands // Subtarget feature that this is deprecated on, if any - // -1 implies this is not deprecated by any single feature. It may still be + // -1 implies this is not deprecated by any single feature. It may still be // deprecated due to a "complex" reason, below. int64_t DeprecatedFeature; @@ -580,8 +580,6 @@ public: return -1; } -private: - /// \brief Return true if this instruction defines the specified physical /// register, either explicitly or implicitly. bool hasDefOfPhysReg(const MCInst &MI, unsigned Reg, diff --git a/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp b/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp index 8f3a683..464454c 100644 --- a/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp +++ b/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp @@ -64,6 +64,8 @@ StringRef stringCFIProtectionStatus(CFIProtectionStatus Status) { return "FAIL_ORPHANS"; case CFIProtectionStatus::FAIL_BAD_CONDITIONAL_BRANCH: return "FAIL_BAD_CONDITIONAL_BRANCH"; + case CFIProtectionStatus::FAIL_REGISTER_CLOBBERED: + return "FAIL_REGISTER_CLOBBERED"; case CFIProtectionStatus::FAIL_INVALID_INSTRUCTION: return "FAIL_INVALID_INSTRUCTION"; } @@ -270,9 +272,52 @@ FileAnalysis::validateCFIProtection(const GraphResult &Graph) const { return CFIProtectionStatus::FAIL_BAD_CONDITIONAL_BRANCH; } + if (indirectCFOperandClobber(Graph) != Graph.BaseAddress) + return CFIProtectionStatus::FAIL_REGISTER_CLOBBERED; + return CFIProtectionStatus::PROTECTED; } +uint64_t FileAnalysis::indirectCFOperandClobber(const GraphResult &Graph) const { + assert(Graph.OrphanedNodes.empty() && "Orphaned nodes should be empty."); + + // Get the set of registers we must check to ensure they're not clobbered. + const Instr &IndirectCF = getInstructionOrDie(Graph.BaseAddress); + DenseSet RegisterNumbers; + for (const auto &Operand : IndirectCF.Instruction) { + if (Operand.isReg()) + RegisterNumbers.insert(Operand.getReg()); + } + assert(RegisterNumbers.size() && "Zero register operands on indirect CF."); + + // Now check all branches to indirect CFs and ensure no clobbering happens. + for (const auto &Branch : Graph.ConditionalBranchNodes) { + uint64_t Node; + if (Branch.IndirectCFIsOnTargetPath) + Node = Branch.Target; + else + Node = Branch.Fallthrough; + + while (Node != Graph.BaseAddress) { + const Instr &NodeInstr = getInstructionOrDie(Node); + const auto &InstrDesc = MII->get(NodeInstr.Instruction.getOpcode()); + + for (unsigned RegNum : RegisterNumbers) { + if (InstrDesc.hasDefOfPhysReg(NodeInstr.Instruction, RegNum, + *RegisterInfo)) + return Node; + } + + const auto &KV = Graph.IntermediateNodes.find(Node); + assert((KV != Graph.IntermediateNodes.end()) && + "Could not get next node."); + Node = KV->second; + } + } + + return Graph.BaseAddress; +} + void FileAnalysis::printInstruction(const Instr &InstrMeta, raw_ostream &OS) const { Printer->printInst(&InstrMeta.Instruction, OS, "", *SubtargetInfo.get()); diff --git a/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.h b/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.h index 820c368..ce81f8b 100644 --- a/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.h +++ b/llvm/tools/llvm-cfi-verify/lib/FileAnalysis.h @@ -59,6 +59,9 @@ enum class CFIProtectionStatus { // There is a path to the instruction from a conditional branch that does not // properly check the destination for this vcall/icall. FAIL_BAD_CONDITIONAL_BRANCH, + // One of the operands of the indirect CF instruction is modified between the + // CFI-check and execution. + FAIL_REGISTER_CLOBBERED, // The instruction referenced does not exist. This normally indicates an // error in the program, where you try and validate a graph that was created // in a different FileAnalysis object. @@ -145,6 +148,13 @@ public: // flow instruction in this file. CFIProtectionStatus validateCFIProtection(const GraphResult &Graph) const; + // Returns the first place the operand register is clobbered between the CFI- + // check and the indirect CF instruction execution. If the register is not + // modified, returns the address of the indirect CF instruction. The result is + // undefined if the provided graph does not fall under either the + // FAIL_REGISTER_CLOBBERED or PROTECTED status (see CFIProtectionStatus). + uint64_t indirectCFOperandClobber(const GraphResult& Graph) const; + // Prints an instruction to the provided stream using this object's pretty- // printers. void printInstruction(const Instr &InstrMeta, raw_ostream &OS) const; diff --git a/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.cpp b/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.cpp index ff92171..65d4b99 100644 --- a/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.cpp +++ b/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.cpp @@ -301,6 +301,7 @@ void GraphBuilder::buildFlowGraphImpl(const FileAnalysis &Analysis, BranchNode.Target = 0; BranchNode.Fallthrough = 0; BranchNode.CFIProtection = false; + BranchNode.IndirectCFIsOnTargetPath = (BranchTarget == Address); if (BranchTarget == Address) BranchNode.Target = Address; diff --git a/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h b/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h index d1ce509..2bf07b5 100644 --- a/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h +++ b/llvm/tools/llvm-cfi-verify/lib/GraphBuilder.h @@ -59,6 +59,7 @@ struct ConditionalBranchNode { // is a CFI trap, and... // - The exit point of the other basic block is an undirect CF instruction. bool CFIProtection; + bool IndirectCFIsOnTargetPath; }; // The canonical graph result structure returned by GraphBuilder. The members diff --git a/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp b/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp index 3e8954f..27275d6 100644 --- a/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp +++ b/llvm/unittests/tools/llvm-cfi-verify/FileAnalysis.cpp @@ -741,6 +741,72 @@ TEST_F(BasicFileAnalysisTest, UndefSearchLengthOneTestFarAway) { SearchLengthForUndef = PrevSearchLengthForUndef; } +TEST_F(BasicFileAnalysisTest, CFIProtectionClobberSinglePathExplicit) { + if (!SuccessfullyInitialised) + return; + Analysis.parseSectionContents( + { + 0x75, 0x02, // 0: jne 4 [+2] + 0x0f, 0x0b, // 2: ud2 + 0x48, 0x05, 0x00, 0x00, 0x00, 0x00, // 4: add $0x0, %rax + 0xff, 0x10, // 10: callq *(%rax) + }, + 0xDEADBEEF); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 10); + EXPECT_EQ(CFIProtectionStatus::FAIL_REGISTER_CLOBBERED, + Analysis.validateCFIProtection(Result)); +} + +TEST_F(BasicFileAnalysisTest, CFIProtectionClobberSinglePathExplicit2) { + if (!SuccessfullyInitialised) + return; + Analysis.parseSectionContents( + { + 0x75, 0x02, // 0: jne 4 [+2] + 0x0f, 0x0b, // 2: ud2 + 0x48, 0x83, 0xc0, 0x00, // 4: add $0x0, %rax + 0xff, 0x10, // 8: callq *(%rax) + }, + 0xDEADBEEF); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 8); + EXPECT_EQ(CFIProtectionStatus::FAIL_REGISTER_CLOBBERED, + Analysis.validateCFIProtection(Result)); +} + +TEST_F(BasicFileAnalysisTest, CFIProtectionClobberSinglePathImplicit) { + if (!SuccessfullyInitialised) + return; + Analysis.parseSectionContents( + { + 0x75, 0x02, // 0: jne 4 [+2] + 0x0f, 0x0b, // 2: ud2 + 0x05, 0x00, 0x00, 0x00, 0x00, // 4: add $0x0, %eax + 0xff, 0x10, // 9: callq *(%rax) + }, + 0xDEADBEEF); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 9); + EXPECT_EQ(CFIProtectionStatus::FAIL_REGISTER_CLOBBERED, + Analysis.validateCFIProtection(Result)); +} + +TEST_F(BasicFileAnalysisTest, CFIProtectionClobberDualPathImplicit) { + if (!SuccessfullyInitialised) + return; + Analysis.parseSectionContents( + { + 0x75, 0x04, // 0: jne 6 [+4] + 0x0f, 0x31, // 2: rdtsc (note: affects eax) + 0xff, 0x10, // 4: callq *(%rax) + 0x0f, 0x0b, // 6: ud2 + 0x75, 0xf9, // 8: jne 2 [-7] + 0x0f, 0x0b, // 10: ud2 + }, + 0xDEADBEEF); + GraphResult Result = GraphBuilder::buildFlowGraph(Analysis, 0xDEADBEEF + 4); + EXPECT_EQ(CFIProtectionStatus::FAIL_REGISTER_CLOBBERED, + Analysis.validateCFIProtection(Result)); +} + } // anonymous namespace } // end namespace cfi_verify } // end namespace llvm -- 2.7.4