Address more review comments

Author: atrosinenko

bolt/include/bolt/Core/MCPlusBuilder.h

@@ -587,18 +587,29 @@ class MCPlusBuilder {
     return getNoRegister();
   }
 
-  virtual MCPhysReg getSafelyMaterializedAddressReg(const MCInst &Inst) const {
+  /// Returns the register containing an address which is safely materialized
+  /// under Pointer Authentication threat model, or NoRegister otherwise.
+  ///
+  /// The produced address should not be attacker-controlled, assuming an
+  /// attacker is able to modify any writable memory, but not executable code
+  /// (as it should be W^X).
+  virtual MCPhysReg getMaterializedAddressRegForPtrAuth(const MCInst &Inst) const {
     llvm_unreachable("not implemented");
     return getNoRegister();
   }
 
-  /// Analyzes if this instruction can safely perform address arithmetics.
+  /// Analyzes if this instruction can safely perform address arithmetics
+  /// under Pointer Authentication threat model.
+  ///
+  /// If an (OutReg, InReg) pair is returned, then after Inst is executed,
+  /// OutReg is as trusted as InReg is.
   ///
-  /// If the first element of the returned pair is no-register, this instruction
-  /// is considered unknown. Otherwise, (output, input) pair is returned,
-  /// so that output is as trusted as input is.
-  virtual std::pair<MCPhysReg, MCPhysReg>
-  analyzeSafeAddressArithmetics(const MCInst &Inst) const {
+  /// The arithmetic instruction is considered safe if OutReg is not attacker-
+  /// controlled, provided InReg and executable code are not. Please note that
+  /// registers other than InReg as well as the contents of memory which is
+  /// writable by the process should be considered attacker-controlled.
+  virtual std::optional<std::pair<MCPhysReg, MCPhysReg>>
+  analyzeAddressArithmeticsForPtrAuth(const MCInst &Inst) const {
     llvm_unreachable("not implemented");
     return std::make_pair(getNoRegister(), getNoRegister());
   }

bolt/lib/Passes/PAuthGadgetScanner.cpp

@@ -353,8 +353,8 @@ class PacRetAnalysis
 
   // Returns all registers that can be treated as if they are written by an
   // authentication instruction.
-  SmallVector<MCPhysReg> getAuthenticatedRegs(const MCInst &Point,
-                                              const State &Cur) const {
+  SmallVector<MCPhysReg> getRegsMadeSafeToDeref(const MCInst &Point,
+                                                const State &Cur) const {
     SmallVector<MCPhysReg> Regs;
     const MCPhysReg NoReg = BC.MIB->getNoRegister();
 
@@ -364,17 +364,16 @@ class PacRetAnalysis
       Regs.push_back(*AutReg);
 
     // ... a safe address can be materialized, or
-    MCPhysReg NewAddrReg = BC.MIB->getSafelyMaterializedAddressReg(Point);
+    MCPhysReg NewAddrReg = BC.MIB->getMaterializedAddressRegForPtrAuth(Point);
     if (NewAddrReg != NoReg)
       Regs.push_back(NewAddrReg);
 
     // ... an address can be updated in a safe manner, producing the result
     // which is as trusted as the input address.
-    MCPhysReg ArithResult, ArithSrc;
-    std::tie(ArithResult, ArithSrc) =
-        BC.MIB->analyzeSafeAddressArithmetics(Point);
-    if (ArithResult != NoReg && Cur.SafeToDerefRegs[ArithSrc])
-      Regs.push_back(ArithResult);
+    if (auto DstAndSrc = BC.MIB->analyzeAddressArithmeticsForPtrAuth(Point)) {
+      if (Cur.SafeToDerefRegs[DstAndSrc->second])
+        Regs.push_back(DstAndSrc->first);
+    }
 
     return Regs;
   }
@@ -403,12 +402,8 @@ class PacRetAnalysis
     // before its execution into account, if necessary.
 
     BitVector Clobbered = getClobberedRegs(Point);
-    // Compute the set of registers that can be considered as written by
-    // an authentication instruction. This includes operations that are
-    // *strictly better* than authentication, such as materializing a
-    // PC-relative constant.
-    SmallVector<MCPhysReg> AuthenticatedOrBetter =
-        getAuthenticatedRegs(Point, Cur);
+    SmallVector<MCPhysReg> NewSafeToDerefRegs =
+        getRegsMadeSafeToDeref(Point, Cur);
 
     // Then, compute the state after this instruction is executed.
     State Next = Cur;
@@ -423,12 +418,12 @@ class PacRetAnalysis
     // After accounting for clobbered registers in general, override the state
     // according to authentication and other *special cases* of clobbering.
 
-    // The sub-registers of each authenticated register are also trusted now,
-    // but not their super-registers (as they retain untrusted register units).
-    BitVector AuthenticatedSubregs(NumRegs);
-    for (MCPhysReg AutReg : AuthenticatedOrBetter)
-      AuthenticatedSubregs |= BC.MIB->getAliases(AutReg, /*OnlySmaller=*/true);
-    for (MCPhysReg Reg : AuthenticatedSubregs.set_bits()) {
+    // The sub-registers are also safe-to-dereference now, but not their
+    // super-registers (as they retain untrusted register units).
+    BitVector NewSafeSubregs(NumRegs);
+    for (MCPhysReg AutReg : NewSafeToDerefRegs)
+      NewSafeSubregs |= BC.MIB->getAliases(AutReg, /*OnlySmaller=*/true);
+    for (MCPhysReg Reg : NewSafeSubregs.set_bits()) {
       Next.SafeToDerefRegs.set(Reg);
       if (RegsToTrackInstsFor.isTracked(Reg))
         lastWritingInsts(Next, Reg).clear();

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

@@ -304,30 +304,36 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
     }
   }
 
-  MCPhysReg getSafelyMaterializedAddressReg(const MCInst &Inst) const override {
+  MCPhysReg getMaterializedAddressRegForPtrAuth(const MCInst &Inst) const override {
     switch (Inst.getOpcode()) {
     case AArch64::ADR:
     case AArch64::ADRP:
+      // These instructions produce an address value based on the information
+      // encoded into the instruction itself (which should reside in a read-only
+      // code memory) and the value of PC register (that is, the location of
+      // this instruction), so the produced value is not attacker-controlled.
       return Inst.getOperand(0).getReg();
     default:
       return getNoRegister();
     }
   }
 
-  std::pair<MCPhysReg, MCPhysReg>
-  analyzeSafeAddressArithmetics(const MCInst &Inst) const override {
+  std::optional<std::pair<MCPhysReg, MCPhysReg>>
+  analyzeAddressArithmeticsForPtrAuth(const MCInst &Inst) const override {
     switch (Inst.getOpcode()) {
     default:
-      return std::make_pair(getNoRegister(), getNoRegister());
+      return std::nullopt;
     case AArch64::ADDXri:
     case AArch64::SUBXri:
+      // The immediate addend is encoded into the instruction itself, so it is
+      // not attacker-controlled under Pointer Authentication threat model.
       return std::make_pair(Inst.getOperand(0).getReg(),
                             Inst.getOperand(1).getReg());
     case AArch64::ORRXrs:
       // "mov Xd, Xm" is equivalent to "orr Xd, XZR, Xm, lsl #0"
       if (Inst.getOperand(1).getReg() != AArch64::XZR ||
           Inst.getOperand(3).getImm() != 0)
-        return std::make_pair(getNoRegister(), getNoRegister());
+        return std::nullopt;
 
       return std::make_pair(Inst.getOperand(0).getReg(),
                             Inst.getOperand(2).getReg());

bolt/test/binary-analysis/AArch64/gs-pauth-address-materialization.s

@@ -1,4 +1,5 @@
-// RUN: %clang %cflags -march=armv8.3-a %s -o %t.exe
+// -Wl,--no-relax prevents converting ADRP+ADD pairs into NOP+ADR.
+// RUN: %clang %cflags -march=armv8.3-a -Wl,--no-relax %s -o %t.exe
 // RUN: llvm-bolt-binary-analysis --scanners=pauth %t.exe 2>&1 | FileCheck %s
 
 // Test various patterns that should or should not be considered safe

bolt/test/binary-analysis/AArch64/lit.local.cfg

@@ -1,8 +1,7 @@
 if "AArch64" not in config.root.targets:
     config.unsupported = True
 
-# -Wl,--no-relax prevents converting ADRP+ADD pairs into NOP+ADR.
-flags = "--target=aarch64-linux-gnu -nostartfiles -nostdlib -ffreestanding -Wl,--no-relax"
+flags = "--target=aarch64-linux-gnu -nostartfiles -nostdlib -ffreestanding"
 
 config.substitutions.insert(0, ("%cflags", f"%cflags {flags}"))
 config.substitutions.insert(0, ("%cxxflags", f"%cxxflags {flags}"))