Address review comments, add a description for FirstInstLeakingReg

Author: atrosinenko

bolt/include/bolt/Passes/PAuthGadgetScanner.h

@@ -272,6 +272,11 @@ class ExtraInfo {
   virtual ~ExtraInfo() {}
 };
 
+/// The set of instructions writing to the affected register in an unsafe
+/// manner.
+///
+/// This is a hint to be printed alongside the report. It should be further
+/// analyzed by the user.
 class ClobberingInfo : public ExtraInfo {
   SmallVector<MCInstReference> ClobberingInstrs;
 
@@ -281,6 +286,11 @@ class ClobberingInfo : public ExtraInfo {
   void print(raw_ostream &OS, const MCInstReference Location) const override;
 };
 
+/// The set of instructions leaking the authenticated pointer before the
+/// result of authentication was checked.
+///
+/// This is a hint to be printed alongside the report. It should be further
+/// analyzed by the user.
 class LeakageInfo : public ExtraInfo {
   SmallVector<MCInstReference> LeakingInstrs;
 

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -746,13 +746,18 @@ SrcSafetyAnalysis::create(BinaryFunction &BF,
 /// Similar to SrcState, it is the responsibility of the analysis to take
 /// register aliasing into account.
 ///
-/// Depending on the implementation, it may be possible that an authentication
+/// Depending on the implementation (such as whether FEAT_FPAC is implemented
+/// by an AArch64 CPU or not), it may be possible that an authentication
 /// instruction returns an invalid pointer on failure instead of terminating
 /// the program immediately (assuming the program will crash as soon as that
-/// pointer is dereferenced). To prevent brute-forcing the correct signature,
-/// it should be impossible for an attacker to test if a pointer is correctly
-/// signed - either the program should be terminated on authentication failure
-/// or it should be impossible to tell whether authentication succeeded or not.
+/// pointer is dereferenced). Since few bits are usually allocated for the PAC
+/// field (such as less than 16 bits on a typical AArch64 system), an attacker
+/// can try every possible signature and guess the correct one if there is a
+/// gadget that tells whether the particular pointer has a correct signature
+/// (a so called "authentication oracle"). For that reason, it should be
+/// impossible for an attacker to test if a pointer is correctly signed -
+/// either the program should be terminated on authentication failure or
+/// the result of authentication should not be accessible to an attacker.
 ///
 /// For that reason, a restricted set of operations is allowed on any register
 /// containing a value derived from the result of an authentication instruction
@@ -778,6 +783,14 @@ struct DstState {
   /// instructions should only be written to such registers.
   BitVector CannotEscapeUnchecked;
 
+  /// A vector of sets, only used on the second analysis run.
+  /// Each element in this vector represents one of the tracked registers.
+  /// For each such register we track the set of first instructions that leak
+  /// the authenticated pointer before it was checked. This is intended to
+  /// provide clues on which instruction made the particular register unsafe.
+  ///
+  /// Please note that the mapping from MCPhysReg values to indexes in this
+  /// vector is provided by RegsToTrackInstsFor field of DstSafetyAnalysis.
   std::vector<SetOfRelatedInsts> FirstInstLeakingReg;
 
   /// Constructs an empty state.
@@ -1463,7 +1476,7 @@ void FunctionAnalysisContext::run() {
 
   SmallVector<PartialReport<MCPhysReg>> UnsafeDefs;
   findUnsafeDefs(UnsafeDefs);
-  handleSimpleReports(UnsafeUses);
+  handleSimpleReports(UnsafeDefs);
   if (!UnsafeDefs.empty())
     augmentUnsafeDefReports(UnsafeDefs);
 }

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

@@ -137,11 +137,56 @@ bad_unknown_usage_read:
 // CHECK-NEXT:  {{[0-9a-f]+}}:   ldr     x2, [x0]
 // CHECK-NEXT:  {{[0-9a-f]+}}:   ret
         autia   x0, x1
+        // Registers are not accessible to an attacker under Pointer
+        // Authentication threat model, until spilled to memory.
+        // Thus, reporting the below MUL instruction is a false positive, since
+        // the next LDR instruction prevents any possible spilling of x3 unless
+        // the authentication succeeded. Though, rejecting anything except for
+        // a closed list of instruction types is the intended behavior of the
+        // analysis, so this false positive is by design.
         mul     x3, x0, x1
         ldr     x2, [x0]
         ret
         .size bad_unknown_usage_read, .-bad_unknown_usage_read
 
+        .globl  bad_store_to_memory_and_wait
+        .type   bad_store_to_memory_and_wait,@function
+bad_store_to_memory_and_wait:
+// CHECK-LABEL: GS-PAUTH: authentication oracle found in function bad_store_to_memory_and_wait, basic block {{[^,]+}}, at address
+// 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]+}}:      str     x0, [x3]
+        autia   x0, x1
+        cbz     x3, 2f
+        str     x0, [x3]
+1:
+        // The thread performs a time-consuming computation while the result of
+        // authentication is accessible in memory.
+        nop
+2:
+        ldr     x2, [x0]
+        ret
+        .size bad_store_to_memory_and_wait, .-bad_store_to_memory_and_wait
+
+// FIXME: Known false negative: if no return instruction is reachable from a
+//        program point (this probably implies an infinite loop), such
+//        instruction cannot be detected as an authentication oracle.
+        .globl  bad_store_to_memory_and_hang
+        .type   bad_store_to_memory_and_hang,@function
+bad_store_to_memory_and_hang:
+// CHECK-NOT: bad_store_to_memory_and_hang
+        autia   x0, x1
+        cbz     x3, 2f
+        str     x0, [x3]
+1:
+        // The thread loops indefinitely while the result of authentication
+        // is accessible in memory.
+        b       1b
+2:
+        ldr     x2, [x0]
+        ret
+        .size bad_store_to_memory_and_hang, .-bad_store_to_memory_and_hang
+
         .globl  bad_unknown_usage_subreg_read
         .type   bad_unknown_usage_subreg_read,@function
 bad_unknown_usage_subreg_read:
@@ -419,6 +464,10 @@ 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:
@@ -454,13 +503,10 @@ good_call_nocfg:
         .type   good_branch_nocfg,@function
 good_branch_nocfg:
 // CHECK-NOT: good_branch_nocfg
-        paciasp
         adr     x2, 1f
         br      x2
 1:
         autia   x0, x1
-        autiasp            // authenticate LR before tail call
-        ldr     x2, [x30]  // check LR before tail call
         br      x0
         .size good_branch_nocfg, .-good_branch_nocfg