[BOLT] Gadget scanner: improve handling of unreachable basic blocks

[atrosinenko]

Instead of refusing to analyze an instruction completely when it is unreachable according to the CFG reconstructed by BOLT, use pessimistic assumption of register state when possible. Nevertheless, unreachable basic blocks found in optimized code likely means imprecise CFG reconstruction, thus report a warning once per function.

[atrosinenko]

• [BOLT] Gadget scanner: make use of C++17 features and LLVM helpers #141665
• [BOLT] Gadget scanner: optionally assume auth traps on failure #139778
• [BOLT] Gadget scanner: prevent false positives due to jump tables #138884
• [BOLT] Introduce helpers to match MCInsts one at a time (NFC) #138883
• [BOLT] Factor out MCInstReference from gadget scanner (NFC) #138655
• [BOLT] Gadget scanner: account for BRK when searching for auth oracles #137975
• [BOLT] Gadget scanner: detect untrusted LR before tail call #137224
• [BOLT] Gadget scanner: fix LR to be safe in leaf functions without CFG #141824
• [BOLT] Gadget scanner: improve handling of unreachable basic blocks #136183 👈 (View in Graphite)
• [BOLT] Gadget scanner: do not crash on debug-printing CFI instructions #136151
• [BOLT] Gadget scanner: detect authentication oracles #135663
• [BOLT] Gadget scanner: clarify MCPlusBuilder callbacks interface #136147
• [BOLT] Gadget scanner: refactor issue reporting #135662
• [BOLT] Gadget scanner: use more appropriate types (NFC) #135661
• [BOLT] Gadget scanner: detect signing oracles #134146
• [BOLT] Gadget scanner: analyze functions without CFG information #133461
• [BOLT] Gadget scanner: refine class names and debug output (NFC) #135073
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Anatoly Trosinenko (atrosinenko)

Changes

Instead of refusing to analyze an instruction completely, when it is
unreachable according to the CFG reconstructed by BOLT, pessimistically
assume all registers to be unsafe at the start of basic blocks without
any predecessors. Nevertheless, unreachable basic blocks found in
optimized code likely means imprecise CFG reconstruction, thus report a
warning once per basic block without predecessors.

---

Full diff: https://github.com/llvm/llvm-project/pull/136183.diff

3 Files Affected:

• (modified) bolt/lib/Passes/PAuthGadgetScanner.cpp (+32-14)
• (modified) bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s (+1-1)
• (modified) bolt/test/binary-analysis/AArch64/gs-pauth-calls.s (+57)

diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
index 2d2126bf05ae1..f998b8fa0f950 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -346,6 +346,12 @@ class SrcSafetyAnalysis {
     return S;
   }
 
+  /// Creates a state with all registers marked unsafe (not to be confused
+  /// with empty state).
+  SrcState createUnsafeState() const {
+    return SrcState(NumRegs, RegsToTrackInstsFor.getNumTrackedRegisters());
+  }
+
   BitVector getClobberedRegs(const MCInst &Point) const {
     BitVector Clobbered(NumRegs);
     // Assume a call can clobber all registers, including callee-saved
@@ -585,6 +591,13 @@ 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 all registers to be unsafe before this basic block and warn about
+    // this fact in FunctionAnalysis::findUnsafeUses().
+    if (BB.pred_empty())
+      return createUnsafeState();
+
     return SrcState();
   }
 
@@ -658,12 +671,6 @@ class CFGUnawareSrcSafetyAnalysis : public SrcSafetyAnalysis {
       BC.MIB->removeAnnotation(I.second, StateAnnotationIndex);
   }
 
-  /// 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,
@@ -1335,19 +1342,30 @@ void FunctionAnalysis::findUnsafeUses(
     BF.dump();
   });
 
+  if (BF.hasCFG()) {
+    // Warn on basic blocks being unreachable according to BOLT, as this
+    // likely means CFG is imprecise.
+    for (BinaryBasicBlock &BB : BF) {
+      if (!BB.pred_empty() || BB.isEntryPoint())
+        continue;
+      // Arbitrarily attach the report to the first instruction of BB.
+      MCInst *InstToReport = BB.getFirstNonPseudoInstr();
+      if (!InstToReport)
+        continue; // BB has no real instructions
+
+      Reports.push_back(
+          make_generic_report(MCInstReference::get(InstToReport, BF),
+                              "Warning: no predecessor basic blocks detected "
+                              "(possibly incomplete CFG)"));
+    }
+  }
+
   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"));
-      return;
-    }
+    assert(!S.empty() && "Instruction has no associated state");
 
     if (auto Report = shouldReportReturnGadget(BC, Inst, S))
       Reports.push_back(*Report);
diff --git a/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s b/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s
index 2193d40131478..a8cc6352de438 100644
--- a/bolt/test/binary-analysis/AArch64/gs-pacret-autiasp.s
+++ b/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: no predecessor basic blocks detected (possibly incomplete CFG) 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
         b       1f
         add     x0, x1, x2
diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s b/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s
index c79c5926a05cd..c20b47ca93e03 100644
--- a/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s
+++ b/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s
@@ -1428,6 +1428,63 @@ printed_instrs_nocfg:
         br      x0
         .size   printed_instrs_nocfg, .-printed_instrs_nocfg
 
+// Test handling of unreachable basic blocks.
+//
+// Basic blocks without any predecessors were observed in real-world optimized
+// code. At least sometimes they were actually reachable via jump table, which
+// was not detected, but the function was processed as if its CFG was
+// reconstructed successfully.
+//
+// As a more predictable model example, let's use really unreachable code
+// for testing.
+
+        .globl  bad_unreachable_call
+        .type   bad_unreachable_call,@function
+bad_unreachable_call:
+// CHECK-LABEL: GS-PAUTH: Warning: no predecessor basic blocks detected (possibly incomplete CFG) in function bad_unreachable_call, basic block {{[^,]+}}, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      blr     x0
+// CHECK-NEXT:  The 0 instructions that write to the affected registers after any authentication are:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_unreachable_call, basic block {{[^,]+}}, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      blr     x0
+// CHECK-NEXT:  The 0 instructions that write to the affected registers after any authentication are:
+        paciasp
+        stp     x29, x30, [sp, #-16]!
+        mov     x29, sp
+
+        b       1f
+        // unreachable basic block:
+        blr     x0
+
+1:      // reachable basic block:
+        ldp     x29, x30, [sp], #16
+        autiasp
+        ret
+        .size bad_unreachable_call, .-bad_unreachable_call
+
+        .globl  good_unreachable_call
+        .type   good_unreachable_call,@function
+good_unreachable_call:
+// CHECK-NOT: non-protected call{{.*}}good_unreachable_call
+// CHECK-LABEL: GS-PAUTH: Warning: no predecessor basic blocks detected (possibly incomplete CFG) in function good_unreachable_call, basic block {{[^,]+}}, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
+// CHECK-NEXT:  The 0 instructions that write to the affected registers after any authentication are:
+// CHECK-NOT: non-protected call{{.*}}good_unreachable_call
+        paciasp
+        stp     x29, x30, [sp, #-16]!
+        mov     x29, sp
+
+        b       1f
+        // unreachable basic block:
+        autia   x0, x1
+        blr     x0      // <-- this call is definitely protected provided at least
+                        //     basic block boundaries are detected correctly
+
+1:      // reachable basic block:
+        ldp     x29, x30, [sp], #16
+        autiasp
+        ret
+        .size good_unreachable_call, .-good_unreachable_call
+
         .globl  main
         .type   main,@function
 main:

[kbeyls]

Thanks, this mostly looks OK to me. I only have one comment, wondering about whether the warning message that gets produced could be improved a bit.

[kbeyls]

IIUC, with this patch basic blocks that are not part of the CFG as reconstructed by BOLT are now also analyzed. These blocks are analyzed with a pessimistic initial state.
There are 2 possible cases why a basic block is not part of the CFG:

1. BOLT wasn't able to reconstruct the CFG correctly.
   In this case, the pessimistic assumptions are probably going to cause more false positives in these "not-part-of-the-CFG basic blocks" than if BOLT was able to reconstruct the CFG?
   If all my assumptions above are correct, maybe the warning messages should state that more clearly, for example, something like "Warning: function {function_name} has seemingly unreachable basic blocks, possibly due to limits in how bolt can reverse engineer the CFG. This may lead to more false positives being reported in these basic blocks"
   Having said just that, I'm also thinking that maybe this is similar to the situation where BOLT cannot create a CFG at all. If so, should we (are we already?) producing a warning then too? But maybe that would produce way too many warnings?
2. The code really contains dead code (not impossible, code generators and assembly writers are known to sometimes make mistakes, or there might be a legitimate reason for seemingly dead code to be present in the binary)
   Would it be correct to say that in this case all reports against these dead code basic blocks would be false positives?

[atrosinenko]

Thank you for the suggestion, I rephrased the warning message in cdc20ab, though I used less concrete wording: not "more false positives" but "the analysis quality may be degraded". Even if incomplete CFG can only result in false positives when caused by jump table not being understood by BOLT (the only reason I observed in the wild so far), this seems to indicate the analyses in BOLT are somewhat broken for the function, so I would not be sure that this cannot result in lots of false negatives under different conditions.

All the reports for dead code are probably false positives, but if the particular code is actually seemingly dead (maybe some precompiled snippet for JIT or something...), then nothing can be told for sure, but this is hardly an issue of the scanner :) Anyway, this does not look like a widespread issue worth implementing a workaround at first glance, but maybe scanning large code bases will prove the opposite...

Considering the functions without CFG at all, printing a warning for them sounds reasonable, but this turned out to break a lot of tests relying on CHECK-NOT: function_name_nocfg - added a FIXME in e14dce9 for now.

[kbeyls]

Thanks, this looks good to me!