merge main into amd-staging (llvm#2826)

Author: z1-cciauto

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -82,6 +82,22 @@ namespace PAuthGadgetScanner {
   dbgs() << "\n";
 }
 
+// Iterates over BinaryFunction's instructions like a range-based for loop:
+//
+// iterateOverInstrs(BF, [&](MCInstReference Inst) {
+//   // loop body
+// });
+template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
+  if (BF.hasCFG()) {
+    for (BinaryBasicBlock &BB : BF)
+      for (int64_t I = 0, E = BB.size(); I < E; ++I)
+        Fn(MCInstInBBReference(&BB, I));
+  } else {
+    for (auto I : BF.instrs())
+      Fn(MCInstInBFReference(&BF, I.first));
+  }
+}
+
 // This class represents mapping from a set of arbitrary physical registers to
 // consecutive array indexes.
 class TrackedRegisters {
@@ -342,6 +358,29 @@ class SrcSafetyAnalysis {
     return S;
   }
 
+  /// Computes a reasonably pessimistic estimation of the register state when
+  /// the previous instruction is not known for sure. Takes the set of registers
+  /// which are trusted at function entry and removes all registers that can be
+  /// clobbered inside this function.
+  SrcState computePessimisticState(BinaryFunction &BF) {
+    BitVector ClobberedRegs(NumRegs);
+    iterateOverInstrs(BF, [&](MCInstReference Inst) {
+      BC.MIB->getClobberedRegs(Inst, ClobberedRegs);
+
+      // If this is a call instruction, no register is safe anymore, unless
+      // it is a tail call. Ignore tail calls for the purpose of estimating the
+      // worst-case scenario, assuming no instructions are executed in the
+      // caller after this point anyway.
+      if (BC.MIB->isCall(Inst) && !BC.MIB->isTailCall(Inst))
+        ClobberedRegs.set();
+    });
+
+    SrcState S = createEntryState();
+    S.SafeToDerefRegs.reset(ClobberedRegs);
+    S.TrustedRegs.reset(ClobberedRegs);
+    return S;
+  }
+
   BitVector getClobberedRegs(const MCInst &Point) const {
     BitVector Clobbered(NumRegs);
     // Assume a call can clobber all registers, including callee-saved
@@ -545,6 +584,10 @@ class DataflowSrcSafetyAnalysis
   using SrcSafetyAnalysis::BC;
   using SrcSafetyAnalysis::computeNext;
 
+  // Pessimistic initial state for basic blocks without any predecessors
+  // (not needed for most functions, thus initialized lazily).
+  SrcState PessimisticState;
+
 public:
   DataflowSrcSafetyAnalysis(BinaryFunction &BF,
                             MCPlusBuilder::AllocatorIdTy AllocId,
@@ -585,6 +628,18 @@ class DataflowSrcSafetyAnalysis
     if (BB.isEntryPoint())
       return createEntryState();
 
+    // If a basic block without any predecessors is found in an optimized code,
+    // this likely means that some CFG edges were not detected. Pessimistically
+    // assume any register that can ever be clobbered in this function to be
+    // unsafe before this basic block.
+    // Warn about this fact in FunctionAnalysis::findUnsafeUses(), as it likely
+    // means imprecise CFG information.
+    if (BB.pred_empty()) {
+      if (PessimisticState.empty())
+        PessimisticState = computePessimisticState(*BB.getParent());
+      return PessimisticState;
+    }
+
     return SrcState();
   }
 
@@ -682,19 +737,14 @@ template <typename StateTy> class CFGUnawareAnalysis {
 //
 // Then, a function can be split into a number of disjoint contiguous sequences
 // of instructions without labels in between. These sequences can be processed
-// the same way basic blocks are processed by data-flow analysis, assuming
-// pessimistically that all registers are unsafe at the start of each sequence.
+// the same way basic blocks are processed by data-flow analysis, with the same
+// pessimistic estimation of the initial state at the start of each sequence
+// (except the first instruction of the function).
 class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
                                     public CFGUnawareAnalysis<SrcState> {
   using SrcSafetyAnalysis::BC;
   BinaryFunction &BF;
 
-  /// Creates a state with all registers marked unsafe (not to be confused
-  /// with empty state).
-  SrcState createUnsafeState() const {
-    return SrcState(NumRegs, RegsToTrackInstsFor.getNumTrackedRegisters());
-  }
-
 public:
   CFGUnawareSrcSafetyAnalysis(BinaryFunction &BF,
                               MCPlusBuilder::AllocatorIdTy AllocId,
@@ -704,6 +754,7 @@ class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
   }
 
   void run() override {
+    const SrcState DefaultState = computePessimisticState(BF);
     SrcState S = createEntryState();
     for (auto &I : BF.instrs()) {
       MCInst &Inst = I.second;
@@ -718,7 +769,7 @@ class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis,
         LLVM_DEBUG({
           traceInst(BC, "Due to label, resetting the state before", Inst);
         });
-        S = createUnsafeState();
+        S = DefaultState;
       }
 
       // Attach the state *before* this instruction executes.
@@ -1344,17 +1395,6 @@ shouldReportAuthOracle(const BinaryContext &BC, const MCInstReference &Inst,
   return make_gadget_report(AuthOracleKind, Inst, *AuthReg);
 }
 
-template <typename T> static void iterateOverInstrs(BinaryFunction &BF, T Fn) {
-  if (BF.hasCFG()) {
-    for (BinaryBasicBlock &BB : BF)
-      for (int64_t I = 0, E = BB.size(); I < E; ++I)
-        Fn(MCInstInBBReference(&BB, I));
-  } else {
-    for (auto I : BF.instrs())
-      Fn(MCInstInBFReference(&BF, I.first));
-  }
-}
-
 static SmallVector<MCPhysReg>
 collectRegsToTrack(ArrayRef<PartialReport<MCPhysReg>> Reports) {
   SmallSet<MCPhysReg, 4> RegsToTrack;
@@ -1375,17 +1415,60 @@ void FunctionAnalysisContext::findUnsafeUses(
     BF.dump();
   });
 
+  bool UnreachableBBReported = false;
+  if (BF.hasCFG()) {
+    // Warn on basic blocks being unreachable according to BOLT (at most once
+    // per BinaryFunction), as this likely means the CFG reconstructed by BOLT
+    // is imprecise. A basic block can be
+    // * reachable from an entry basic block - a hopefully correct non-empty
+    //   state is propagated to that basic block sooner or later. All basic
+    //   blocks are expected to belong to this category under normal conditions.
+    // * reachable from a "directly unreachable" BB (a basic block that has no
+    //   direct predecessors and this is not because it is an entry BB) - *some*
+    //   non-empty state is propagated to this basic block sooner or later, as
+    //   the initial state of directly unreachable basic blocks is
+    //   pessimistically initialized to "all registers are unsafe"
+    //   - a warning can be printed for the "directly unreachable" basic block
+    // * neither reachable from an entry nor from a "directly unreachable" BB
+    //   (such as if this BB is in an isolated loop of basic blocks) - the final
+    //   state is computed to be empty for this basic block
+    //   - a warning can be printed for this basic block
+    for (BinaryBasicBlock &BB : BF) {
+      MCInst *FirstInst = BB.getFirstNonPseudoInstr();
+      // Skip empty basic block early for simplicity.
+      if (!FirstInst)
+        continue;
+
+      bool IsDirectlyUnreachable = BB.pred_empty() && !BB.isEntryPoint();
+      bool HasNoStateComputed = Analysis->getStateBefore(*FirstInst).empty();
+      if (!IsDirectlyUnreachable && !HasNoStateComputed)
+        continue;
+
+      // Arbitrarily attach the report to the first instruction of BB.
+      // This is printed as "[message] in function [name], basic block ...,
+      // at address ..." when the issue is reported to the user.
+      Reports.push_back(make_generic_report(
+          MCInstReference::get(FirstInst, BF),
+          "Warning: possibly imprecise CFG, the analysis quality may be "
+          "degraded in this function. According to BOLT, unreachable code is "
+          "found" /* in function [name]... */));
+      UnreachableBBReported = true;
+      break; // One warning per function.
+    }
+  }
+  // FIXME: Warn the user about imprecise analysis when the function has no CFG
+  //        information at all.
+
   iterateOverInstrs(BF, [&](MCInstReference Inst) {
     if (BC.MIB->isCFI(Inst))
       return;
 
     const SrcState &S = Analysis->getStateBefore(Inst);
-
-    // If non-empty state was never propagated from the entry basic block
-    // to Inst, assume it to be unreachable and report a warning.
     if (S.empty()) {
-      Reports.push_back(
-          make_generic_report(Inst, "Warning: unreachable instruction found"));
+      LLVM_DEBUG(
+          { traceInst(BC, "Instruction has no state, skipping", Inst); });
+      assert(UnreachableBBReported && "Should be reported at least once");
+      (void)UnreachableBBReported;
       return;
     }
 

bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s

@@ -215,7 +215,7 @@ f_callclobbered_calleesaved:
         .globl  f_unreachable_instruction
         .type   f_unreachable_instruction,@function
 f_unreachable_instruction:
-// CHECK-LABEL: GS-PAUTH: Warning: unreachable instruction found in function f_unreachable_instruction, basic block {{[0-9a-zA-Z.]+}}, at address
+// CHECK-LABEL: GS-PAUTH: Warning: possibly imprecise CFG, the analysis quality may be degraded in this function. According to BOLT, unreachable code is found in function f_unreachable_instruction, basic block {{[0-9a-zA-Z.]+}}, at address
 // CHECK-NEXT:    The instruction is     {{[0-9a-f]+}}:       add     x0, x1, x2
 // CHECK-NOT:   instructions that write to the affected registers after any authentication are:
         b       1f
@@ -224,20 +224,33 @@ f_unreachable_instruction:
         ret
         .size f_unreachable_instruction, .-f_unreachable_instruction
 
-// Expected false positive: without CFG, the state is reset to all-unsafe
-// after an unconditional branch.
+// Without CFG, the state is reset at labels, assuming every register that can
+// be clobbered in the function was actually clobbered.
 
-        .globl  state_is_reset_after_indirect_branch_nocfg
-        .type   state_is_reset_after_indirect_branch_nocfg,@function
-state_is_reset_after_indirect_branch_nocfg:
-// CHECK-LABEL: GS-PAUTH: non-protected ret found in function state_is_reset_after_indirect_branch_nocfg, at address
-// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:         ret
+        .globl  lr_untouched_nocfg
+        .type   lr_untouched_nocfg,@function
+lr_untouched_nocfg:
+// CHECK-NOT: lr_untouched_nocfg
+        adr     x2, 1f
+        br      x2
+1:
+        ret
+        .size lr_untouched_nocfg, .-lr_untouched_nocfg
+
+        .globl  lr_clobbered_nocfg
+        .type   lr_clobbered_nocfg,@function
+lr_clobbered_nocfg:
+// CHECK-LABEL: GS-PAUTH: non-protected ret found in function lr_clobbered_nocfg, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      ret
 // CHECK-NEXT:  The 0 instructions that write to the affected registers after any authentication are:
         adr     x2, 1f
         br      x2
 1:
+        b       2f
+        bl      g   // never executed, but affects the expected worst-case scenario
+2:
         ret
-        .size state_is_reset_after_indirect_branch_nocfg, .-state_is_reset_after_indirect_branch_nocfg
+        .size lr_clobbered_nocfg, .-lr_clobbered_nocfg
 
 /// Now do a basic sanity check on every different Authentication instruction:
 

bolt/test/binary-analysis/AArch64/gs-pauth-authentication-oracles.s

@@ -491,10 +491,6 @@ good_address_arith_multi_bb:
         ret
         .size good_address_arith_multi_bb, .-good_address_arith_multi_bb
 
-// FIXME: Most *_nocfg test cases contain paciasp+autiasp instructions even if
-//        LR is not spilled - this is a workaround for RET instructions being
-//        reported as non-protected, because LR state is reset at every label.
-
         .globl  good_ret_nocfg
         .type   good_ret_nocfg,@function
 good_ret_nocfg:
@@ -541,29 +537,25 @@ good_branch_nocfg:
         .type   good_load_other_reg_nocfg,@function
 good_load_other_reg_nocfg:
 // CHECK-NOT: good_load_other_reg_nocfg
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         ldr     x2, [x0]
 
-        autiasp
         ret
         .size good_load_other_reg_nocfg, .-good_load_other_reg_nocfg
 
         .globl  good_load_same_reg_nocfg
         .type   good_load_same_reg_nocfg,@function
 good_load_same_reg_nocfg:
 // CHECK-NOT: good_load_same_reg_nocfg
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         ldr     x0, [x0]
 
-        autiasp
         ret
         .size good_load_same_reg_nocfg, .-good_load_same_reg_nocfg
 
@@ -575,13 +567,11 @@ bad_unchecked_nocfg:
 // CHECK-LABEL: GS-PAUTH: authentication oracle found in function bad_unchecked_nocfg, at address
 // CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
 // CHECK-NEXT:  The 0 instructions that leak the affected registers are:
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
 
-        autiasp
         ret
         .size bad_unchecked_nocfg, .-bad_unchecked_nocfg
 
@@ -615,15 +605,13 @@ bad_unknown_usage_read_nocfg:
 // CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
 // CHECK-NEXT:  The 1 instructions that leak the affected registers are:
 // CHECK-NEXT:  1.     {{[0-9a-f]+}}:      mul     x3, x0, x1
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         mul     x3, x0, x1
         ldr     x2, [x0]
 
-        autiasp
         ret
         .size bad_unknown_usage_read_nocfg, .-bad_unknown_usage_read_nocfg
 
@@ -634,15 +622,13 @@ bad_unknown_usage_subreg_read_nocfg:
 // CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
 // CHECK-NEXT:  The 1 instructions that leak the affected registers are:
 // CHECK-NEXT:  1.     {{[0-9a-f]+}}:      mul     w3, w0, w1
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         mul     w3, w0, w1
         ldr     x2, [x0]
 
-        autiasp
         ret
         .size bad_unknown_usage_subreg_read_nocfg, .-bad_unknown_usage_subreg_read_nocfg
 
@@ -653,38 +639,33 @@ bad_unknown_usage_update_nocfg:
 // CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
 // CHECK-NEXT:  The 1 instructions that leak the affected registers are:
 // CHECK-NEXT:  1.     {{[0-9a-f]+}}:      movk    x0, #0x2a, lsl #16
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         movk    x0, #42, lsl #16  // does not overwrite x0 completely
         ldr     x2, [x0]
 
-        autiasp
         ret
         .size bad_unknown_usage_update_nocfg, .-bad_unknown_usage_update_nocfg
 
         .globl  good_overwrite_with_constant_nocfg
         .type   good_overwrite_with_constant_nocfg,@function
 good_overwrite_with_constant_nocfg:
 // CHECK-NOT: good_overwrite_with_constant_nocfg
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
         mov     x0, #42
 
-        autiasp
         ret
         .size good_overwrite_with_constant_nocfg, .-good_overwrite_with_constant_nocfg
 
         .globl  good_address_arith_nocfg
         .type   good_address_arith_nocfg,@function
 good_address_arith_nocfg:
 // CHECK-NOT: good_address_arith_nocfg
-        paciasp
         adr     x2, 1f
         br      x2
 1:
@@ -698,7 +679,6 @@ good_address_arith_nocfg:
         mov     x1, #0
         mov     x2, #0
 
-        autiasp
         ret
         .size good_address_arith_nocfg, .-good_address_arith_nocfg