Corruption with program headers #243
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there a way to compile a simple program to reproduce this so it can be added to regression tests? |
|
I couldn't find a special linker flag that causes this to break. I found out that many standard shared objects (like pthread, libm, libdl...) from the glibc package on both rhel6 and rhel5. |
|
@edolstra do you think it's fine in that case to vendor the library? |
|
Hi! I also encountered this issue with the following binaries: repro.tar.gz $ ./repro
2021/05/18 16:07:23 no config file specified
$ ./src/patchelf --set-interpreter ./ld-linux-x86-64.so.2 repro
patching ELF file 'repro'
replacing section '.interp' with size 23
this is a dynamic library
last page is 0x1fb1000
needed space is 24
shifting new PT_LOAD segment by 2768896 bytes to work around a Linux kernel bug
rewriting section '.interp' from offset 0xf8a (size 118) to offset 0x2255000 (size 23)
rewriting symbol table section 13
rewriting symbol table section 34
writing repro
$ ./repro
Segmentation fault
$ readelf -a repro > /dev/null
readelf: Warning: [ 0]: Link field (24) should index a string section.
readelf: Error: the PHDR segment is not covered by a LOAD segmentNote: #270 patch needs to be applied, otherwise the issue is not reproducible since commit dd4d2af was merged. If commit 4a61b24 is rebased on top of current master, the issue is no longer present: diff --git a/src/patchelf.cc b/src/patchelf.cc
index 5c8b5c0..0e2ecfd 100644
--- a/src/patchelf.cc
+++ b/src/patchelf.cc
@@ -772,12 +772,15 @@ void ElfFile<ElfFileParamNames>::rewriteSectionsLibrary()
PT_LOAD segment located directly after the last virtual address
page of other segments. */
Elf_Addr startPage = 0;
+ Elf_Addr firstPage = 0;
for (auto & phdr : phdrs) {
Elf_Addr thisPage = roundUp(rdi(phdr.p_vaddr) + rdi(phdr.p_memsz), getPageSize());
if (thisPage > startPage) startPage = thisPage;
+ if (rdi(phdr.p_type) == PT_PHDR) firstPage = rdi(phdr.p_vaddr) - rdi(phdr.p_offset);
}
debug("last page is 0x%llx\n", (unsigned long long) startPage);
+ debug("first page is 0x%llx\n", (unsigned long long) firstPage);
/* When normalizing note segments we will in the worst case be adding
1 program header for each SHT_NOTE section. */
@@ -848,7 +851,7 @@ void ElfFile<ElfFileParamNames>::rewriteSectionsLibrary()
assert(curOff == startOffset + neededSpace);
/* Write out the updated program and section headers */
- rewriteHeaders(rdi(hdr->e_phoff));
+ rewriteHeaders(firstPage + hdr->e_phoff);
}Output after rebuilding patchelf: $ ./repro
2021/05/18 16:13:42 no config file specified
$ ./src/patchelf --set-interpreter ./ld-linux-x86-64.so.2 repro
patching ELF file 'repro'
replacing section '.interp' with size 23
this is a dynamic library
last page is 0x1fb1000
first page is 0x400000
needed space is 24
shifting new PT_LOAD segment by 2768896 bytes to work around a Linux kernel bug
rewriting section '.interp' from offset 0xf8a (size 118) to offset 0x2255000 (size 23)
rewriting symbol table section 13
rewriting symbol table section 34
writing repro
$ ./repro
2021/05/18 16:13:52 no config file specified
$ readelf -a repro > /dev/null
readelf: Warning: [ 0]: Link field (24) should index a string section.Could you please let me know how I could help in order to proceed with the integration? Thanks! |
|
If someone opens a PR with rebased version and with repro file added as a test, it has a higher chance of being merged. |
|
@ovpanait Is there source for that (So I guess this reproducer works?) |
The repro file is generated by the go build system in a cross-compilation environment and so far I was not able to create a manual reproducer in C.
Maybe the testcase should just check readelf output for "the PHDR segment is not covered by a LOAD segment" error rather than running the prebuilt binary? (since it's an x86_64 binary that will not run on other arches) |
|
Fixed in #278 |
ovpanait
added a commit
to ovpanait/patchelf
that referenced
this pull request
Sep 26, 2021
A PT_PHDR corruption was previously reported and fixed in [1]: the issue was that the VirtAddr field of the PT_PHDR program header would get overwritten with the file offset of the program header table rather than the virtual address. A testcase for this was also added in [2]. However, the tescase is not included in the Makefile.am regression testsuite and also tries to run a x86_64 prebuilt binary unconditionally, which would not work on other architectures. To fix this, create a standalone testcase for the PT_PHDR VirtAddr field corruption and include it in Makefile.am. In order to reproduce [1], a binary with the following characteristics is needed: - the ELF file type must be ET_DYN - the ELF file must contain a PT_PHDR program header - the file offset and the VirtAddr field of the PT_PHDR program header must be different [1] NixOS#243 [2] NixOS@8f94e11 Signed-off-by: Ovidiu Panait <ovpanait@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This corruption doesn't affect runtime linking but can be observed with inspection tools like ldd and readelf (probably because dynamic linker doesn't really go over all the program headers of shared object - only a theory)
Please use the following library for reproduction: libm.so.6.zip
The root cause of this issue is similar to the one of #224 or #239
Thank you!