llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-bolt Author: Gergely Bálint (bgergely0) <details> <summary>Changes</summary> This patch adds the patchPLTEntryForBTI to enable patching PLT entries generated by LLD. Context: To keep BTI consistent, targets of stubs inserted in LongJmp need to be patched. As PLTs are not optimized and emitted by BOLT, this patch adds a helper for patching them in the original location. For PLTs generated by LLD, this is safe as LLD inserts extra nops to PLTs which don't already contain a BTI. PLT entry before patching: adrp x16, Page(&(.got.plt[n])) ldr x17, [x16, Offset(&(.got.plt[n]))] add x16, x16, Offset(&(.got.plt[n])) br x17 nop nop PLT entry after patching: bti c adrp x16, Page(&(.got.plt[n])) ldr x17, [x16, Offset(&(.got.plt[n]))] add x16, x16, Offset(&(.got.plt[n])) br x17 nop Safety considerations: The PLT entry can become incorrect if shifting the ADRP moves it across a page boundary. The PLT entry is 24 bytes, and page size is 4096 (or 16384) bytes. Their GCD is 8 bytes, meaning that shifting the ADRP is safe, as long as it's shifted less than 8 bytes. The introduced function only shifts the ADRP by one instruction (4 bytes), meaning there is no need to recompute the ADRP offset. --- Full diff: https://github.com/llvm/llvm-project/pull/173245.diff 4 Files Affected: - (modified) bolt/include/bolt/Core/MCPlusBuilder.h (+4) - (modified) bolt/lib/Passes/LongJmp.cpp (+5) - (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (+61) - (modified) bolt/test/runtime/AArch64/long-jmp-bti-plt.c (+33-4) ``````````diff diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index 31bf73c839c3a..fdd86deea4c5d 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -1784,6 +1784,10 @@ class MCPlusBuilder { return 0; } + virtual void patchPLTEntryForBTI(BinaryFunction &PLTFunction, MCInst &Call) { + llvm_unreachable("not implemented"); + } + virtual bool analyzeVirtualMethodCall(InstructionIterator Begin, InstructionIterator End, std::vector<MCInst *> &MethodFetchInsns, diff --git a/bolt/lib/Passes/LongJmp.cpp b/bolt/lib/Passes/LongJmp.cpp index 9fedf919dd489..798e1ba08918a 100644 --- a/bolt/lib/Passes/LongJmp.cpp +++ b/bolt/lib/Passes/LongJmp.cpp @@ -499,6 +499,11 @@ Error LongJmpPass::relaxStub(BinaryBasicBlock &StubBB, bool &Modified) { << RealTargetSym->getName() << "\n"; exit(1); } + if (TargetFunction && TargetFunction->isPLTFunction()) { + BC.MIB->patchPLTEntryForBTI(*TargetFunction, + *StubBB.getLastNonPseudoInstr()); + return; + } if (TargetFunction && TargetFunction->isIgnored()) { // Includes PLT functions. BC.errs() << "BOLT-ERROR: Cannot add BTI landing pad to ignored function " diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp index 03fb4ddc2f238..a56cf0fc6d9cb 100644 --- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp @@ -1669,6 +1669,67 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { return Base + Offset; } + /// PLT entry before patching: + /// + /// adrp x16, Page(&(.got.plt[n])) + /// ldr x17, [x16, Offset(&(.got.plt[n]))] + /// add x16, x16, Offset(&(.got.plt[n])) + /// br x17 + /// nop + /// nop + /// + /// PLT entry after patching: + /// + /// bti c + /// adrp x16, Page(&(.got.plt[n])) + /// ldr x17, [x16, Offset(&(.got.plt[n]))] + /// add x16, x16, Offset(&(.got.plt[n])) + /// br x17 + /// nop + /// + /// Safety considerations: + /// + /// The PLT entry will become incorrect if shifting the ADRP by one + /// instruction (4 bytes) moves it across a page boundary. + /// + /// The PLT entry is 24 bytes, and page size is 4096 (or 16384) bytes. + /// Their GCD is 8 bytes, meaning that shifting the ADRP is safe, as long as + /// it is shifted by less than 8 bytes. + /// + /// If the PLT entry does not contain extra nops, this function will create an + /// error. This can happen in binaries linked using BFD. + void patchPLTEntryForBTI(BinaryFunction &PLTFunction, MCInst &Call) override { + BinaryContext &BC = PLTFunction.getBinaryContext(); + assert(PLTFunction.isPLTFunction() && + "patchPLTEntryForBTI called on a non-PLT function"); + // Checking if the PLT entry already starts with the BTI needed for Call. + auto FirstBBI = PLTFunction.begin(); + auto FirstII = FirstBBI->begin(); + assert(FirstII != FirstBBI->end() && "Cannot patch empty PLT entry"); + if (isCallCoveredByBTI(Call, *FirstII)) + return; + // Checking if there are extra nops at the end. If not, BOLT cannot patch + // the PLT entry. + auto LastBBI = std::prev(PLTFunction.end()); + auto LastII = std::prev(LastBBI->end()); + if (!isNoop(*LastII)) { + errs() << "BOLT-ERROR: Cannot patch PLT entry " + << PLTFunction.getPrintName() + << " to have a BTI landing pad. Relink the workload using LLD.\n"; + exit(1); + } + // If the PLT does not have a BTI, and it has nops, create a new instruction + // sequence to patch the entry with. + InstructionListType NewPLTSeq; + MCInst BTIInst; + createBTI(BTIInst, BTIKind::C); + NewPLTSeq.push_back(BTIInst); + for (auto II = FirstBBI->begin(); II != FirstBBI->end(); ++II) { + NewPLTSeq.push_back(*II); + } + BC.createInstructionPatch(PLTFunction.getAddress(), NewPLTSeq); + } + unsigned getInvertedBranchOpcode(unsigned Opcode) const { switch (Opcode) { default: diff --git a/bolt/test/runtime/AArch64/long-jmp-bti-plt.c b/bolt/test/runtime/AArch64/long-jmp-bti-plt.c index 6d3e1e7a3e820..4e8aebf6c4f53 100644 --- a/bolt/test/runtime/AArch64/long-jmp-bti-plt.c +++ b/bolt/test/runtime/AArch64/long-jmp-bti-plt.c @@ -1,5 +1,13 @@ -/* This test checks that LongJmp can (not) add BTI instructions to PLT entries - when targeting them using stubs. +// This test checks that LongJmp can add BTI instructions to PLT entries when +// targeting them using stubs. +// +// The test uses a non-default layout, where the .plt section is placed before +// the text section. This is needed so the new code placed under the .data +// section is far enough from the PLt to trigger shortJmp stubs. +// +// In practice, the PLT section can also be placed after the original text +// section. In this scenario, the text section has to be large enough to trigger +// shortJmp stubs. // REQUIRES: system-linux @@ -8,10 +16,31 @@ // RUN: %clang --target=aarch64-unknown-linux-gnu -mbranch-protection=standard \ // RUN: -no-pie %t/bti-plt.c -o %t.exe -Wl,-q -fuse-ld=lld \ // RUN: -Wl,-T,%t/link.ld -Wl,-z,force-bti -// RUN: not llvm-bolt %t.exe -o %t.bolt 2>&1 | FileCheck %s +// RUN: llvm-bolt %t.exe -o %t.bolt | FileCheck %s // CHECK: BOLT-INFO: binary is using BTI -// CHECK: BOLT-ERROR: Cannot add BTI landing pad to ignored function abort@PLT +// Checking PLT entries before running BOLT +// RUN: llvm-objdump -d -j .plt %t.exe | FileCheck %s --check-prefix=CHECK-EXE +// CHECK-EXE: <abort@plt> +// CHECK-EXE-NEXT: adrp x16, 0x8230000 +// CHECK-EXE-NEXT: ldr x17, [x16, #0xaf0] +// CHECK-EXE-NEXT: add x16, x16, #0xaf0 +// CHECK-EXE-NEXT: br x17 +// CHECK-EXE-NEXT: nop +// CHECK-EXE-NEXT: nop + +// Checking PLT entries after patching them in BOLT +// RUN: llvm-objdump -d -j .plt %t.bolt | FileCheck %s \ +// RUN: --check-prefix=CHECK-BOLT +// CHECK-BOLT: <abort@plt> +// CHECK-BOLT-NEXT: bti c +// CHECK-BOLT-NEXT: adrp x16, 0x8230000 +// CHECK-BOLT-NEXT: ldr x17, [x16, #0xaf0] +// CHECK-BOLT-NEXT: add x16, x16, #0xaf0 +// CHECK-BOLT-NEXT: br x17 +// CHECK-BOLT-NEXT: nop + +/* #--- link.ld SECTIONS { `````````` </details> https://github.com/llvm/llvm-project/pull/173245 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
