diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index 132d58f3f9f79..b233452985502 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -562,35 +562,56 @@ class MCPlusBuilder { return {}; } - virtual ErrorOr getAuthenticatedReg(const MCInst &Inst) const { - llvm_unreachable("not implemented"); - return getNoRegister(); - } - - virtual bool isAuthenticationOfReg(const MCInst &Inst, - MCPhysReg AuthenticatedReg) const { + /// Returns the register where an authenticated pointer is written to by Inst, + /// or std::nullopt if not authenticating any register. + /// + /// Sets IsChecked if the instruction always checks authenticated pointer, + /// i.e. it either writes a successfully authenticated pointer or terminates + /// the program abnormally (such as "ldra x0, [x1]!" on AArch64, which crashes + /// on authentication failure even if FEAT_FPAC is not implemented). + virtual std::optional + getWrittenAuthenticatedReg(const MCInst &Inst, bool &IsChecked) const { llvm_unreachable("not implemented"); - return false; + return std::nullopt; } - virtual MCPhysReg getSignedReg(const MCInst &Inst) const { + /// Returns the register signed by Inst, or std::nullopt if not signing any + /// register. + /// + /// The returned register is assumed to be both input and output operand, + /// as it is done on AArch64. + virtual std::optional getSignedReg(const MCInst &Inst) const { llvm_unreachable("not implemented"); - return getNoRegister(); + return std::nullopt; } - virtual ErrorOr getRegUsedAsRetDest(const MCInst &Inst) const { + /// Returns the register used as a return address. Returns std::nullopt if + /// not applicable, such as reading the return address from a system register + /// or from the stack. + /// + /// Sets IsAuthenticatedInternally if the instruction accepts a signed + /// pointer as its operand and authenticates it internally. + /// + /// Should only be called when isReturn(Inst) is true. + virtual std::optional + getRegUsedAsRetDest(const MCInst &Inst, + bool &IsAuthenticatedInternally) const { llvm_unreachable("not implemented"); - return getNoRegister(); + return std::nullopt; } /// Returns the register used as the destination of an indirect branch or call /// instruction. Sets IsAuthenticatedInternally if the instruction accepts /// a signed pointer as its operand and authenticates it internally. + /// + /// Should only be called if isIndirectCall(Inst) or isIndirectBranch(Inst) + /// returns true. virtual MCPhysReg getRegUsedAsIndirectBranchDest(const MCInst &Inst, bool &IsAuthenticatedInternally) const { llvm_unreachable("not implemented"); - return getNoRegister(); + return 0; // Unreachable. A valid register should be returned by the + // target implementation. } /// Returns the register containing an address safely materialized by `Inst` @@ -602,14 +623,14 @@ class MCPlusBuilder { /// controlled, under the Pointer Authentication threat model. /// /// If the instruction does not write to any register satisfying the above - /// two conditions, NoRegister is returned. + /// two conditions, std::nullopt is returned. /// /// The Pointer Authentication threat model assumes an attacker is able to /// modify any writable memory, but not executable code (due to W^X). - virtual MCPhysReg + virtual std::optional getMaterializedAddressRegForPtrAuth(const MCInst &Inst) const { llvm_unreachable("not implemented"); - return getNoRegister(); + return std::nullopt; } /// Analyzes if this instruction can safely perform address arithmetics @@ -622,10 +643,13 @@ class MCPlusBuilder { /// 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. + /// + /// The instruction should not write any values derived from InReg anywhere, + /// except for OutReg. virtual std::optional> analyzeAddressArithmeticsForPtrAuth(const MCInst &Inst) const { llvm_unreachable("not implemented"); - return std::make_pair(getNoRegister(), getNoRegister()); + return std::nullopt; } /// Analyzes if a pointer is checked to be authenticated successfully @@ -670,10 +694,10 @@ class MCPlusBuilder { /// /// Use this function for simple, single-instruction patterns instead of /// its getAuthCheckedReg(BB) counterpart. - virtual MCPhysReg getAuthCheckedReg(const MCInst &Inst, - bool MayOverwrite) const { + virtual std::optional getAuthCheckedReg(const MCInst &Inst, + bool MayOverwrite) const { llvm_unreachable("not implemented"); - return getNoRegister(); + return std::nullopt; } virtual bool isTerminator(const MCInst &Inst) const; diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp index 21bdd06aeb09c..971ea5fdef420 100644 --- a/bolt/lib/Passes/PAuthGadgetScanner.cpp +++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp @@ -66,14 +66,12 @@ namespace PAuthGadgetScanner { } [[maybe_unused]] static void traceReg(const BinaryContext &BC, StringRef Label, - ErrorOr Reg) { + MCPhysReg Reg) { dbgs() << " " << Label << ": "; - if (Reg.getError()) - dbgs() << "(error)"; - else if (*Reg == BC.MIB->getNoRegister()) + if (Reg == BC.MIB->getNoRegister()) dbgs() << "(none)"; else - dbgs() << BC.MRI->getName(*Reg); + dbgs() << BC.MRI->getName(Reg); dbgs() << "\n"; } @@ -365,17 +363,15 @@ class SrcSafetyAnalysis { SmallVector getRegsMadeSafeToDeref(const MCInst &Point, const SrcState &Cur) const { SmallVector Regs; - const MCPhysReg NoReg = BC.MIB->getNoRegister(); // A signed pointer can be authenticated, or - ErrorOr AutReg = BC.MIB->getAuthenticatedReg(Point); - if (AutReg && *AutReg != NoReg) + bool Dummy = false; + if (auto AutReg = BC.MIB->getWrittenAuthenticatedReg(Point, Dummy)) Regs.push_back(*AutReg); // ... a safe address can be materialized, or - MCPhysReg NewAddrReg = BC.MIB->getMaterializedAddressRegForPtrAuth(Point); - if (NewAddrReg != NoReg) - Regs.push_back(NewAddrReg); + if (auto NewAddrReg = BC.MIB->getMaterializedAddressRegForPtrAuth(Point)) + Regs.push_back(*NewAddrReg); // ... an address can be updated in a safe manner, producing the result // which is as trusted as the input address. @@ -391,13 +387,20 @@ class SrcSafetyAnalysis { SmallVector getRegsMadeTrusted(const MCInst &Point, const SrcState &Cur) const { SmallVector Regs; - const MCPhysReg NoReg = BC.MIB->getNoRegister(); // An authenticated pointer can be checked, or - MCPhysReg CheckedReg = + std::optional CheckedReg = BC.MIB->getAuthCheckedReg(Point, /*MayOverwrite=*/false); - if (CheckedReg != NoReg && Cur.SafeToDerefRegs[CheckedReg]) - Regs.push_back(CheckedReg); + if (CheckedReg && Cur.SafeToDerefRegs[*CheckedReg]) + Regs.push_back(*CheckedReg); + + // ... a pointer can be authenticated by an instruction that always checks + // the pointer, or + bool IsChecked = false; + std::optional AutReg = + BC.MIB->getWrittenAuthenticatedReg(Point, IsChecked); + if (AutReg && IsChecked) + Regs.push_back(*AutReg); if (CheckerSequenceInfo.contains(&Point)) { MCPhysReg CheckedReg; @@ -413,9 +416,8 @@ class SrcSafetyAnalysis { } // ... a safe address can be materialized, or - MCPhysReg NewAddrReg = BC.MIB->getMaterializedAddressRegForPtrAuth(Point); - if (NewAddrReg != NoReg) - Regs.push_back(NewAddrReg); + if (auto NewAddrReg = BC.MIB->getMaterializedAddressRegForPtrAuth(Point)) + Regs.push_back(*NewAddrReg); // ... an address can be updated in a safe manner, producing the result // which is as trusted as the input address. @@ -738,25 +740,27 @@ shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst, if (!BC.MIB->isReturn(Inst)) return std::nullopt; - ErrorOr MaybeRetReg = BC.MIB->getRegUsedAsRetDest(Inst); - if (MaybeRetReg.getError()) { + bool IsAuthenticated = false; + std::optional RetReg = + BC.MIB->getRegUsedAsRetDest(Inst, IsAuthenticated); + if (!RetReg) { return make_generic_report( Inst, "Warning: pac-ret analysis could not analyze this return " "instruction"); } - MCPhysReg RetReg = *MaybeRetReg; + if (IsAuthenticated) + return std::nullopt; + LLVM_DEBUG({ traceInst(BC, "Found RET inst", Inst); - traceReg(BC, "RetReg", RetReg); - traceReg(BC, "Authenticated reg", BC.MIB->getAuthenticatedReg(Inst)); + traceReg(BC, "RetReg", *RetReg); + traceRegMask(BC, "SafeToDerefRegs", S.SafeToDerefRegs); }); - if (BC.MIB->isAuthenticationOfReg(Inst, RetReg)) - return std::nullopt; - LLVM_DEBUG({ traceRegMask(BC, "SafeToDerefRegs", S.SafeToDerefRegs); }); - if (S.SafeToDerefRegs[RetReg]) + + if (S.SafeToDerefRegs[*RetReg]) return std::nullopt; - return make_gadget_report(RetKind, Inst, RetReg); + return make_gadget_report(RetKind, Inst, *RetReg); } static std::optional> @@ -772,7 +776,7 @@ shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst, if (IsAuthenticated) return std::nullopt; - assert(DestReg != BC.MIB->getNoRegister()); + assert(DestReg != BC.MIB->getNoRegister() && "Valid register expected"); LLVM_DEBUG({ traceInst(BC, "Found call inst", Inst); traceReg(BC, "Call destination reg", DestReg); @@ -789,19 +793,19 @@ shouldReportSigningOracle(const BinaryContext &BC, const MCInstReference &Inst, const SrcState &S) { static const GadgetKind SigningOracleKind("signing oracle found"); - MCPhysReg SignedReg = BC.MIB->getSignedReg(Inst); - if (SignedReg == BC.MIB->getNoRegister()) + std::optional SignedReg = BC.MIB->getSignedReg(Inst); + if (!SignedReg) return std::nullopt; LLVM_DEBUG({ traceInst(BC, "Found sign inst", Inst); - traceReg(BC, "Signed reg", SignedReg); + traceReg(BC, "Signed reg", *SignedReg); traceRegMask(BC, "TrustedRegs", S.TrustedRegs); }); - if (S.TrustedRegs[SignedReg]) + if (S.TrustedRegs[*SignedReg]) return std::nullopt; - return make_gadget_report(SigningOracleKind, Inst, SignedReg); + return make_gadget_report(SigningOracleKind, Inst, *SignedReg); } template static void iterateOverInstrs(BinaryFunction &BF, T Fn) { diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp index 3c69dc9091231..9d5a578cfbdff 100644 --- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp @@ -196,7 +196,10 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { return {AArch64::LR}; } - ErrorOr getAuthenticatedReg(const MCInst &Inst) const override { + std::optional + getWrittenAuthenticatedReg(const MCInst &Inst, + bool &IsChecked) const override { + IsChecked = false; switch (Inst.getOpcode()) { case AArch64::AUTIAZ: case AArch64::AUTIBZ: @@ -206,33 +209,12 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { case AArch64::AUTIBSPPCi: case AArch64::AUTIASPPCr: case AArch64::AUTIBSPPCr: - case AArch64::RETAA: - case AArch64::RETAB: - case AArch64::RETAASPPCi: - case AArch64::RETABSPPCi: - case AArch64::RETAASPPCr: - case AArch64::RETABSPPCr: return AArch64::LR; - case AArch64::AUTIA1716: case AArch64::AUTIB1716: case AArch64::AUTIA171615: case AArch64::AUTIB171615: return AArch64::X17; - - case AArch64::ERETAA: - case AArch64::ERETAB: - // The ERETA{A,B} instructions use either register ELR_EL1, ELR_EL2 or - // ELR_EL3, depending on the current Exception Level at run-time. - // - // Furthermore, these registers are not modelled by LLVM as a regular - // MCPhysReg.... So there is no way to indicate that through the current - // API. - // - // Therefore, return an error to indicate that LLVM/BOLT cannot model - // this. - return make_error_code(std::errc::result_out_of_range); - case AArch64::AUTIA: case AArch64::AUTIB: case AArch64::AUTDA: @@ -242,22 +224,18 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { case AArch64::AUTDZA: case AArch64::AUTDZB: return Inst.getOperand(0).getReg(); - - // FIXME: BL?RA(A|B)Z? and LDRA(A|B) should probably be handled here too. - + case AArch64::LDRAAwriteback: + case AArch64::LDRABwriteback: + // Note that LDRA(A|B)indexed are not listed here, as they do not write + // an authenticated pointer back to the register. + IsChecked = true; + return Inst.getOperand(2).getReg(); default: - return getNoRegister(); + return std::nullopt; } } - bool isAuthenticationOfReg(const MCInst &Inst, MCPhysReg Reg) const override { - if (Reg == getNoRegister()) - return false; - ErrorOr AuthenticatedReg = getAuthenticatedReg(Inst); - return AuthenticatedReg.getError() ? false : *AuthenticatedReg == Reg; - } - - MCPhysReg getSignedReg(const MCInst &Inst) const override { + std::optional getSignedReg(const MCInst &Inst) const override { switch (Inst.getOpcode()) { case AArch64::PACIA: case AArch64::PACIB: @@ -283,26 +261,36 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { case AArch64::PACIB171615: return AArch64::X17; default: - return getNoRegister(); + return std::nullopt; } } - ErrorOr getRegUsedAsRetDest(const MCInst &Inst) const override { + std::optional + getRegUsedAsRetDest(const MCInst &Inst, + bool &IsAuthenticatedInternally) const override { assert(isReturn(Inst)); switch (Inst.getOpcode()) { case AArch64::RET: + IsAuthenticatedInternally = false; return Inst.getOperand(0).getReg(); + case AArch64::RETAA: case AArch64::RETAB: case AArch64::RETAASPPCi: case AArch64::RETABSPPCi: case AArch64::RETAASPPCr: case AArch64::RETABSPPCr: + IsAuthenticatedInternally = true; return AArch64::LR; case AArch64::ERET: case AArch64::ERETAA: case AArch64::ERETAB: - return make_error_code(std::errc::result_out_of_range); + // The ERET* instructions use either register ELR_EL1, ELR_EL2 or + // ELR_EL3, depending on the current Exception Level at run-time. + // + // Furthermore, these registers are not modelled by LLVM as a regular + // MCPhysReg, so there is no way to indicate that through the current API. + return std::nullopt; default: llvm_unreachable("Unhandled return instruction"); } @@ -332,7 +320,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { } } - MCPhysReg + std::optional getMaterializedAddressRegForPtrAuth(const MCInst &Inst) const override { switch (Inst.getOpcode()) { case AArch64::ADR: @@ -343,7 +331,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { // this instruction), so the produced value is not attacker-controlled. return Inst.getOperand(0).getReg(); default: - return getNoRegister(); + return std::nullopt; } } @@ -483,8 +471,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { } } - MCPhysReg getAuthCheckedReg(const MCInst &Inst, - bool MayOverwrite) const override { + std::optional getAuthCheckedReg(const MCInst &Inst, + bool MayOverwrite) const override { // Cannot trivially reuse AArch64InstrInfo::getMemOperandWithOffsetWidth() // method as it accepts an instance of MachineInstr, not MCInst. const MCInstrDesc &Desc = Info->get(Inst.getOpcode()); @@ -545,6 +533,10 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { return false; }; + // FIXME: Not all load instructions are handled by this->mayLoad(Inst). + // On the other hand, MCInstrDesc::mayLoad() is permitted to return + // true for non-load instructions (such as AArch64::HINT) which + // would result in false negatives. if (mayLoad(Inst)) { // The first Use operand is the base address register. unsigned BaseRegIndex = Desc.getNumDefs(); @@ -553,10 +545,10 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { // the resulting address arbitrarily. for (unsigned I = BaseRegIndex + 1, E = Desc.getNumOperands(); I < E; ++I) if (Inst.getOperand(I).isReg()) - return getNoRegister(); + return std::nullopt; if (!MayOverwrite && ClobbersBaseRegExceptWriteback(BaseRegIndex)) - return getNoRegister(); + return std::nullopt; return Inst.getOperand(BaseRegIndex).getReg(); } @@ -564,7 +556,7 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { // Store instructions are not handled yet, as they are not important for // pauthtest ABI. Though, they could be handled similar to loads, if needed. - return getNoRegister(); + return std::nullopt; } bool isADRP(const MCInst &Inst) const override { @@ -854,6 +846,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { } bool mayLoad(const MCInst &Inst) const override { + // FIXME: Probably this could be tablegen-erated not to miss any existing + // or future opcodes. return isLDRB(Inst) || isLDRH(Inst) || isLDRW(Inst) || isLDRX(Inst) || isLDRQ(Inst) || isLDRD(Inst) || isLDRS(Inst); } diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-debug-output.s b/bolt/test/binary-analysis/AArch64/gs-pauth-debug-output.s index 078a8aca72d9c..82494d834a15c 100644 --- a/bolt/test/binary-analysis/AArch64/gs-pauth-debug-output.s +++ b/bolt/test/binary-analysis/AArch64/gs-pauth-debug-output.s @@ -119,7 +119,6 @@ simple: // PAUTH-NEXT: SafeToDerefRegs: W0 X0 W0_HI{{[ \t]*$}} // CHECK-NEXT: Found RET inst: 00000000: ret # DataflowSrcSafetyAnalysis: src-state // CHECK-NEXT: RetReg: LR -// CHECK-NEXT: Authenticated reg: (none) // CHECK-NEXT: SafeToDerefRegs: LR W30 W30_HI{{[ \t]*$}} .globl clobber @@ -146,7 +145,6 @@ clobber: // CHECK-EMPTY: // CHECK-NEXT: Found RET inst: 00000000: ret # DataflowSrcSafetyAnalysis: src-state // CHECK-NEXT: RetReg: LR -// CHECK-NEXT: Authenticated reg: (none) // CHECK-NEXT: SafeToDerefRegs: W30_HI{{[ \t]*$}} // CHECK-EMPTY: // CHECK-NEXT: Running detailed src register safety analysis... @@ -225,7 +223,6 @@ nocfg: // PAUTH-NEXT: SafeToDerefRegs: LR W0 W30 X0 W0_HI W30_HI // CHECK-NEXT: Found RET inst: 00000000: ret # Offset: 8 # CFGUnawareSrcSafetyAnalysis: src-state // CHECK-NEXT: RetReg: LR -// CHECK-NEXT: Authenticated reg: (none) // CHECK-NEXT: SafeToDerefRegs: // CHECK-EMPTY: // CHECK-NEXT: Running detailed src register safety analysis... diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s b/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s index 10fcf08cf158c..334a4108d8ab8 100644 --- a/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s +++ b/bolt/test/binary-analysis/AArch64/gs-pauth-signing-oracles.s @@ -985,6 +985,51 @@ inst_pacnbibsppc: ret .size inst_pacnbibsppc, .-inst_pacnbibsppc +// Test that write-back forms of LDRA(A|B) instructions are handled properly. + + .globl inst_ldraa_wb + .type inst_ldraa_wb,@function +inst_ldraa_wb: +// CHECK-NOT: inst_ldraa_wb + ldraa x2, [x0]! + pacda x0, x1 + ret + .size inst_ldraa_wb, .-inst_ldraa_wb + + .globl inst_ldrab_wb + .type inst_ldrab_wb,@function +inst_ldrab_wb: +// CHECK-NOT: inst_ldrab_wb + ldrab x2, [x0]! + pacda x0, x1 + ret + .size inst_ldrab_wb, .-inst_ldrab_wb + +// Non write-back forms of LDRA(A|B) instructions do not modify the address +// register, and thus do not make it safe. + + .globl inst_ldraa_non_wb + .type inst_ldraa_non_wb,@function +inst_ldraa_non_wb: +// CHECK-LABEL: GS-PAUTH: signing oracle found in function inst_ldraa_non_wb, basic block {{[^,]+}}, at address +// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: pacdb x0, x1 +// CHECK-NEXT: The 0 instructions that write to the affected registers after any authentication are: + ldraa x2, [x0] + pacdb x0, x1 + ret + .size inst_ldraa_non_wb, .-inst_ldraa_non_wb + + .globl inst_ldrab_non_wb + .type inst_ldrab_non_wb,@function +inst_ldrab_non_wb: +// CHECK-LABEL: GS-PAUTH: signing oracle found in function inst_ldrab_non_wb, basic block {{[^,]+}}, at address +// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: pacda x0, x1 +// CHECK-NEXT: The 0 instructions that write to the affected registers after any authentication are: + ldrab x2, [x0] + pacda x0, x1 + ret + .size inst_ldrab_non_wb, .-inst_ldrab_non_wb + .globl main .type main,@function main: