Simplify a questionable rule in DstSafetyAnalysis::getRegsMadeProtected

Author: atrosinenko

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -759,14 +759,24 @@ SrcSafetyAnalysis::create(BinaryFunction &BF,
 /// 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
-/// until that register is either wiped or checked not to contain a result of a
-/// failed authentication.
+/// Considering the instructions in forward order as they are executed, a
+/// restricted set of operations can be allowed on any register containing a
+/// value derived from the result of an authentication instruction until that
+/// value is checked not to contain the result of a failed authentication.
+/// In DstSafetyAnalysis, these rules are adapted, so that the safety property
+/// for a register is computed by iterating the instructions in backward order.
+/// Then the resulting properties are used at authentication instruction sites
+/// to check output registers and report the particular instruction if it writes
+/// to an unsafe register.
 ///
-/// Specifically, the safety property for a register is computed by iterating
-/// the instructions in backward order: the source register Xn of an instruction
-/// Inst is safe if at least one of the following is true:
+/// Another approach would be to simulate the above rules as-is, iterating over
+/// the instructions in forward direction. To make it possible to report the
+/// particular instructions as oracles, this would probably require tracking
+/// references to these instructions for each register currently containing
+/// sensitive data.
+///
+/// In DstSafetyAnalysis, the source register Xn of an instruction Inst is safe
+/// if at least one of the following is true:
 /// * Inst checks if Xn contains the result of a successful authentication and
 ///   terminates the program on failure. Note that Inst can either naturally
 ///   dereference Xn (load, branch, return, etc. instructions) or be the first
@@ -776,7 +786,7 @@ SrcSafetyAnalysis::create(BinaryFunction &BF,
 ///   execution of Inst (temporaries are not used on AArch64 and thus not
 ///   currently supported/allowed).
 ///   See MCPlusBuilder::analyzeAddressArithmeticsForPtrAuth for the details.
-/// * Inst fully overwrites Xn with an unrelated value.
+/// * Inst fully overwrites Xn with a constant.
 struct DstState {
   /// The set of registers whose values cannot be inspected by an attacker in
   /// a way usable as an authentication oracle. The results of authentication
@@ -913,8 +923,9 @@ class DstSafetyAnalysis {
   }
 
   /// Returns the set of registers that can be leaked by this instruction.
-  /// This is computed similar to the set of clobbered registers, but taking
-  /// input operands instead of outputs.
+  /// A register is considered leaked if it has any intersection with any
+  /// register read by Inst. This is similar to how the set of clobbered
+  /// registers is computed, but taking input operands instead of outputs.
   BitVector getLeakedRegs(const MCInst &Inst) const {
     BitVector Leaked(NumRegs);
 
@@ -982,17 +993,18 @@ class DstSafetyAnalysis {
         Regs.push_back(SrcReg);
     }
 
-    // ... the register can be overwritten in whole with an unrelated value -
-    // for that reason, ignore the registers that are both read and written:
-    //
-    //     movk x0, #42, lsl #16  // keeps some bits of x0
-    //     mul  x1, x1, #3        // not all information is actually lost
-    //
-    BitVector FullyOverwrittenRegs;
-    BC.MIB->getWrittenRegs(Inst, FullyOverwrittenRegs);
-    FullyOverwrittenRegs.reset(LeakedRegs);
-    for (MCPhysReg Reg : FullyOverwrittenRegs.set_bits())
-      Regs.push_back(Reg);
+    // ... the register can be overwritten in whole with a constant: for that
+    // purpose, look for the instructions with no register inputs (neither
+    // explicit nor implicit ones) and no side effects (to rule out reading
+    // not modelled locations).
+    const MCInstrDesc &Desc = BC.MII->get(Inst.getOpcode());
+    bool HasExplicitSrcRegs = llvm::any_of(BC.MIB->useOperands(Inst),
+                                           [](auto Op) { return Op.isReg(); });
+    if (!Desc.hasUnmodeledSideEffects() && !HasExplicitSrcRegs &&
+        Desc.implicit_uses().empty()) {
+      for (const MCOperand &Def : BC.MIB->defOperands(Inst))
+        Regs.push_back(Def.getReg());
+    }
 
     return Regs;
   }

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

@@ -223,10 +223,37 @@ bad_unknown_usage_update:
         ret
         .size bad_unknown_usage_update, .-bad_unknown_usage_update
 
+        .globl  good_overwrite_with_constant
+        .type   good_overwrite_with_constant,@function
+good_overwrite_with_constant:
+// CHECK-NOT: good_overwrite_with_constant
+        autia   x0, x1
+        mov     x0, #42
+        ret
+        .size good_overwrite_with_constant, .-good_overwrite_with_constant
+
+// Overwriting sensitive data by instructions with unmodelled side-effects is
+// explicitly rejected, even though this particular MRS is safe.
+        .globl  bad_overwrite_with_side_effects
+        .type   bad_overwrite_with_side_effects,@function
+bad_overwrite_with_side_effects:
+// CHECK-LABEL: GS-PAUTH: authentication oracle found in function bad_overwrite_with_side_effects, basic block {{[^,]+}}, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
+// CHECK-NEXT:  The 0 instructions that leak the affected registers are:
+        autia   x0, x1
+        mrs     x0, CTR_EL0
+        ret
+        .size bad_overwrite_with_side_effects, .-bad_overwrite_with_side_effects
+
+// Here the new value written by MUL to x0 is completely unrelated to the result
+// of authentication, so this is a false positive.
+// FIXME: Can/should we generalize overwriting by constant to handle such cases?
         .globl  good_unknown_overwrite
         .type   good_unknown_overwrite,@function
 good_unknown_overwrite:
-// CHECK-NOT: good_unknown_overwrite
+// CHECK-LABEL: GS-PAUTH: authentication oracle found in function good_unknown_overwrite, basic block {{[^,]+}}, at address
+// CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
+// CHECK-NEXT:  The 0 instructions that leak the affected registers are:
         autia   x0, x1
         mul     x0, x1, x2
         ret
@@ -235,15 +262,15 @@ good_unknown_overwrite:
 // This is a false positive: when a general-purpose register is written to as
 // a 32-bit register, its top 32 bits are zeroed, but according to LLVM
 // representation, the instruction only overwrites the Wn register.
-        .globl  good_unknown_wreg_overwrite
-        .type   good_unknown_wreg_overwrite,@function
-good_unknown_wreg_overwrite:
-// CHECK-LABEL: GS-PAUTH: authentication oracle found in function good_unknown_wreg_overwrite, basic block {{[^,]+}}, at address
+        .globl  good_wreg_overwrite
+        .type   good_wreg_overwrite,@function
+good_wreg_overwrite:
+// CHECK-LABEL: GS-PAUTH: authentication oracle found in function good_wreg_overwrite, basic block {{[^,]+}}, at address
 // CHECK-NEXT:  The instruction is     {{[0-9a-f]+}}:      autia   x0, x1
         autia   x0, x1
-        mul     w0, w1, w2
+        mov     w0, #42
         ret
-        .size good_unknown_wreg_overwrite, .-good_unknown_wreg_overwrite
+        .size good_wreg_overwrite, .-good_wreg_overwrite
 
         .globl  good_address_arith
         .type   good_address_arith,@function
@@ -435,16 +462,16 @@ bad_unknown_usage_update_multi_bb:
         ret
         .size bad_unknown_usage_update_multi_bb, .-bad_unknown_usage_update_multi_bb
 
-        .globl  good_unknown_overwrite_multi_bb
-        .type   good_unknown_overwrite_multi_bb,@function
-good_unknown_overwrite_multi_bb:
-// CHECK-NOT: good_unknown_overwrite_multi_bb
+        .globl  good_overwrite_with_constant_multi_bb
+        .type   good_overwrite_with_constant_multi_bb,@function
+good_overwrite_with_constant_multi_bb:
+// CHECK-NOT: good_overwrite_with_constant_multi_bb
         autia   x0, x1
         cbz     x3, 1f
 1:
-        mul     x0, x1, x2
+        mov     x0, #42
         ret
-        .size good_unknown_overwrite_multi_bb, .-good_unknown_overwrite_multi_bb
+        .size good_overwrite_with_constant_multi_bb, .-good_overwrite_with_constant_multi_bb
 
         .globl  good_address_arith_multi_bb
         .type   good_address_arith_multi_bb,@function
@@ -638,20 +665,20 @@ bad_unknown_usage_update_nocfg:
         ret
         .size bad_unknown_usage_update_nocfg, .-bad_unknown_usage_update_nocfg
 
-        .globl  good_unknown_overwrite_nocfg
-        .type   good_unknown_overwrite_nocfg,@function
-good_unknown_overwrite_nocfg:
-// CHECK-NOT: good_unknown_overwrite_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
-        mul     x0, x1, x2
+        mov     x0, #42
 
         autiasp
         ret
-        .size good_unknown_overwrite_nocfg, .-good_unknown_overwrite_nocfg
+        .size good_overwrite_with_constant_nocfg, .-good_overwrite_with_constant_nocfg
 
         .globl  good_address_arith_nocfg
         .type   good_address_arith_nocfg,@function