Skip to content

[BOLT] Gadget scanner: detect non-protected indirect calls #131899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 3, 2025
+923 −31
Merged

[BOLT] Gadget scanner: detect non-protected indirect calls #131899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d82b35
[BOLT] Gadget scanner: detect non-protected indirect calls
atrosinenko Mar 18, 2025
9074fad
Add a separate scanner category: pauth
atrosinenko Mar 26, 2025
a1516bb
Tests: spill LR, format, drop redundant cases
atrosinenko Mar 26, 2025
bbd4791
Streamline getRegUsedAsCallDest()
atrosinenko Mar 26, 2025
8865d1a
Address the review comments
atrosinenko Mar 26, 2025
656e100
Fix formatting
atrosinenko Mar 26, 2025
c9bc2c4
Tests: use the correct register
atrosinenko Mar 27, 2025
7324b6a
Tests: define `callee` function instead of referencing `callee_ext`
atrosinenko Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions bolt/include/bolt/Core/MCPlusBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 a 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 {
Expand Down
6 changes: 5 additions & 1 deletion bolt/include/bolt/Passes/PAuthGadgetScanner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,9 @@ struct FunctionAnalysisResult {
};

class Analysis : public BinaryFunctionPass {
/// Only search for pac-ret violations.
bool PacRetGadgetsOnly;

void runOnFunction(BinaryFunction &Function,
MCPlusBuilder::AllocatorIdTy AllocatorId);
FunctionAnalysisResult findGadgets(BinaryFunction &BF,
Expand All @@ -261,7 +264,8 @@ class Analysis : public BinaryFunctionPass {
std::mutex AnalysisResultsMutex;

public:
explicit Analysis() : BinaryFunctionPass(false) {}
explicit Analysis(bool PacRetGadgetsOnly)
: BinaryFunctionPass(false), PacRetGadgetsOnly(PacRetGadgetsOnly) {}

const char *getName() const override { return "pauth-gadget-scanner"; }

Expand Down
4 changes: 2 additions & 2 deletions bolt/include/bolt/Utils/CommandLineOpts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ extern llvm::cl::opt<unsigned> Verbosity;
/// Return true if we should process all functions in the binary.
bool processAllFunctions();

enum GadgetScannerKind { GS_PACRET, GS_ALL };
enum GadgetScannerKind { GS_PACRET, GS_PAUTH, GS_ALL };

extern llvm::cl::list<GadgetScannerKind> GadgetScannersToRun;
extern llvm::cl::bits<GadgetScannerKind> GadgetScannersToRun;

} // namespace opts

Expand Down
37 changes: 33 additions & 4 deletions bolt/lib/Passes/PAuthGadgetScanner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,11 +401,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;
Expand Down Expand Up @@ -453,6 +453,29 @@ shouldReportReturnGadget(const BinaryContext &BC, const MCInstReference &Inst,
return std::make_shared<GadgetReport>(RetKind, Inst, RetReg);
}

static std::shared_ptr<Report>
shouldReportCallGadget(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::findGadgets(BinaryFunction &BF,
MCPlusBuilder::AllocatorIdTy AllocatorId) {
Expand All @@ -469,7 +492,7 @@ Analysis::findGadgets(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 non-empty state was never propagated from the entry basic block
// to Inst, assume it to be unreachable and report a warning.
Expand All @@ -481,6 +504,12 @@ Analysis::findGadgets(BinaryFunction &BF,

if (auto Report = shouldReportReturnGadget(BC, Inst, S))
Result.Diagnostics.push_back(Report);

if (PacRetGadgetsOnly)
continue;

if (auto Report = shouldReportCallGadget(BC, Inst, S))
Result.Diagnostics.push_back(Report);
}
}
return Result;
Expand Down
32 changes: 20 additions & 12 deletions bolt/lib/Rewrite/RewriteInstance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,14 @@ static cl::opt<bool> WriteBoltInfoSection(
"bolt-info", cl::desc("write bolt info section in the output binary"),
cl::init(true), cl::Hidden, cl::cat(BoltOutputCategory));

cl::list<GadgetScannerKind>
GadgetScannersToRun("scanners", cl::desc("which gadget scanners to run"),
cl::values(clEnumValN(GS_PACRET, "pacret", "pac-ret"),
clEnumValN(GS_ALL, "all", "all")),
cl::ZeroOrMore, cl::CommaSeparated,
cl::cat(BinaryAnalysisCategory));
cl::bits<GadgetScannerKind> GadgetScannersToRun(
"scanners", cl::desc("which gadget scanners to run"),
cl::values(
clEnumValN(GS_PACRET, "pacret",
"pac-ret: return address protection (subset of \"pauth\")"),
clEnumValN(GS_PAUTH, "pauth", "All Pointer Authentication scanners"),
clEnumValN(GS_ALL, "all", "All implemented scanners")),
cl::ZeroOrMore, cl::CommaSeparated, cl::cat(BinaryAnalysisCategory));

} // namespace opts

Expand Down Expand Up @@ -3539,12 +3541,18 @@ void RewriteInstance::runBinaryAnalyses() {
// FIXME: add a pass that warns about which functions do not have CFG,
// and therefore, analysis is most likely to be less accurate.
using GSK = opts::GadgetScannerKind;
// if no command line option was given, act as if "all" was specified.
if (opts::GadgetScannersToRun.empty())
opts::GadgetScannersToRun.addValue(GSK::GS_ALL);
for (GSK ScannerToRun : opts::GadgetScannersToRun) {
if (ScannerToRun == GSK::GS_PACRET || ScannerToRun == GSK::GS_ALL)
Manager.registerPass(std::make_unique<PAuthGadgetScanner::Analysis>());
using PAuthScanner = PAuthGadgetScanner::Analysis;

// If no command line option was given, act as if "all" was specified.
bool RunAll = !opts::GadgetScannersToRun.getBits() ||
opts::GadgetScannersToRun.isSet(GSK::GS_ALL);

if (RunAll || opts::GadgetScannersToRun.isSet(GSK::GS_PAUTH)) {
Manager.registerPass(
std::make_unique<PAuthScanner>(/*OnlyPacRetChecks=*/false));
} else if (RunAll || opts::GadgetScannersToRun.isSet(GSK::GS_PACRET)) {
Manager.registerPass(
std::make_unique<PAuthScanner>(/*OnlyPacRetChecks=*/true));
}

BC->logBOLTErrorsAndQuitOnFatal(Manager.runPasses());
Expand Down
27 changes: 27 additions & 0 deletions bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,33 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
}
}

MCPhysReg
getRegUsedAsCallDest(const MCInst &Inst,
bool &IsAuthenticatedInternally) const override {
Comment on lines +281 to +282
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out! Non-indirect control flow instructions are inherently irrelevant here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isIndirectCall() would help but I think it needs updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a separate PR #133227 on updating isIndirectCall().

assert(isCall(Inst) || isBranch(Inst));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this assert could be made more strict now to only allow indirect calls and indirect branches?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = false;

switch (Inst.getOpcode()) {
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:
if (isIndirectCall(Inst) || isIndirectBranch(Inst))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

llvm_unreachable("Unhandled indirect branch");
return getNoRegister();
}
}

bool isADRP(const MCInst &Inst) const override {
return Inst.getOpcode() == AArch64::ADRP;
}
Expand Down
5 changes: 3 additions & 2 deletions bolt/test/binary-analysis/AArch64/cmdline-args.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ HELP-EMPTY:
HELP-NEXT: BinaryAnalysis options:
HELP-EMPTY:
HELP-NEXT: --scanners=<value> - which gadget scanners to run
HELP-NEXT: =pacret - pac-ret
HELP-NEXT: =all - all
HELP-NEXT: =pacret - pac-ret: return address protection (subset of "pauth")
HELP-NEXT: =pauth - All Pointer Authentication scanners
HELP-NEXT: =all - All implemented scanners
HELP-EMPTY:
HELP-NEXT: Generic Options:
Loading