[BOLT] Gadget scanner: detect non-protected indirect calls #131899
Conversation
atrosinenko
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo,
dcci and
yota9
as code owners
|
@llvm/pr-subscribers-bolt Author: Anatoly Trosinenko (atrosinenko) ChangesPatch is 33.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/131899.diff 4 Files Affected:
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 76ea2489e7038..b3d54ccd5955d 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -577,6 +577,16 @@ class MCPlusBuilder {
return getNoRegister();
}
+ /// Returns the register used as call destination, or no-register, if not
+ /// an indirect call. Sets IsAuthenticatedInternally if the instruction
+ /// accepts signed pointer as its operand and authenticates it internally.
+ virtual MCPhysReg
+ getRegUsedAsCallDest(const MCInst &Inst,
+ bool &IsAuthenticatedInternally) const {
+ llvm_unreachable("not implemented");
+ return getNoRegister();
+ }
+
virtual bool isTerminator(const MCInst &Inst) const;
virtual bool isNoop(const MCInst &Inst) const {
diff --git a/bolt/lib/Passes/PAuthGadgetScanner.cpp b/bolt/lib/Passes/PAuthGadgetScanner.cpp
index ebfa606ceb7c3..bdaf362811662 100644
--- a/bolt/lib/Passes/PAuthGadgetScanner.cpp
+++ b/bolt/lib/Passes/PAuthGadgetScanner.cpp
@@ -382,11 +382,11 @@ class PacRetAnalysis
public:
std::vector<MCInstReference>
- getLastClobberingInsts(const MCInst Ret, BinaryFunction &BF,
- const ArrayRef<MCPhysReg> UsedDirtyRegs) const {
+ getLastClobberingInsts(const MCInst &Inst, BinaryFunction &BF,
+ const ArrayRef<MCPhysReg> UsedDirtyRegs) {
if (RegsToTrackInstsFor.empty())
return {};
- auto MaybeState = getStateAt(Ret);
+ auto MaybeState = getStateBefore(Inst);
if (!MaybeState)
llvm_unreachable("Expected State to be present");
const State &S = *MaybeState;
@@ -434,6 +434,29 @@ static std::shared_ptr<Report> tryCheckReturn(const BinaryContext &BC,
return std::make_shared<GadgetReport>(RetKind, Inst, RetReg);
}
+static std::shared_ptr<Report> tryCheckCall(const BinaryContext &BC,
+ const MCInstReference &Inst,
+ const State &S) {
+ static const GadgetKind CallKind("non-protected call found");
+ if (!BC.MIB->isCall(Inst) && !BC.MIB->isBranch(Inst))
+ return nullptr;
+
+ bool IsAuthenticated = false;
+ MCPhysReg DestReg = BC.MIB->getRegUsedAsCallDest(Inst, IsAuthenticated);
+ if (IsAuthenticated || DestReg == BC.MIB->getNoRegister())
+ return nullptr;
+
+ LLVM_DEBUG({
+ traceInst(BC, "Found call inst", Inst);
+ traceReg(BC, "Call destination reg", DestReg);
+ traceRegMask(BC, "SafeToDerefRegs", S.SafeToDerefRegs);
+ });
+ if (S.SafeToDerefRegs[DestReg])
+ return nullptr;
+
+ return std::make_shared<GadgetReport>(CallKind, Inst, DestReg);
+}
+
FunctionAnalysisResult
Analysis::computeDfState(BinaryFunction &BF,
MCPlusBuilder::AllocatorIdTy AllocatorId) {
@@ -450,10 +473,12 @@ Analysis::computeDfState(BinaryFunction &BF,
for (BinaryBasicBlock &BB : BF) {
for (int64_t I = 0, E = BB.size(); I < E; ++I) {
MCInstReference Inst(&BB, I);
- const State &S = *PRA.getStateAt(Inst);
+ const State &S = *PRA.getStateBefore(Inst);
if (auto Report = tryCheckReturn(BC, Inst, S))
Result.Diagnostics.push_back(Report);
+ if (auto Report = tryCheckCall(BC, Inst, S))
+ Result.Diagnostics.push_back(Report);
}
}
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index d238a1df5c7d7..9ce1514639f95 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -277,6 +277,48 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
}
}
+ MCPhysReg
+ getRegUsedAsCallDest(const MCInst &Inst,
+ bool &IsAuthenticatedInternally) const override {
+ assert(isCall(Inst) || isBranch(Inst));
+ IsAuthenticatedInternally = false;
+
+ switch (Inst.getOpcode()) {
+ case AArch64::B:
+ case AArch64::BL:
+ assert(Inst.getOperand(0).isExpr());
+ return getNoRegister();
+ case AArch64::Bcc:
+ case AArch64::CBNZW:
+ case AArch64::CBNZX:
+ case AArch64::CBZW:
+ case AArch64::CBZX:
+ assert(Inst.getOperand(1).isExpr());
+ return getNoRegister();
+ case AArch64::TBNZW:
+ case AArch64::TBNZX:
+ case AArch64::TBZW:
+ case AArch64::TBZX:
+ assert(Inst.getOperand(2).isExpr());
+ return getNoRegister();
+ case AArch64::BR:
+ case AArch64::BLR:
+ return Inst.getOperand(0).getReg();
+ case AArch64::BRAA:
+ case AArch64::BRAB:
+ case AArch64::BRAAZ:
+ case AArch64::BRABZ:
+ case AArch64::BLRAA:
+ case AArch64::BLRAB:
+ case AArch64::BLRAAZ:
+ case AArch64::BLRABZ:
+ IsAuthenticatedInternally = true;
+ return Inst.getOperand(0).getReg();
+ default:
+ llvm_unreachable("Unhandled call instruction");
+ }
+ }
+
bool isADRP(const MCInst &Inst) const override {
return Inst.getOpcode() == AArch64::ADRP;
}
diff --git a/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s b/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s
new file mode 100644
index 0000000000000..3098e8ec954d2
--- /dev/null
+++ b/bolt/test/binary-analysis/AArch64/gs-pauth-calls.s
@@ -0,0 +1,676 @@
+// RUN: %clang %cflags -march=armv8.3-a %s -o %t.exe
+// RUN: llvm-bolt-binary-analysis --scanners=pacret %t.exe 2>&1 | FileCheck %s
+
+// FIXME In the below test cases, LR is usually not spilled as needed, as it is
+// not checked by BOLT.
+
+ .text
+
+ .globl good_direct_call
+ .type good_direct_call,@function
+good_direct_call:
+// CHECK-NOT: good_direct_call
+ paciasp
+ bl callee_ext
+ autiasp
+ ret
+ .size good_direct_call, .-good_direct_call
+
+ .globl good_indirect_call_arg
+ .type good_indirect_call_arg,@function
+good_indirect_call_arg:
+// CHECK-NOT: good_indirect_call_arg
+ paciasp
+ autia x0, x1
+ blr x0
+ autiasp
+ ret
+ .size good_indirect_call_arg, .-good_indirect_call_arg
+
+ .globl good_indirect_call_mem
+ .type good_indirect_call_mem,@function
+good_indirect_call_mem:
+// CHECK-NOT: good_indirect_call_mem
+ paciasp
+ ldr x16, [x0]
+ autia x16, x0
+ blr x16
+ autiasp
+ ret
+ .size good_indirect_call_mem, .-good_indirect_call_mem
+
+ .globl good_indirect_call_arg_v83
+ .type good_indirect_call_arg_v83,@function
+good_indirect_call_arg_v83:
+// CHECK-NOT: good_indirect_call_arg_v83
+ paciasp
+ blraa x0, x1
+ autiasp
+ ret
+ .size good_indirect_call_arg_v83, .-good_indirect_call_arg_v83
+
+ .globl good_indirect_call_mem_v83
+ .type good_indirect_call_mem_v83,@function
+good_indirect_call_mem_v83:
+// CHECK-NOT: good_indirect_call_mem_v83
+ paciasp
+ ldr x16, [x0]
+ blraa x16, x0
+ autiasp
+ ret
+ .size good_indirect_call_mem_v83, .-good_indirect_call_mem_v83
+
+ .globl bad_indirect_call_arg
+ .type bad_indirect_call_arg,@function
+bad_indirect_call_arg:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_arg, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x0
+// CHECK-NEXT: The 0 instructions that write to the affected registers after any authentication are:
+ paciasp
+ blr x0
+ autiasp
+ ret
+ .size bad_indirect_call_arg, .-bad_indirect_call_arg
+
+ .globl bad_indirect_call_mem
+ .type bad_indirect_call_mem,@function
+bad_indirect_call_mem:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: ldr x16, [x0]
+// CHECK-NEXT: This happens in the following basic block:
+// CHECK-NEXT: {{[0-9a-f]+}}: paciasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ldr x16, [x0]
+// CHECK-NEXT: {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: {{[0-9a-f]+}}: autiasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ret
+ paciasp
+ ldr x16, [x0]
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem, .-bad_indirect_call_mem
+
+ .globl bad_indirect_call_arg_clobber
+ .type bad_indirect_call_arg_clobber,@function
+bad_indirect_call_arg_clobber:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_arg_clobber, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x0
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: mov w0, w2
+// CHECK-NEXT: This happens in the following basic block:
+// CHECK-NEXT: {{[0-9a-f]+}}: paciasp
+// CHECK-NEXT: {{[0-9a-f]+}}: autia x0, x1
+// CHECK-NEXT: {{[0-9a-f]+}}: mov w0, w2
+// CHECK-NEXT: {{[0-9a-f]+}}: blr x0
+// CHECK-NEXT: {{[0-9a-f]+}}: autiasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ret
+ paciasp
+ autia x0, x1
+ mov w0, w2
+ blr x0
+ autiasp
+ ret
+ .size bad_indirect_call_arg_clobber, .-bad_indirect_call_arg_clobber
+
+ .globl bad_indirect_call_mem_clobber
+ .type bad_indirect_call_mem_clobber,@function
+bad_indirect_call_mem_clobber:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem_clobber, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: mov w16, w2
+// CHECK-NEXT: This happens in the following basic block:
+// CHECK-NEXT: {{[0-9a-f]+}}: paciasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ldr x16, [x0]
+// CHECK-NEXT: {{[0-9a-f]+}}: autia x16, x0
+// CHECK-NEXT: {{[0-9a-f]+}}: mov w16, w2
+// CHECK-NEXT: {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: {{[0-9a-f]+}}: autiasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ret
+ paciasp
+ ldr x16, [x0]
+ autia x16, x0
+ mov w16, w2
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem_clobber, .-bad_indirect_call_mem_clobber
+
+ .globl good_indirect_call_mem_chain_of_auts
+ .type good_indirect_call_mem_chain_of_auts,@function
+good_indirect_call_mem_chain_of_auts:
+// CHECK-NOT: good_indirect_call_mem_chain_of_auts
+ paciasp
+ ldr x16, [x0]
+ autia x16, x1
+ ldr x16, [x16]
+ autia x16, x0
+ blr x16
+ autiasp
+ ret
+ .size good_indirect_call_mem_chain_of_auts, .-good_indirect_call_mem_chain_of_auts
+
+ .globl bad_indirect_call_mem_chain_of_auts
+ .type bad_indirect_call_mem_chain_of_auts,@function
+bad_indirect_call_mem_chain_of_auts:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem_chain_of_auts, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: ldr x16, [x16]
+// CHECK-NEXT: This happens in the following basic block:
+// CHECK-NEXT: {{[0-9a-f]+}}: paciasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ldr x16, [x0]
+// CHECK-NEXT: {{[0-9a-f]+}}: autia x16, x1
+// CHECK-NEXT: {{[0-9a-f]+}}: ldr x16, [x16]
+// CHECK-NEXT: {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: {{[0-9a-f]+}}: autiasp
+// CHECK-NEXT: {{[0-9a-f]+}}: ret
+ paciasp
+ ldr x16, [x0]
+ autia x16, x1
+ ldr x16, [x16]
+ // Missing AUT of x16. The fact that x16 was authenticated above has nothing to do with it.
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem_chain_of_auts, .-bad_indirect_call_mem_chain_of_auts
+
+// Multi-BB test cases.
+//
+// Positive ("good") test cases are designed so that the register is made safe
+// in one BB and used in the other. Negative ("bad") ones are designed so that
+// there are two predecessors, one of them ends with the register in a safe
+// state and the other ends with that register being unsafe.
+
+ .globl good_indirect_call_arg_multi_bb
+ .type good_indirect_call_arg_multi_bb,@function
+good_indirect_call_arg_multi_bb:
+// CHECK-NOT: good_indirect_call_arg_multi_bb
+ paciasp
+ autia x0, x1
+ cbz x2, 1f
+ blr x0
+1:
+ ldr x1, [x0]
+ autiasp
+ ret
+ .size good_indirect_call_arg_multi_bb, .-good_indirect_call_arg_multi_bb
+
+ .globl good_indirect_call_mem_multi_bb
+ .type good_indirect_call_mem_multi_bb,@function
+good_indirect_call_mem_multi_bb:
+// CHECK-NOT: good_indirect_call_mem_multi_bb
+ paciasp
+ ldr x16, [x0]
+ autia x16, x0
+ cbz x2, 1f
+ blr x16
+1:
+ ldr w0, [x16]
+ autiasp
+ ret
+ .size good_indirect_call_mem_multi_bb, .-good_indirect_call_mem_multi_bb
+
+ .globl bad_indirect_call_arg_multi_bb
+ .type bad_indirect_call_arg_multi_bb,@function
+bad_indirect_call_arg_multi_bb:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_arg_multi_bb, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x0
+// CHECK-NEXT: The 0 instructions that write to the affected registers after any authentication are:
+ paciasp
+ cbz x2, 1f
+ autia x0, x1
+1:
+ blr x0
+ autiasp
+ ret
+ .size bad_indirect_call_arg_multi_bb, .-bad_indirect_call_arg_multi_bb
+
+ .globl bad_indirect_call_mem_multi_bb
+ .type bad_indirect_call_mem_multi_bb,@function
+bad_indirect_call_mem_multi_bb:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem_multi_bb, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: ldr x16, [x0]
+ paciasp
+ ldr x16, [x0]
+ cbz x2, 1f
+ autia x16, x1
+1:
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem_multi_bb, .-bad_indirect_call_mem_multi_bb
+
+ .globl bad_indirect_call_arg_clobber_multi_bb
+ .type bad_indirect_call_arg_clobber_multi_bb,@function
+bad_indirect_call_arg_clobber_multi_bb:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_arg_clobber_multi_bb, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x0
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: mov w0, w3
+ paciasp
+ autia x0, x1
+ cbz x2, 1f
+ mov w0, w3
+1:
+ blr x0
+ autiasp
+ ret
+ .size bad_indirect_call_arg_clobber_multi_bb, .-bad_indirect_call_arg_clobber_multi_bb
+
+ .globl bad_indirect_call_mem_clobber_multi_bb
+ .type bad_indirect_call_mem_clobber_multi_bb,@function
+bad_indirect_call_mem_clobber_multi_bb:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem_clobber_multi_bb, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: mov w16, w2
+ paciasp
+ ldr x16, [x0]
+ autia x16, x0
+ cbz x2, 1f
+ mov w16, w2
+1:
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem_clobber_multi_bb, .-bad_indirect_call_mem_clobber_multi_bb
+
+ .globl good_indirect_call_mem_chain_of_auts_multi_bb
+ .type good_indirect_call_mem_chain_of_auts_multi_bb,@function
+good_indirect_call_mem_chain_of_auts_multi_bb:
+// CHECK-NOT: good_indirect_call_mem_chain_of_auts_multi_bb
+ paciasp
+ ldr x16, [x0]
+ autia x16, x1
+ ldr x16, [x16]
+ autia x16, x0
+ cbz x2, 1f
+ blr x16
+1:
+ ldr w0, [x16]
+ autiasp
+ ret
+ .size good_indirect_call_mem_chain_of_auts_multi_bb, .-good_indirect_call_mem_chain_of_auts_multi_bb
+
+ .globl bad_indirect_call_mem_chain_of_auts_multi_bb
+ .type bad_indirect_call_mem_chain_of_auts_multi_bb,@function
+bad_indirect_call_mem_chain_of_auts_multi_bb:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_call_mem_chain_of_auts_multi_bb, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: blr x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: ldr x16, [x16]
+ paciasp
+ ldr x16, [x0]
+ autia x16, x1
+ ldr x16, [x16]
+ cbz x2, 1f
+ autia x16, x0
+1:
+ blr x16
+ autiasp
+ ret
+ .size bad_indirect_call_mem_chain_of_auts_multi_bb, .-bad_indirect_call_mem_chain_of_auts_multi_bb
+
+// Test that noreturn function calls via BR are checked as well.
+
+ .globl good_indirect_noreturn_call
+ .type good_indirect_noreturn_call,@function
+good_indirect_noreturn_call:
+// CHECK-NOT: good_indirect_noreturn_call
+ paciasp
+ cbz x0, 2f
+ autiasp
+ ldr w1, [x30]
+ autia x0, x1
+ br x0
+2:
+ autiasp
+ ret
+ .size good_indirect_noreturn_call, .-good_indirect_noreturn_call
+
+ .globl bad_indirect_noreturn_call
+ .type bad_indirect_noreturn_call,@function
+bad_indirect_noreturn_call:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_noreturn_call, basic block .LFT{{[0-9]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: br x0
+// CHECK-NEXT: The 0 instructions that write to the affected registers after any authentication are:
+ paciasp
+ cbz x0, 2f
+ br x0
+2:
+ autiasp
+ ret
+ .size bad_indirect_noreturn_call, .-bad_indirect_noreturn_call
+
+// Test tail calls. To somewhat decrease the number of test cases and not
+// duplicate all of the above, only implement "mem" variant of test cases and
+// mostly test negative cases.
+
+ .globl good_direct_tailcall
+ .type good_direct_tailcall,@function
+good_direct_tailcall:
+// CHECK-NOT: good_direct_tailcall
+ b callee_ext
+ .size good_direct_tailcall, .-good_direct_tailcall
+
+ .globl good_indirect_tailcall_mem
+ .type good_indirect_tailcall_mem,@function
+good_indirect_tailcall_mem:
+// CHECK-NOT: good_indirect_tailcall_mem
+ ldr x16, [x0]
+ autia x16, x0
+ br x16
+ .size good_indirect_tailcall_mem, .-good_indirect_tailcall_mem
+
+ .globl good_indirect_tailcall_mem_v83
+ .type good_indirect_tailcall_mem_v83,@function
+good_indirect_tailcall_mem_v83:
+// CHECK-NOT: good_indirect_tailcall_mem_v83
+ ldr x16, [x0]
+ braa x16, x0
+ .size good_indirect_tailcall_mem_v83, .-good_indirect_tailcall_mem_v83
+
+ .globl bad_indirect_tailcall_mem
+ .type bad_indirect_tailcall_mem,@function
+bad_indirect_tailcall_mem:
+// CHECK-LABEL: GS-PAUTH: non-protected call found in function bad_indirect_tailcall_mem, basic block {{[^,]+}}, at address
+// CHECK-NEXT: The instruction is {{[0-9a-f]+}}: br x16
+// CHECK-NEXT: The 1 instructions that write to the affected registers after any authentication are:
+// CHECK-NEXT: 1. {{[0-9a-f]+}}: ldr x16, [x0]
+// CHECK-NEXT: This happens in the following ...
[truncated]
|
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
from
51bdcad to
81acf8b
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-reformulate-dfa-state
branch
from
aa9215f to
7fecfc4
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
from
81acf8b to
fa84405
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-reformulate-dfa-state
branch
from
7fecfc4 to
9de64af
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
2 times, most recently
from
f5a03e6 to
d99a5b0
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-reformulate-dfa-state
branch
2 times, most recently
from
709008c to
479bf68
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
2 times, most recently
from
0b39fd4 to
bbb379d
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-reformulate-dfa-state
branch
from
479bf68 to
7e77ccb
Compare
| getRegUsedAsCallDest(const MCInst &Inst, | ||
| bool &IsAuthenticatedInternally) const override { |
There was a problem hiding this comment.
I'm wondering if this could be adapt so that it only needs to handle indirect calls?
That would make the switch statement simpler, and also easier to maintain, because it won't need to handle all branch instructions.
For example, at the moment, it seems the switch statement is not handling the newly introduced (in armv9.5) Compare and Branch instructions, see https://developer.arm.com/documentation/ddi0602/2024-09/Base-Instructions/CB-cc---register---Compare-registers-and-branch-
My understanding is that only indirect calls need to be checked, not direct calls.
There was a problem hiding this comment.
Thanks for pointing this out! Non-indirect control flow instructions are inherently irrelevant here.
There was a problem hiding this comment.
isIndirectCall() would help but I think it needs updating.
There was a problem hiding this comment.
Opened a separate PR #133227 on updating isIndirectCall().
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
from
bbb379d to
126800a
Compare
atrosinenko
force-pushed
the
users/atrosinenko/bolt-gs-analyze-indirect-calls
branch
from
126800a to
2d82b35
Compare
| MCPhysReg | ||
| getRegUsedAsCallDest(const MCInst &Inst, | ||
| bool &IsAuthenticatedInternally) const override { | ||
| assert(isCall(Inst) || isBranch(Inst)); |
There was a problem hiding this comment.
I guess this assert could be made more strict now to only allow indirect calls and indirect branches?
There was a problem hiding this comment.
That'd be more consistent with getRegUsedAsRetDest.
Sorry about the noise: GitHub showed this duplicated, so I tried to delete one, and then it deleted both.
| IsAuthenticatedInternally = true; | ||
| return Inst.getOperand(0).getReg(); | ||
| default: | ||
| if (isIndirectCall(Inst) || isIndirectBranch(Inst)) |
There was a problem hiding this comment.
As noted on another thread, I think isIndirectCall() needs updating, but then it'll work as an assertion (and a gate before this is called).
There was a problem hiding this comment.
Assuming changing isIndirectCall() may affect something else, I opened a separate PR #133227. If it is OK to keep this PR as-is and improve it in a follow-up, I will update #133227 after merging this PR and clear its draft status. I did not add that PR in this stack, as it doesn't seem to block anything, it is purely a cleanup.
There was a problem hiding this comment.
I think either way is fine. Let's not block progress on discussing which order these PRs should land in. Please go with the order that makes most sense to you @atrosinenko .
There was a problem hiding this comment.
IIUC, #133227 currently only corrects the implementation of the isIndirectCall function, but doesn't update this area of the code to make use of that updated isIndirectCall function?
What is your plan to then update this area of the code based on the corrected implementation of isIndirectCall in #133227?
I think that not clearly knowing the plan to get this area of the code cleaned up after merging #133227 is the only reason why I'm not approving this PR yet...
There was a problem hiding this comment.
My idea is to update #133227 (which targets the main branch) after merging this PR and then mark it as ready for review. The initial version was uploaded more as a reminder to myself - that is the reason why it is created as a draft PR. As far as I understand, opening a draft PR does not automatically notify anyone, so it should be harmless from the perspective of disturbing the reviewers.
Note that #133227 is targeted to main branch, thus its merge target does not have the changes from this PR yet. Another approach could be to manually stack that PR on top of this one instead of main (or maybe Graphite supports arbitrary trees of PRs) - I just didn't tried to make everything as efficient as possible, but just to have a reminder and not to disturb the reviewers unless #133227 is finally ready.
There was a problem hiding this comment.
What happens if -fno-plt is used? It appears to have no effect on AArch64 Clang, but GCC supports it, and it generates an inline GOT load followed by an indirect branch:
adrp x16, ...
ldr x16, [x16, ...]
br x16
That's actually fine as long as the GOT is RELRO, but I don't think this scanner could distinguish it.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
I guess it should be possible to make BOLT understand this (by adding another register state: "non-signed, points to read-only memory"), but, of course, it would not work out-of-the-box. |
There was a problem hiding this comment.
As written in one of the specific comments, I'm fine with either this PR or #133227 landing first, whatever is most convenient to @atrosinenko .
I don't think there's anything I'd still would want to see changed in the patch before merging.
I'm not fully sure if @jacobbramley still wanted to see some changes before merging this patch?
| IsAuthenticatedInternally = true; | ||
| return Inst.getOperand(0).getReg(); | ||
| default: | ||
| if (isIndirectCall(Inst) || isIndirectBranch(Inst)) |
There was a problem hiding this comment.
I think either way is fine. Let's not block progress on discussing which order these PRs should land in. Please go with the order that makes most sense to you @atrosinenko .
The only thing bothering me is the The other outstanding threads are about LGTM, thanks! |
Implement the detection of non-protected indirect calls and branches similar to pac-ret scanner.