rewriteSections: rewrite executables like shared objects (#622)

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

src/patchelf.cc

@@ -1011,188 +1011,6 @@ void ElfFile<ElfFileParamNames>::rewriteSectionsLibrary()
 
 static bool noSort = false;
 
-template<ElfFileParams>
-void ElfFile<ElfFileParamNames>::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<const std::string, std::string> & 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<ElfFileParams>
 void ElfFile<ElfFileParamNames>::normalizeNoteSegments()
 {
@@ -1278,7 +1096,7 @@ void ElfFile<ElfFileParamNames>::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<ElfFileParamNames>::buildResolutionCache()
         return;
     }
 
-    auto strTab = getStrTab(shdrDynStr->get());
-    auto dynSpan = getSectionSpan<Elf_Dyn>(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<char> strTab;
+    span<Elf_Dyn> dynSpan;
+    try {
+        strTab = getStrTab(shdrDynStr->get());
+        dynSpan = getSectionSpan<Elf_Dyn>(shdrDynamic->get());
+    } catch (...) {
+        failIfStale();
+        throw;
+    }
 
     std::vector<std::string> needed;
     const char * dtRunPath = nullptr;

src/patchelf.h

@@ -131,8 +131,6 @@ class ElfFile
 
     void rewriteSectionsLibrary();
 
-    void rewriteSectionsExecutable();
-
     void normalizeNoteSegments();
 
     void removeResolutionCache();

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

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

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"