From bd120491cb2d90e720c44fec232658bd6431117c Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Wed, 29 Apr 2026 21:37:53 -0700 Subject: [PATCH] rewriteSections: rewrite executables like shared objects (#622) Patching a non-PIE (ET_EXEC) binary so that the program header table grows made the executable rewrite path shift every LOAD segment's p_vaddr down by one or more pages, producing LOAD segments below the input's lowest p_vaddr. On systems that enforce vm.mmap_min_addr at that floor (e.g. riscv64-linux at 0x10000) the kernel rejects the fixed-address mmap at exec time and the patched binary fails to load. Shared objects already avoid this: rewriteSectionsLibrary appends the rewritten sections in a fresh PT_LOAD at the end of the file, leaving every existing LOAD's vaddr unchanged. Route ET_EXEC through the same path and drop the executable-specific rewriteSectionsExecutable. The program header table stays at its canonical front offset, which executables need: loaders such as qemu-user (and the dynamic linkers it runs) compute AT_PHDR as (lowest LOAD vaddr) + e_phoff, so a PHT moved to a tail segment with a different vaddr/offset skew would be found at the wrong address. Also harden buildResolutionCache to report an already-present cache note loudly even when .dynamic/.dynstr are too malformed to parse, instead of erroring on the section first. Adds no-vaddr-underrun.sh (asserts the post-patch lowest LOAD p_vaddr is not below the pre-patch value on a non-PIE x86_64 binary). build-resolution-cache-no-pie.sh bakes a link-time DT_RPATH into the main-no-pie fixture so it can build its layout without patchelf. Closes #622 --- ChangeLog.md | 11 ++ src/patchelf.cc | 199 ++----------------------- src/patchelf.h | 2 - tests/Makefile.am | 10 +- tests/build-resolution-cache-no-pie.sh | 20 ++- tests/no-vaddr-underrun.sh | 66 ++++++++ 6 files changed, 113 insertions(+), 195 deletions(-) create mode 100755 tests/no-vaddr-underrun.sh diff --git a/ChangeLog.md b/ChangeLog.md index 67625b04..f5dcf391 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -1,5 +1,16 @@ # Release History +## Unreleased + +### Bug fixes + +* Fix patching of non-PIE (`ET_EXEC`) binaries placing new `PT_LOAD` + segments below the input's lowest load address, which broke loading on + systems that enforce `vm.mmap_min_addr` at that floor (e.g. + riscv64-linux at `0x10000`). Executables are now rewritten with the + same tail-append strategy as shared objects instead of shifting load + addresses down. See [#622](https://github.com/NixOS/patchelf/issues/622). + ## 0.19.0 (June 26, 2026) This is the first feature release since 0.18.0 (April 2023) and collects three diff --git a/src/patchelf.cc b/src/patchelf.cc index a186f5a8..1597470c 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -1011,188 +1011,6 @@ void ElfFile::rewriteSectionsLibrary() static bool noSort = false; -template -void ElfFile::rewriteSectionsExecutable() -{ - if (!noSort) { - /* Sort the sections by offset, otherwise we won't correctly find - all the sections before the last replaced section. */ - sortShdrs(); - } - - /* What is the index of the last replaced section? */ - unsigned int lastReplaced = 0; - for (unsigned int i = 1; i < rdi(hdr()->e_shnum); ++i) { - std::string sectionName = getSectionName(shdrs.at(i)); - if (replacedSections.count(sectionName)) { - debug("using replaced section '%s'\n", sectionName.c_str()); - lastReplaced = i; - } - } - - assert(lastReplaced != 0); - - debug("last replaced is %d\n", lastReplaced); - - /* Try to replace all sections before that, as far as possible. - Stop when we reach an irreplacable section (such as one of type - SHT_PROGBITS). These cannot be moved in virtual address space - since that would invalidate absolute references to them. */ - assert(lastReplaced + 1 < shdrs.size()); /* !!! I'm lazy. */ - size_t startOffset = rdi(shdrs.at(lastReplaced + 1).sh_offset); - Elf_Addr startAddr = rdi(shdrs.at(lastReplaced + 1).sh_addr); - std::string prevSection; - for (unsigned int i = 1; i <= lastReplaced; ++i) { - Elf_Shdr & shdr(shdrs.at(i)); - std::string sectionName = getSectionName(shdr); - debug("looking at section '%s'\n", sectionName.c_str()); - /* !!! Why do we stop after a .dynstr section? I can't - remember! */ - if ((rdi(shdr.sh_type) == SHT_PROGBITS && sectionName != ".interp") - || prevSection == ".dynstr") - { - startOffset = rdi(shdr.sh_offset); - startAddr = rdi(shdr.sh_addr); - lastReplaced = i - 1; - break; - } - if (!replacedSections.count(sectionName)) { - debug("replacing section '%s' which is in the way\n", sectionName.c_str()); - replaceSection(sectionName, rdi(shdr.sh_size)); - } - prevSection = std::move(sectionName); - } - - debug("first reserved offset/addr is 0x%x/0x%llx\n", - startOffset, (unsigned long long) startAddr); - - assert(startAddr % getPageSize() == startOffset % getPageSize()); - Elf_Addr firstPage = startAddr - startOffset; - debug("first page is 0x%llx\n", (unsigned long long) firstPage); - - if (rdi(hdr()->e_shoff) < startOffset) { - /* The section headers occur too early in the file and would be - overwritten by the replaced sections. Move them to the end of the file - before proceeding. */ - off_t shoffNew = fileContents->size(); - off_t shSize = rdi(hdr()->e_shoff) + rdi(hdr()->e_shnum) * rdi(hdr()->e_shentsize); - fileContents->resize(fileContents->size() + shSize, 0); - wri(hdr()->e_shoff, shoffNew); - - /* Rewrite the section header table. For neatness, keep the - sections sorted. */ - assert(rdi(hdr()->e_shnum) == shdrs.size()); - sortShdrs(); - for (unsigned int i = 1; i < rdi(hdr()->e_shnum); ++i) - * ((Elf_Shdr *) (fileContents->data() + rdi(hdr()->e_shoff)) + i) = shdrs.at(i); - } - - - normalizeNoteSegments(); - - - /* Compute the total space needed for the replaced sections, the - ELF header, and the program headers. */ - size_t neededSpace = sizeof(Elf_Ehdr) + phdrs.size() * sizeof(Elf_Phdr); - for (auto & i : replacedSections) - neededSpace += roundUp(i.second.size(), sectionAlignment); - - debug("needed space is %d\n", neededSpace); - - /* A writable section (commonly .dynamic, which the loader writes DT_DEBUG - into) may be relocated into the reserved area, so the segment covering - that area must end up writable. */ - bool needWritable = std::any_of(replacedSections.begin(), replacedSections.end(), - [this](const std::pair & i) { - return (rdi(findSectionHeader(i.first).sh_flags) & SHF_WRITE) != 0; - }); - - /* If that area sits in an executable LOAD segment, marking it writable - would create a W+X segment. Prefer to grow the file instead, so shiftFile - splits the reserved area into its own R/W segment and leaves the - executable one untouched. Growing adds one program header and one page; - only take this path when both fit (otherwise the underrun check below - would abort), else fall back to marking the segment writable (W+X). */ - bool growForWritable = false; - if (needWritable && neededSpace + sizeof(Elf_Phdr) <= startOffset - && firstPage >= getPageSize()) { - Elf_Off hdrOff = sizeof(Elf_Ehdr) + phdrs.size() * sizeof(Elf_Phdr); - for (const auto & phdr : phdrs) - if (rdi(phdr.p_type) == PT_LOAD && - rdi(phdr.p_offset) <= hdrOff && - rdi(phdr.p_offset) + rdi(phdr.p_filesz) > hdrOff) - { - growForWritable = (rdi(phdr.p_flags) & PF_X) != 0; - break; - } - } - - /* If we need more space at the start of the file, then grow the - file by the minimum number of pages and adjust internal - offsets. */ - if (neededSpace > startOffset || growForWritable) { - /* We also need an additional program header, so adjust for that. */ - neededSpace += sizeof(Elf_Phdr); - debug("needed space is %d\n", neededSpace); - - /* Calculate how many bytes are needed out of the additional pages. */ - size_t extraSpace = neededSpace > startOffset ? neededSpace - startOffset : 0; - // Always give one extra page to avoid colliding with segments that start at - // unaligned addresses and will be rounded down when loaded - unsigned int neededPages = 1 + roundUp(extraSpace, getPageSize()) / getPageSize(); - debug("needed pages is %d\n", neededPages); - if (neededPages * getPageSize() > firstPage) - error("virtual address space underrun!"); - - shiftFile(neededPages, startOffset, extraSpace); - - firstPage -= neededPages * getPageSize(); - startOffset += neededPages * getPageSize(); - } - - Elf_Off curOff = sizeof(Elf_Ehdr) + phdrs.size() * sizeof(Elf_Phdr); - - /* Ensure PHDR is covered by a LOAD segment. - - Because PHDR is supposed to have been covered by such section before, in - here we assume that we don't have to create any new section, but rather - extend the existing one. */ - for (auto& phdr : phdrs) - if (rdi(phdr.p_type) == PT_LOAD && - rdi(phdr.p_offset) <= curOff && - rdi(phdr.p_offset) + rdi(phdr.p_filesz) > curOff) - { - if (rdi(phdr.p_filesz) < neededSpace) { - wri(phdr.p_filesz, neededSpace); - wri(phdr.p_memsz, neededSpace); - } - /* The segment may already be large enough (e.g. with a large page - size), so set this regardless of whether it was extended. If a - grow happened above this is the new R/W segment shiftFile created - and the flag is already set; otherwise we set it here, which for - an executable segment is the W+X fallback when growing was not - possible. */ - if (needWritable) - wri(phdr.p_flags, rdi(phdr.p_flags) | PF_W); - break; - } - - /* Clear out the free space. startOffset is taken from an sh_offset in - the input, so must be bounded before being used as a write extent. */ - if (startOffset < curOff || startOffset > fileContents->size()) - error("section offsets are inconsistent with file size"); - debug("clearing first %d bytes\n", startOffset - curOff); - memset(fileContents->data() + curOff, 0, startOffset - curOff); - - /* Write out the replaced sections. */ - writeReplacedSections(curOff, firstPage, 0); - assert(curOff == neededSpace); - - /* Write out the updated program and section headers */ - rewriteHeaders(firstPage + rdi(hdr()->e_phoff)); -} - - template void ElfFile::normalizeNoteSegments() { @@ -1278,7 +1096,7 @@ void ElfFile::rewriteSections(bool force) rewriteSectionsLibrary(); } else if (rdi(hdr()->e_type) == ET_EXEC) { debug("this is an executable\n"); - rewriteSectionsExecutable(); + rewriteSectionsLibrary(); } else error("unknown ELF type"); } @@ -2320,8 +2138,19 @@ void ElfFile::buildResolutionCache() return; } - auto strTab = getStrTab(shdrDynStr->get()); - auto dynSpan = getSectionSpan(shdrDynamic->get()); + /* A cache note that is already present means we will refuse to rebuild (or, + at most, confirm it is up to date). Don't let a malformed .dynamic or + .dynstr abort with a raw section error first: a note that is already + present must fail loudly as "already present", which callers rely on. */ + span strTab; + span dynSpan; + try { + strTab = getStrTab(shdrDynStr->get()); + dynSpan = getSectionSpan(shdrDynamic->get()); + } catch (...) { + failIfStale(); + throw; + } std::vector needed; const char * dtRunPath = nullptr; diff --git a/src/patchelf.h b/src/patchelf.h index 0700b9d3..c24dab23 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -131,8 +131,6 @@ class ElfFile void rewriteSectionsLibrary(); - void rewriteSectionsExecutable(); - void normalizeNoteSegments(); void removeResolutionCache(); diff --git a/tests/Makefile.am b/tests/Makefile.am index 8e11d451..918a13af 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -64,7 +64,8 @@ src_TESTS = \ empty-note.sh \ set-interpreter-same.sh \ property-rewrite.sh \ - large-page-dynamic.sh + large-page-dynamic.sh \ + no-vaddr-underrun.sh build_TESTS = \ $(no_rpath_arch_TESTS) @@ -108,11 +109,16 @@ main_DEPENDENCIES = libfoo.so main_LDFLAGS = $(LDFLAGS_local) # Non-PIE (ET_EXEC) variant of main, to exercise rewriteSectionsExecutable(). +# Carries a concrete DT_RPATH to the build directory so +# build-resolution-cache-no-pie.sh can resolve the run path without first +# rewriting sections with patchelf: after the vaddr-underrun fix, patching a +# non-PIE binary moves the section header table off the end of the file, which +# would make pad-to-page's "SHT ends on a page boundary" layout impossible. main_no_pie_SOURCES = main.c main_no_pie_CFLAGS = -fno-pie main_no_pie_LDADD = -lfoo $(AM_LDADD) main_no_pie_DEPENDENCIES = libfoo.so -main_no_pie_LDFLAGS = $(LDFLAGS_local) -no-pie +main_no_pie_LDFLAGS = $(LDFLAGS_local) -no-pie -Wl,-rpath,`pwd` # Non-PIE binary with a max-page-size larger than the runtime page size. On # 32-bit targets this makes patchelf relocate .dynamic into the read-only first diff --git a/tests/build-resolution-cache-no-pie.sh b/tests/build-resolution-cache-no-pie.sh index 6e13650b..74799b71 100755 --- a/tests/build-resolution-cache-no-pie.sh +++ b/tests/build-resolution-cache-no-pie.sh @@ -1,23 +1,31 @@ #! /bin/sh -e # ET_EXEC path: with the section header table ending on a page boundary, the # in-place SHT rewrite would land the note's own header on top of its Elf_Nhdr. +# +# main-no-pie carries a concrete DT_RPATH baked in at link time (see +# tests/Makefile.am), so --build-resolution-cache can resolve the run path +# straight from the linker output. That matters because, after the +# vaddr-underrun fix, using patchelf to set the run path on a non-PIE binary +# appends the rewritten sections at the end of the file and moves the SHT off +# the end, which would make the pad-to-page layout below impossible to build. SCRATCH=scratch/$(basename "$0" .sh) READELF=${READELF:-readelf} PATCHELF=$(readlink -f "../src/patchelf") +LIBDIR=$(pwd) rm -rf "${SCRATCH}" -mkdir -p "${SCRATCH}"/libs +mkdir -p "${SCRATCH}" cp main-no-pie "${SCRATCH}/main" -cp libfoo.so "${SCRATCH}/libs/" -cp libbar.so "${SCRATCH}/libs/" if ! ${READELF} -h "${SCRATCH}/main" | grep -q "Type:.*EXEC"; then echo "SKIP: toolchain did not produce an ET_EXEC binary" exit 77 fi -${PATCHELF} --set-rpath "$(pwd)/${SCRATCH}/libs" "${SCRATCH}/main" +# Push the section header table end onto a page boundary, the layout that used +# to corrupt the note. This needs the SHT at the end of the file, which the +# linker output provides because no patchelf section rewrite has run yet. ./pad-to-page "${SCRATCH}/main" ${PATCHELF} --build-resolution-cache "${SCRATCH}/main" @@ -36,13 +44,13 @@ fi strings=$(${READELF} -p .note.nixos.ldcache "${SCRATCH}/main") echo "$strings" -if ! echo "$strings" | grep -q "${SCRATCH}/libs/libfoo.so"; then +if ! echo "$strings" | grep -q "${LIBDIR}/libfoo.so"; then echo "FAIL: cache does not resolve libfoo.so" exit 1 fi exitCode=0 -LD_LIBRARY_PATH="$(pwd)/${SCRATCH}/libs" "${SCRATCH}/main" || exitCode=$? +LD_LIBRARY_PATH="${LIBDIR}" "${SCRATCH}/main" || exitCode=$? if [ "$exitCode" != 46 ]; then echo "FAIL: bad exit code $exitCode (expected 46)" exit 1 diff --git a/tests/no-vaddr-underrun.sh b/tests/no-vaddr-underrun.sh new file mode 100755 index 00000000..ce6c4430 --- /dev/null +++ b/tests/no-vaddr-underrun.sh @@ -0,0 +1,66 @@ +#! /bin/sh -e +# Regression test for https://github.com/NixOS/patchelf/issues/622. +# +# Patching a non-PIE binary in a way that grows the program header table +# used to shift every LOAD segment's p_vaddr down by one or more pages, +# producing LOAD segments below the input's lowest p_vaddr. On systems +# where vm.mmap_min_addr equals that lowest p_vaddr (riscv64-linux on +# Ubuntu, x86_64 with hardened sysctls, NixOS initrd with raised +# mmap_min_addr), the kernel refuses the fixed-address mmap and the +# patched binary fails to load. +# +# This test asserts that after patching a non-PIE binary, the lowest +# LOAD p_vaddr is not below the original lowest LOAD p_vaddr. + +SCRATCH=scratch/$(basename "$0" .sh) + +rm -rf "${SCRATCH}" +mkdir -p "${SCRATCH}" +cp simple "${SCRATCH}/simple" + +# Confirm the fixture is actually non-PIE; otherwise the test is silently +# meaningless. +if ! readelf -h "${SCRATCH}/simple" | grep -qE 'Type:\s+EXEC'; then + echo "skipping test: simple is not a non-PIE EXEC on this toolchain" >&2 + exit 77 +fi + +# Smallest LOAD p_vaddr from `readelf -lW`. The address is the third +# whitespace-separated column on LOAD lines. +lowest_load_vaddr() { + readelf -lW "$1" \ + | awk '/^ LOAD/ { print strtonum($3) }' \ + | sort -n \ + | head -1 +} + +pre=$(lowest_load_vaddr "${SCRATCH}/simple") +echo "pre-patch lowest LOAD vaddr: $(printf '0x%x' "$pre")" + +# Force the program header table to grow by adding a long rpath. Combined +# with what's already in the binary this is enough to overflow the +# original PHT space and force a layout change. +long_rpath=$(printf '/very/long/rpath/segment%.0s' $(seq 1 16)) +../src/patchelf --force-rpath --set-rpath "$long_rpath" "${SCRATCH}/simple" + +post=$(lowest_load_vaddr "${SCRATCH}/simple") +echo "post-patch lowest LOAD vaddr: $(printf '0x%x' "$post")" + +if [ "$post" -lt "$pre" ]; then + echo "FAIL: post-patch lowest LOAD vaddr 0x$(printf '%x' "$post") is below" \ + "pre-patch lowest LOAD vaddr 0x$(printf '%x' "$pre")" + exit 1 +fi + +# The patched binary should still load and run correctly. This catches +# layout problems that pass the static check above (e.g. PT_PHDR.p_vaddr +# pointing outside any LOAD segment). +exitCode=0 +(cd "${SCRATCH}" && ./simple) || exitCode=$? + +if [ "$exitCode" != 0 ]; then + echo "FAIL: patched binary exited with code $exitCode" + exit 1 +fi + +echo "PASS"