From f4f1848e42b68adca04019bcf0d47132d1530bf2 Mon Sep 17 00:00:00 2001 From: Jason Date: Fri, 2 Dec 2022 09:58:49 -0500 Subject: [PATCH 1/3] shiftFile: when splitting a segment into two pieces, preserve the original flags in both --- src/patchelf.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 398404c8..4b4fea43 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -463,6 +463,9 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start int splitIndex = -1; size_t splitShift = 0; + /* Save off the flags from the segment that we are splitting so we can apply the same value + to both of the resulting segments. */ + decltype(phdrs.at(0).p_flags) splitFlags = 0; /* Update the offsets in the program headers. */ for (int i = 0; i < rdi(hdr()->e_phnum); ++i) { @@ -473,6 +476,7 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start splitIndex = i; splitShift = startOffset - p_start; + splitFlags = rdi(phdrs.at(i).p_flags); /* This is the load segment we're currently extending within, so we split it. */ wri(phdrs.at(i).p_offset, startOffset); @@ -513,7 +517,7 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start wri(phdr.p_paddr, phdrs.at(splitIndex).p_paddr - splitShift - shift); wri(phdr.p_vaddr, phdrs.at(splitIndex).p_vaddr - splitShift - shift); wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + shift)); - wri(phdr.p_flags, PF_R | PF_W); + wri(phdr.p_flags, splitFlags); wri(phdr.p_align, getPageSize()); } From 8d2cb4f9ab8d564904c292099a022ffb3cccd52d Mon Sep 17 00:00:00 2001 From: Jason Date: Fri, 2 Dec 2022 10:01:41 -0500 Subject: [PATCH 2/3] Fix bug in file shifting that could cause conflicting PT_LOAD segments When a section in the file needs to be enlarged (e.g. to accommodate setting a larger RPATH), shiftFile() is used to shift all content following the growing section to a later position in the file. Commit 109b771f53ee3d37ede8c0f165665605183c0975 introduced logic to ensure that, after the segment split, no sections span multiple segments. This is done by sliding the portion of the segment after the split point later in the file, then adding a new PT_LOAD segment that contains the preceding data plus the extra room that is being added. The existing implementation does this by simply adding `extraPages*getPageSize()` bytes to the number of bytes ahead of the split point in the segment. However, this approach can result in two PT_LOAD segments that overlap when page boundaries are taken into account. As an example, this PT_LOAD section (taken from a Python 3.10 binary): LOAD 0x0000000000000000 0x0000000000400000 0x0000000000400000 0x0000000000000948 0x0000000000000948 R E 0x200000 is split into the following two sections: LOAD 0x0000000000000000 0x00000000003ff000 0x00000000003ff000 0x0000000000001594 0x0000000000001594 R E 0x1000 LOAD 0x0000000000001594 0x0000000000400594 0x0000000000400594 0x00000000000003b4 0x00000000000003b4 R E 0x1000 Note that the two PT_LOAD sections both contain the memory page at address 0x400000. The Linux kernel's ELF loader (at least as of v4.18) does not accept this as a valid ELF executable, triggering a segfault with si_code=SI_KERNEL immediately when the binary is executed. The fix here is to set the length of the segment that comes before the split point more carefully; instead of adding `extraPages*getPageSize()` bytes to the portion of the segment that came before the split, the actual number of padding bytes that were needed (before rounding up to the next multiple of the page size) are used. This avoids the overlap in the PT_LOAD segments and makes the output files executable again. --- src/patchelf.cc | 10 ++++++---- src/patchelf.h | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 4b4fea43..03bb3d3d 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -436,7 +436,7 @@ static uint64_t roundUp(uint64_t n, uint64_t m) template -void ElfFile::shiftFile(unsigned int extraPages, size_t startOffset) +void ElfFile::shiftFile(unsigned int extraPages, size_t startOffset, size_t extraBytes) { assert(startOffset >= sizeof(Elf_Ehdr)); @@ -516,7 +516,7 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start wri(phdr.p_offset, phdrs.at(splitIndex).p_offset - splitShift - shift); wri(phdr.p_paddr, phdrs.at(splitIndex).p_paddr - splitShift - shift); wri(phdr.p_vaddr, phdrs.at(splitIndex).p_vaddr - splitShift - shift); - wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + shift)); + wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + extraBytes)); wri(phdr.p_flags, splitFlags); wri(phdr.p_align, getPageSize()); } @@ -916,12 +916,14 @@ void ElfFile::rewriteSectionsExecutable() neededSpace += sizeof(Elf_Phdr); debug("needed space is %d\n", neededSpace); - unsigned int neededPages = roundUp(neededSpace - startOffset, getPageSize()) / getPageSize(); + /* Calculate how many bytes are needed out of the additional pages. */ + size_t extraSpace = neededSpace - startOffset; + unsigned int neededPages = roundUp(extraSpace, getPageSize()) / getPageSize(); debug("needed pages is %d\n", neededPages); if (neededPages * getPageSize() > firstPage) error("virtual address space underrun!"); - shiftFile(neededPages, startOffset); + shiftFile(neededPages, startOffset, extraSpace); firstPage -= neededPages * getPageSize(); startOffset += neededPages * getPageSize(); diff --git a/src/patchelf.h b/src/patchelf.h index c336c511..f4eec6f2 100644 --- a/src/patchelf.h +++ b/src/patchelf.h @@ -77,7 +77,7 @@ class ElfFile void sortShdrs(); - void shiftFile(unsigned int extraPages, size_t sizeOffset); + void shiftFile(unsigned int extraPages, size_t sizeOffset, size_t extraBytes); std::string getSectionName(const Elf_Shdr & shdr) const; From 3a6d77112780e6cf072ecd5abbbab61db8ab0266 Mon Sep 17 00:00:00 2001 From: Jason Date: Wed, 28 Dec 2022 23:47:36 -0500 Subject: [PATCH 3/3] Revert "shiftFile: when splitting a segment into two pieces, preserve the original flags in both" This reverts commit f4f1848e42b68adca04019bcf0d47132d1530bf2. --- src/patchelf.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/patchelf.cc b/src/patchelf.cc index 03bb3d3d..da4e30b0 100644 --- a/src/patchelf.cc +++ b/src/patchelf.cc @@ -463,9 +463,6 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start int splitIndex = -1; size_t splitShift = 0; - /* Save off the flags from the segment that we are splitting so we can apply the same value - to both of the resulting segments. */ - decltype(phdrs.at(0).p_flags) splitFlags = 0; /* Update the offsets in the program headers. */ for (int i = 0; i < rdi(hdr()->e_phnum); ++i) { @@ -476,7 +473,6 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start splitIndex = i; splitShift = startOffset - p_start; - splitFlags = rdi(phdrs.at(i).p_flags); /* This is the load segment we're currently extending within, so we split it. */ wri(phdrs.at(i).p_offset, startOffset); @@ -517,7 +513,7 @@ void ElfFile::shiftFile(unsigned int extraPages, size_t start wri(phdr.p_paddr, phdrs.at(splitIndex).p_paddr - splitShift - shift); wri(phdr.p_vaddr, phdrs.at(splitIndex).p_vaddr - splitShift - shift); wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + extraBytes)); - wri(phdr.p_flags, splitFlags); + wri(phdr.p_flags, PF_R | PF_W); wri(phdr.p_align, getPageSize()); }