From 1c575d59c6786a2fd8fac857758bb37763d421b0 Mon Sep 17 00:00:00 2001 From: YongKang Zhu Date: Sat, 22 Feb 2025 16:07:10 -0800 Subject: [PATCH] [BOLT][instr] Avoid WX segment BOLT instrumented binary today has a readable (R), writeable (W) and also executable (X) segment, which Android system won't load due to its WX attribute. Such RWX segment was produced because BOLT has a two step linking, first for everything in the updated or rewritten input binary and next for runtime library. Each linking will layout sections in the order of RX sections followed by RO sections and then followed by RW sections, so we could end up having a RW section `.bolt.instr.counters` surrounded by a number of RO and RX sections, and a new text segment was then formed by including all RX sections which includes the RW section in the middle, and hence the RWX segment. An easy way to fix this is to separate the RW `.bolt.instr.counters` section into its own segment by a). assigning the starting addresses for section `.bolt.instr.counters` and its following section with regular page aligned addresses and b). creating two extra program headers accordingly. --- bolt/include/bolt/Rewrite/RewriteInstance.h | 3 + bolt/lib/Passes/Instrumentation.cpp | 2 +- bolt/lib/Rewrite/RewriteInstance.cpp | 108 +++++++++++++++++--- bolt/test/avoid-wx-segment.c | 15 +++ 4 files changed, 110 insertions(+), 18 deletions(-) create mode 100644 bolt/test/avoid-wx-segment.c diff --git a/bolt/include/bolt/Rewrite/RewriteInstance.h b/bolt/include/bolt/Rewrite/RewriteInstance.h index 42094cb732107..fdd65bbd535f7 100644 --- a/bolt/include/bolt/Rewrite/RewriteInstance.h +++ b/bolt/include/bolt/Rewrite/RewriteInstance.h @@ -505,6 +505,9 @@ class RewriteInstance { /// Number of local symbols in newly written symbol table. uint64_t NumLocalSymbols{0}; + /// Flag indicating runtime library linking just started. + bool StartLinkingRuntimeLib{false}; + /// Information on special Procedure Linkage Table sections. There are /// multiple variants generated by different linkers. struct PLTSectionInfo { diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp index 76766b05b9176..fbf889279f1c0 100644 --- a/bolt/lib/Passes/Instrumentation.cpp +++ b/bolt/lib/Passes/Instrumentation.cpp @@ -604,7 +604,7 @@ Error Instrumentation::runOnFunctions(BinaryContext &BC) { /*IsText=*/false, /*IsAllocatable=*/true); BC.registerOrUpdateSection(".bolt.instr.counters", ELF::SHT_PROGBITS, Flags, - nullptr, 0, 1); + nullptr, 0, BC.RegularPageSize); BC.registerOrUpdateNoteSection(".bolt.instr.tables", nullptr, 0, /*Alignment=*/1, diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 70a9f084f009b..a97762063eb1e 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -628,6 +628,11 @@ Error RewriteInstance::discoverStorage() { unsigned Phnum = Obj.getHeader().e_phnum; Phnum += 3; + // Reserve two more pheaders to avoid having writeable and executable + // segment in instrumented binary. + if (opts::Instrument) + Phnum += 2; + NextAvailableAddress += Phnum * sizeof(ELF64LEPhdrTy); NextAvailableOffset += Phnum * sizeof(ELF64LEPhdrTy); } @@ -2083,6 +2088,13 @@ void RewriteInstance::adjustCommandLineOptions() { opts::HotText = false; } + if (opts::Instrument && opts::UseGnuStack) { + BC->errs() << "BOLT-ERROR: cannot avoid having writeable and executable " + "segment in instrumented binary if program headers will be " + "updated in place\n"; + exit(1); + } + if (opts::HotText && opts::HotTextMoveSections.getNumOccurrences() == 0) { opts::HotTextMoveSections.addValue(".stub"); opts::HotTextMoveSections.addValue(".mover"); @@ -3612,11 +3624,13 @@ void RewriteInstance::emitAndLink() { static_cast(*Streamer).getAssembler()); } - if (RuntimeLibrary *RtLibrary = BC->getRuntimeLibrary()) + if (RuntimeLibrary *RtLibrary = BC->getRuntimeLibrary()) { + StartLinkingRuntimeLib = true; RtLibrary->link(*BC, ToolPath, *Linker, [this](auto MapSection) { // Map newly registered sections. this->mapAllocatableSections(MapSection); }); + } // Once the code is emitted, we can rename function sections to actual // output sections and de-register sections used for emission. @@ -4011,12 +4025,17 @@ void RewriteInstance::mapAllocatableSections( Section.setOutputFileOffset(Section.getInputFileOffset()); MapSection(Section, Section.getAddress()); } else { - NextAvailableAddress = - alignTo(NextAvailableAddress, Section.getAlignment()); + uint64_t Alignment = Section.getAlignment(); + if (opts::Instrument && StartLinkingRuntimeLib) { + Alignment = BC->RegularPageSize; + StartLinkingRuntimeLib = false; + } + NextAvailableAddress = alignTo(NextAvailableAddress, Alignment); + LLVM_DEBUG({ - dbgs() << "BOLT: mapping section " << Section.getName() << " (0x" - << Twine::utohexstr(Section.getAllocAddress()) << ") to 0x" - << Twine::utohexstr(NextAvailableAddress) << ":0x" + dbgs() << "BOLT-DEBUG: mapping section " << Section.getName() + << " (0x" << Twine::utohexstr(Section.getAllocAddress()) + << ") to 0x" << Twine::utohexstr(NextAvailableAddress) << ":0x" << Twine::utohexstr(NextAvailableAddress + Section.getOutputSize()) << '\n'; @@ -4079,6 +4098,9 @@ void RewriteInstance::patchELFPHDRTable() { } } + if (opts::Instrument) + Phnum += 2; + // NOTE Currently .eh_frame_hdr appends to the last segment, recalculate // last segments size based on the NextAvailableAddress variable. if (!NewWritableSegmentSize) { @@ -4093,7 +4115,8 @@ void RewriteInstance::patchELFPHDRTable() { const uint64_t SavedPos = OS.tell(); OS.seek(PHDRTableOffset); - auto createNewTextPhdr = [&]() { + auto createNewPhdrs = [&]() { + SmallVector NewPhdrs; ELF64LEPhdrTy NewPhdr; NewPhdr.p_type = ELF::PT_LOAD; if (PHDRTableAddress) { @@ -4108,20 +4131,67 @@ void RewriteInstance::patchELFPHDRTable() { NewPhdr.p_filesz = NewTextSegmentSize; NewPhdr.p_memsz = NewTextSegmentSize; NewPhdr.p_flags = ELF::PF_X | ELF::PF_R; - if (opts::Instrument) { - // FIXME: Currently instrumentation is experimental and the runtime data - // is emitted with code, thus everything needs to be writable. - NewPhdr.p_flags |= ELF::PF_W; - } NewPhdr.p_align = BC->PageAlign; - return NewPhdr; + if (!opts::Instrument) { + NewPhdrs.push_back(NewPhdr); + } else { + ErrorOr Sec = + BC->getUniqueSectionByName(".bolt.instr.counters"); + assert(Sec && "expected one and only one `.bolt.instr.counters` section"); + const uint64_t Addr = Sec->getOutputAddress(); + const uint64_t Offset = Sec->getOutputFileOffset(); + const uint64_t Size = Sec->getOutputSize(); + assert(Addr > NewPhdr.p_vaddr && + Addr + Size < NewPhdr.p_vaddr + NewPhdr.p_memsz && + "`.bolt.instr.counters` section is expected to be included in the " + "new text sgement"); + + // Set correct size for the previous header since we are breaking the + // new text segment into three segments. + uint64_t Delta = Addr - NewPhdr.p_vaddr; + NewPhdr.p_filesz = Delta; + NewPhdr.p_memsz = Delta; + NewPhdrs.push_back(NewPhdr); + + // Create a program header for a RW segment that includes the + // `.bolt.instr.counters` section only. + ELF64LEPhdrTy NewPhdrRWSegment; + NewPhdrRWSegment.p_type = ELF::PT_LOAD; + NewPhdrRWSegment.p_offset = Offset; + NewPhdrRWSegment.p_vaddr = Addr; + NewPhdrRWSegment.p_paddr = Addr; + NewPhdrRWSegment.p_filesz = Size; + NewPhdrRWSegment.p_memsz = Size; + NewPhdrRWSegment.p_flags = ELF::PF_R | ELF::PF_W; + NewPhdrRWSegment.p_align = BC->RegularPageSize; + NewPhdrs.push_back(NewPhdrRWSegment); + + // Create a program header for a RX segment that includes all the RX + // sections from runtime library. + ELF64LEPhdrTy NewPhdrRXSegment; + NewPhdrRXSegment.p_type = ELF::PT_LOAD; + const uint64_t AddrRX = alignTo(Addr + Size, BC->RegularPageSize); + const uint64_t OffsetRX = alignTo(Offset + Size, BC->RegularPageSize); + const uint64_t SizeRX = NewTextSegmentSize - (AddrRX - NewPhdr.p_paddr); + NewPhdrRXSegment.p_offset = OffsetRX; + NewPhdrRXSegment.p_vaddr = AddrRX; + NewPhdrRXSegment.p_paddr = AddrRX; + NewPhdrRXSegment.p_filesz = SizeRX; + NewPhdrRXSegment.p_memsz = SizeRX; + NewPhdrRXSegment.p_flags = ELF::PF_X | ELF::PF_R; + NewPhdrRXSegment.p_align = BC->RegularPageSize; + NewPhdrs.push_back(NewPhdrRXSegment); + } + + return NewPhdrs; }; auto writeNewSegmentPhdrs = [&]() { if (PHDRTableAddress || NewTextSegmentSize) { - ELF64LE::Phdr NewPhdr = createNewTextPhdr(); - OS.write(reinterpret_cast(&NewPhdr), sizeof(NewPhdr)); + SmallVector NewPhdrs = createNewPhdrs(); + OS.write(reinterpret_cast(NewPhdrs.data()), + sizeof(ELF64LE::Phdr) * NewPhdrs.size()); } if (NewWritableSegmentSize) { @@ -4169,8 +4239,12 @@ void RewriteInstance::patchELFPHDRTable() { } case ELF::PT_GNU_STACK: if (opts::UseGnuStack) { - // Overwrite the header with the new text segment header. - NewPhdr = createNewTextPhdr(); + // Overwrite the header with the new segment header. + assert(!opts::Instrument); + SmallVector NewPhdrs = createNewPhdrs(); + assert(NewPhdrs.size() == 1 && + "expect exactly one program header was created"); + NewPhdr = NewPhdrs[0]; ModdedGnuStack = true; } break; diff --git a/bolt/test/avoid-wx-segment.c b/bolt/test/avoid-wx-segment.c new file mode 100644 index 0000000000000..fcc3eb6e4c640 --- /dev/null +++ b/bolt/test/avoid-wx-segment.c @@ -0,0 +1,15 @@ +// Test bolt instrumentation won't generate a binary with any segment that +// is writable and executable. Basically we want to put `.bolt.instr.counters` +// section into its own segment, separated from its surrounding RX sections. + +// REQUIRES: system-linux + +void foo() {} +void bar() { foo(); } + +// RUN: %clang %cflags -c %s -o %t.o +// RUN: ld.lld -q -o %t.so %t.o -shared --init=foo --fini=foo +// RUN: llvm-bolt --instrument %t.so -o %tt.so +// RUN: llvm-readelf -l %tt.so | FileCheck %s +// CHECK-NOT: RWE +// CHECK: {{[0-9]*}} .bolt.instr.counters {{$}}