Skip to content

Commit f568ed0

Browse files
committed
[BOLT] Gadget scanner: prevent false positives due to jump tables
As part of PAuth hardening, AArch64 LLVM backend can use a special BR_JumpTable pseudo (enabled by -faarch64-jump-table-hardening Clang option) which is expanded in the AsmPrinter into a contiguous sequence without unsafe instructions in the middle. This commit adds another target-specific callback to MCPlusBuilder to make it possible to inhibit false positives for known-safe jump table dispatch sequences. Without special handling, the branch instruction is likely to be reported as a non-protected call (as its destination is not produced by an auth instruction, PC-relative address materialization, etc.) and possibly as a tail call being performed with unsafe link register (as the detection whether the branch instruction is a tail call is an heuristic). For now, only the specific instruction sequence used by the AArch64 LLVM backend is matched.
1 parent ec302b4 commit f568ed0

File tree

6 files changed

+829
-0
lines changed

6 files changed

+829
-0
lines changed

bolt/include/bolt/Core/MCInstUtils.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,15 @@ class MCInstReference {
101101
/// this function may be called from multithreaded code.
102102
uint64_t computeAddress(const MCCodeEmitter *Emitter = nullptr) const;
103103

104+
/// Returns the only preceding instruction, or std::nullopt if multiple or no
105+
/// predecessors are possible.
106+
///
107+
/// If CFG information is available, basic block boundary can be crossed,
108+
/// provided there is exactly one predecessor. If CFG is not available, the
109+
/// preceding instruction in the offset order is returned, unless this is the
110+
/// first instruction of the function.
111+
std::optional<MCInstReference> getSinglePredecessor();
112+
104113
raw_ostream &print(raw_ostream &OS) const;
105114

106115
private:

bolt/include/bolt/Core/MCPlusBuilder.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#define BOLT_CORE_MCPLUSBUILDER_H
1616

1717
#include "bolt/Core/BinaryBasicBlock.h"
18+
#include "bolt/Core/MCInstUtils.h"
1819
#include "bolt/Core/MCPlus.h"
1920
#include "bolt/Core/Relocation.h"
2021
#include "llvm/ADT/ArrayRef.h"
@@ -718,6 +719,19 @@ class MCPlusBuilder {
718719
return std::nullopt;
719720
}
720721

722+
/// Tests if BranchInst corresponds to an instruction sequence which is known
723+
/// to be a safe dispatch via jump table.
724+
///
725+
/// The target can decide which instruction sequences to consider "safe" from
726+
/// the Pointer Authentication point of view, such as any jump table dispatch
727+
/// sequence without function calls inside, any sequence which is contiguous,
728+
/// or only some specific well-known sequences.
729+
virtual bool
730+
isSafeJumpTableBranchForPtrAuth(MCInstReference BranchInst) const {
731+
llvm_unreachable("not implemented");
732+
return false;
733+
}
734+
721735
virtual bool isTerminator(const MCInst &Inst) const;
722736

723737
virtual bool isNoop(const MCInst &Inst) const {

bolt/lib/Core/MCInstUtils.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,3 +84,23 @@ raw_ostream &MCInstReference::print(raw_ostream &OS) const {
8484
OS << ">";
8585
return OS;
8686
}
87+
88+
std::optional<MCInstReference> MCInstReference::getSinglePredecessor() {
89+
if (const RefInBB *Ref = tryGetRefInBB()) {
90+
if (Ref->Index != 0)
91+
return MCInstReference(*Ref->BB, Ref->Index - 1);
92+
93+
if (Ref->BB->pred_size() != 1)
94+
return std::nullopt;
95+
96+
BinaryBasicBlock &PredBB = **Ref->BB->pred_begin();
97+
assert(!PredBB.empty() && "Empty basic blocks are not supported yet");
98+
return MCInstReference(PredBB, *PredBB.rbegin());
99+
}
100+
101+
const RefInBF &Ref = getRefInBF();
102+
if (Ref.It == Ref.BF->instrs().begin())
103+
return std::nullopt;
104+
105+
return MCInstReference(*Ref.BF, std::prev(Ref.It));
106+
}

bolt/lib/Passes/PAuthGadgetScanner.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1364,6 +1364,11 @@ shouldReportUnsafeTailCall(const BinaryContext &BC, const BinaryFunction &BF,
13641364
return std::nullopt;
13651365
}
13661366

1367+
if (BC.MIB->isSafeJumpTableBranchForPtrAuth(Inst)) {
1368+
LLVM_DEBUG({ dbgs() << " Safe jump table detected, skipping.\n"; });
1369+
return std::nullopt;
1370+
}
1371+
13671372
// Returns at most one report per instruction - this is probably OK...
13681373
for (auto Reg : RegsToCheck)
13691374
if (!S.TrustedRegs[Reg])
@@ -1394,6 +1399,11 @@ shouldReportCallGadget(const BinaryContext &BC, const MCInstReference &Inst,
13941399
if (S.SafeToDerefRegs[DestReg])
13951400
return std::nullopt;
13961401

1402+
if (BC.MIB->isSafeJumpTableBranchForPtrAuth(Inst)) {
1403+
LLVM_DEBUG({ dbgs() << " Safe jump table detected, skipping.\n"; });
1404+
return std::nullopt;
1405+
}
1406+
13971407
return make_gadget_report(CallKind, Inst, DestReg);
13981408
}
13991409

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,79 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
545545
return std::nullopt;
546546
}
547547

548+
bool
549+
isSafeJumpTableBranchForPtrAuth(MCInstReference BranchInst) const override {
550+
MCInstReference CurRef = BranchInst;
551+
auto StepBack = [&]() {
552+
do {
553+
auto PredInst = CurRef.getSinglePredecessor();
554+
if (!PredInst)
555+
return false;
556+
CurRef = *PredInst;
557+
} while (isCFI(CurRef));
558+
559+
return true;
560+
};
561+
562+
// Match this contiguous sequence:
563+
// cmp Xm, #count
564+
// csel Xm, Xm, xzr, ls
565+
// adrp Xn, .LJTIxyz
566+
// add Xn, Xn, :lo12:.LJTIxyz
567+
// ldrsw Xm, [Xn, Xm, lsl #2]
568+
// .Ltmp:
569+
// adr Xn, .Ltmp
570+
// add Xm, Xn, Xm
571+
// br Xm
572+
573+
// FIXME: Check label operands of ADR/ADRP+ADD and #count operand of CMP.
574+
575+
using namespace LowLevelInstMatcherDSL;
576+
Reg Xm, Xn;
577+
578+
if (!matchInst(CurRef, AArch64::BR, Xm) || !StepBack())
579+
return false;
580+
581+
if (!matchInst(CurRef, AArch64::ADDXrs, Xm, Xn, Xm, Imm(0)) || !StepBack())
582+
return false;
583+
584+
if (!matchInst(CurRef, AArch64::ADR, Xn /*, .Ltmp*/) || !StepBack())
585+
return false;
586+
587+
if (!matchInst(CurRef, AArch64::LDRSWroX, Xm, Xn, Xm, Imm(0), Imm(1)) ||
588+
!StepBack())
589+
return false;
590+
591+
if (matchInst(CurRef, AArch64::ADR, Xn /*, .LJTIxyz*/)) {
592+
if (!StepBack())
593+
return false;
594+
if (!matchInst(CurRef, AArch64::HINT, Imm(0)) || !StepBack())
595+
return false;
596+
} else if (matchInst(CurRef, AArch64::ADDXri, Xn,
597+
Xn /*, :lo12:.LJTIxyz*/)) {
598+
if (!StepBack())
599+
return false;
600+
if (!matchInst(CurRef, AArch64::ADRP, Xn /*, .LJTIxyz*/) || !StepBack())
601+
return false;
602+
} else {
603+
return false;
604+
}
605+
606+
if (!matchInst(CurRef, AArch64::CSELXr, Xm, Xm, Reg(AArch64::XZR),
607+
Imm(AArch64CC::LS)) ||
608+
!StepBack())
609+
return false;
610+
611+
if (!matchInst(CurRef, AArch64::SUBSXri, Reg(AArch64::XZR),
612+
Xm /*, #count*/))
613+
return false;
614+
615+
// Some platforms treat X16 and X17 as more protected registers, others
616+
// do not make such distinction. So far, accept any registers as Xm and Xn.
617+
618+
return true;
619+
}
620+
548621
bool isADRP(const MCInst &Inst) const override {
549622
return Inst.getOpcode() == AArch64::ADRP;
550623
}

0 commit comments

Comments
 (0)