[BOLT][binary-analysis] Fix pac-ret scanner's "major limitation"

[bgergely0]

Quoting bolt/docs/BinaryAnalysis.md:

BOLT cannot currently handle functions with cfi_negate_ra_state correctly, i.e. any binaries built with -mbranch-protection=pac-ret. The scanner is meant to be used on specifically such binaries, so this is a major limitation! Work is going on in PR #120064 to fix this.

For pac-ret analysis to work, we don't actually need the whole work in #120064 to be merged.

Some context

The BOLT codebase holds different tools: BOLT itself is used to optimize the layout of a binary, while other tools only need to generate the same internal representation to aid in that process, for example perf2bolt only processes profile information, and does not output an optimized binary.

The llvm-bolt-binary-analysis tool is also such a tool.

This is important, because the .cfi_negate_ra_state Call Frame Instruction needs to be read, tracked, and eventually emitted by tools that produce a binary: this CFI signals to the unwinder if it needs to strip the PAC bits from pointers or not.

This means, that for all other tools, we don't actually have to read it from the binary!

Solution

The solution is fairly simple: we need to pass the ToolName to CFIReaderWriter::fillCFIInfoFor, and branch based on this ToolName.

if (getToolName() == "llvm-bolt") { BC.errs() << "BOLT-ERROR: pointer authentication is not supported ""yet. Please compile ""your target binary using '-mbranch-protection=none'.\n"; exit(1); }

This is a two-birds-one-stone solution: it also adds a user-friendly error message about the lack of PAC support. Previously, users got a fairly arcane error about a unsupported CFI opcode. The new error is actionable, so hopefully it would lead to less confusion while support for PAC is added.

Edit: eventually the ToolName check should look like this:

if (getToolName() == "llvm-bolt") { // adding NegateRAState for llvm-bolt Function.addCFIInstruction( Offset, MCCFIInstruction::createNegateRAState(nullptr)); } // else: not adding NegateRAState

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[bgergely0]

PTAL @kbeyls, @paschalis-mpeis

[llvmbot]

@llvm/pr-subscribers-bolt

Author: Gergely Bálint (bgergely0)

Changes

Quoting bolt/docs/BinaryAnalysis.md:
> BOLT cannot currently handle functions with cfi_negate_ra_state correctly, i.e. any binaries built with -mbranch-protection=pac-ret. The scanner is meant to be used on specifically such binaries, so this is a major limitation! Work is going on in PR #120064 to fix this.

For pac-ret analysis to work, we don't actually need the whole work in #120064 to be merged.

Some context

The BOLT codebase holds different tools: BOLT itself is used to optimize the layout of a binary, while other tools only need to generate the same internal representation to aid in that process, for example perf2bolt only processes profile information, and does not output an optimized binary.

The llvm-bolt-binary-analysis tool is also such a tool.

This is important, because the .cfi_negate_ra_state Call Frame Instruction needs to be read, tracked, and eventually emitted by tools that produce a binary: this CFI signals to the unwinder if it needs to strip the PAC bits from pointers or not.

This means, that for all other tools, we don't actually have to read it from the binary!

Solution

The solution is fairly simple: we need to pass the ToolName to CFIReaderWriter::fillCFIInfoFor, and branch based on this ToolName.

if (getToolName() == "llvm-bolt") { BC.errs() << "BOLT-ERROR: pointer authentication is not supported ""yet. Please compile ""your target binary using '-mbranch-protection=none'.\n"; exit(1); }

This is a two-bird-one-stone solution: it also adds a user-friendly error message about the lack of PAC support. Previously, users got a fairly arcane error about a unsupported CFI opcode. The new error is actionable, so hopefully it would lead to less confusion while support for PAC is added.

---

Full diff: https://github.com/llvm/llvm-project/pull/136664.diff

5 Files Affected:

• (modified) bolt/docs/BinaryAnalysis.md (-6)
• (modified) bolt/include/bolt/Core/Exceptions.h (+5-1)
• (modified) bolt/lib/Core/Exceptions.cpp (+12-3)
• (modified) bolt/lib/Rewrite/RewriteInstance.cpp (+2-1)
• (modified) bolt/test/AArch64/dw_cfa_gnu_window_save.test (+3-3)

diff --git a/bolt/docs/BinaryAnalysis.md b/bolt/docs/BinaryAnalysis.md index 9f0f018980517..b13410cd96355 100644 --- a/bolt/docs/BinaryAnalysis.md+++ b/bolt/docs/BinaryAnalysis.md@@ -180,12 +180,6 @@ The following are current known cases of false negatives: [prototype branch]( https://github.com/llvm/llvm-project/compare/main...kbeyls:llvm-project:bolt-gadget-scanner-prototype). -BOLT cannot currently handle functions with `cfi_negate_ra_state` correctly,-i.e. any binaries built with `-mbranch-protection=pac-ret`. The scanner is meant-to be used on specifically such binaries, so this is a major limitation! Work is-going on in PR [#120064](https://github.com/llvm/llvm-project/pull/120064) to-fix this.- ## How to add your own binary analysis _TODO: this section needs to be written. Ideally, we should have a simple diff --git a/bolt/include/bolt/Core/Exceptions.h b/bolt/include/bolt/Core/Exceptions.h index f10cf776f0943..54e7fc50078da 100644 --- a/bolt/include/bolt/Core/Exceptions.h+++ b/bolt/include/bolt/Core/Exceptions.h@@ -37,7 +37,8 @@ class BinaryFunction; /// BinaryFunction, as well as rewriting CFI sections. class CFIReaderWriter { public: - explicit CFIReaderWriter(BinaryContext &BC, const DWARFDebugFrame &EHFrame);+ explicit CFIReaderWriter(BinaryContext &BC, const DWARFDebugFrame &EHFrame,+ StringRef ToolName); bool fillCFIInfoFor(BinaryFunction &Function) const; @@ -56,9 +57,12 @@ class CFIReaderWriter { const FDEsMap &getFDEs() const { return FDEs; } + StringRef getToolName() const { return ToolName; }+ private: BinaryContext &BC; FDEsMap FDEs; + StringRef ToolName; }; /// Parse an existing .eh_frame and invoke the callback for each diff --git a/bolt/lib/Core/Exceptions.cpp b/bolt/lib/Core/Exceptions.cpp index 0b2e63b8ca6a7..cbb29a05bee20 100644 --- a/bolt/lib/Core/Exceptions.cpp+++ b/bolt/lib/Core/Exceptions.cpp@@ -463,8 +463,10 @@ void BinaryFunction::updateEHRanges() { const uint8_t DWARF_CFI_PRIMARY_OPCODE_MASK = 0xc0; CFIReaderWriter::CFIReaderWriter(BinaryContext &BC, - const DWARFDebugFrame &EHFrame)+ const DWARFDebugFrame &EHFrame,+ StringRef ToolName) : BC(BC) { + this->ToolName = ToolName; // Prepare FDEs for fast lookup for (const dwarf::FrameEntry &Entry : EHFrame.entries()) { const auto *CurFDE = dyn_cast<dwarf::FDE>(&Entry); @@ -632,8 +634,15 @@ bool CFIReaderWriter::fillCFIInfoFor(BinaryFunction &Function) const { // DW_CFA_GNU_window_save and DW_CFA_GNU_NegateRAState just use the same // id but mean different things. The latter is used in AArch64. if (Function.getBinaryContext().isAArch64()) { - Function.addCFIInstruction(- Offset, MCCFIInstruction::createNegateRAState(nullptr));+ // .cfi_negate_ra_state is only needed for tools producing binaries (so+ // BOLT itself). Other BOLT-based tools (perf2bolt, merge-fdata,+ // llvm-bolt-binary-analysis, etc.) can safely drop this CFI.+ if (getToolName() == "llvm-bolt") {+ BC.errs() << "BOLT-ERROR: pointer authentication is not supported "+ "yet. Please compile "+ "your target binary using '-mbranch-protection=none'.\n";+ exit(1);+ } break; } if (opts::Verbosity >= 1) diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 69fb736d7bde0..6dc7cad4f538c 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp+++ b/bolt/lib/Rewrite/RewriteInstance.cpp@@ -2048,7 +2048,8 @@ Error RewriteInstance::readSpecialSections() { Expected<const DWARFDebugFrame *> EHFrameOrError = BC->DwCtx->getEHFrame(); if (!EHFrameOrError) report_error("expected valid eh_frame section", EHFrameOrError.takeError()); - CFIRdWrt.reset(new CFIReaderWriter(*BC, *EHFrameOrError.get()));+ StringRef ToolName = llvm::sys::path::stem(Argv[0]);+ CFIRdWrt.reset(new CFIReaderWriter(*BC, *EHFrameOrError.get(), ToolName)); processSectionMetadata(); diff --git a/bolt/test/AArch64/dw_cfa_gnu_window_save.test b/bolt/test/AArch64/dw_cfa_gnu_window_save.test index 2e044b399720a..97b257ee04666 100644 --- a/bolt/test/AArch64/dw_cfa_gnu_window_save.test+++ b/bolt/test/AArch64/dw_cfa_gnu_window_save.test@@ -1,8 +1,8 @@-# Check that llvm-bolt can handle DW_CFA_GNU_window_save on AArch64.+# Check that llvm-bolt refuses binaries with DW_CFA_GNU_window_save on AArch64. RUN: yaml2obj %p/Inputs/dw_cfa_gnu_window_save.yaml &> %t.exe -RUN: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s+RUN not: llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s CHECK-NOT: paciasp CHECK-NOT: autiasp -CHECK-NOT: ERROR: unable to fill CFI.+CHECK: BOLT-ERROR: pointer authentication is not supported yet.

[bgergely0]

also tagging @atrosinenko as I see you are very active on the pac-ret gadget scanner :)

[atrosinenko]

Thank you very much for unblocking gadget scanner - I think it is a good idea to first fix the "read only mode" and then discuss the general case which is much more complex.

I am rather cautious about matching executable name (though I'm not very familiar with BOLT's core). Maybe it is better to choose the mode of operation based on opts::BinaryAnalysisMode and other such options, see these lines for example. Then, instead of assigning this->ToolName, you could compute this property in constructor and assign it to a boolean field.

[bgergely0]

@atrosinenko Thanks for the review.

Maybe it is better to choose the mode of operation based on opts::BinaryAnalysisMode

Good point, probably better to only change the behaviour for the binary-analysis tool, because the current way it allows all tools besides BOLT. Doing perf2bolt when you cannot do bolt after isn't much help.

[paschalis-mpeis]

Hey Gergely,

Thanks for your work. I think it should be OK to restrict this to both llvm-bolt and perf2bolt. As said, the latter is not affected, but then llvm-bolt won't be able to run regardless.

[paschalis-mpeis]

Looks good, thanks!

Let's allow 1-2 days in case there are additional comments.

[atrosinenko]

@bgergely0 Thank you for the updates! I have no more comments except for one minor nit-picking (feel free to ignore).

I agree with @paschalis-mpeis, let's wait 1-2 days for others to comment.

[paschalis-mpeis]

Could also rename the title to something like:

[BOLT][AArch64] Abort on pac-ret binaries in llvm-bolt and perf2bolt

[kbeyls]

Thank you, this mostly looks good to me!
I just had one nit-pick comment.

I would also improve the title of this PR/commit message to be more descriptive, as @paschalis-mpeis recommended.

[kbeyls]

I would not recommend users to turn of a security hardening feature in an error message.
Maybe it'd be better to instead say something like

"BOLT-ERROR: binaries using pac-ret hardening (e.g. as produced by '-mbranch-protection=pac-ret') are currently not supported by BOLT\n"; 

[paschalis-mpeis]

Noting that this new behavior is triggered on 39 tests on AArch64, causing all to fail.
It will have to be addressed before proceeding.

Total Discovered Tests: 547 Skipped : 13 (2.38%) Unsupported : 81 (14.81%) Passed : 413 (75.50%) Expectedly Failed: 1 (0.18%) Failed : 39 (7.13%)