-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split segment size fix #447
Conversation
…ginal flags in both
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 109b771 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.
Sorry, missed this pr. |
This PR seems to break a number of tests. Can you have a look by running |
I haven't been able to reproduce them on my system; all of the tests pass successfully. I guess that is to be expected, though, as I'm on Ubuntu 18.04 and all of the Ubuntu tests in CI seem to have passed as well; only the nix tests show failures. |
Here is the full log:
Since nix is also just using normal binutils, glibc und gcc, I think those crashes are valid and might also affect ubuntu eventually or already under certain circumstances. If you have the bandwidth and motivation, I can give you access to a nixos machine: https://github.com/nix-community/infra#community-builder |
src/patchelf.cc
Outdated
wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + shift)); | ||
wri(phdr.p_flags, PF_R | PF_W); | ||
wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + extraBytes)); | ||
wri(phdr.p_flags, splitFlags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flags change made sense to me initially too but can be incorrect, since we can move a .dynamic
section into here which needs write permissions, hence the hard RW - anything less than RW can actually often break and there should be no executable code here to need the X bit. I suppose ideally we'd harden this by splitting this more fine grained so we only give write permissions to sections that need it, but that's a more complex fix since we'd need to separate this into 3 pages. Worthwhile to probably do at some point though.
This change might be the cause of some CI failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that makes sense. I am admittedly not an expert in the intricacies of the ELF format, so I wasn't aware of this. When I'm able to look at this again (maybe not until next week), I will back out the change to the flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change to how the segment flags are handled. I'm not sure how to re-run the CI build though.
@@ -512,8 +516,8 @@ void ElfFile<ElfFileParamNames>::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_flags, PF_R | PF_W); | |||
wri(phdr.p_filesz, wri(phdr.p_memsz, splitShift + extraBytes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check: can you post what the sections look like for you after this change?
In particular, my head says the address of the moved section needs adjusting too, but I might not be reading this correctly so comparing the output before & after would be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I should have made that more clear in the beginning. Here is the output of readelf -l <file>
for the three cases (again, from a Python 3.10 executable that I built from source)
Unmodified executable used as input to patchelf
Elf file type is EXEC (Executable file)
Entry point 0x4006a0
There are 9 program headers, starting at offset 64
Program Headers:
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
PHDR 0x0000000000000040 0x0000000000400040 0x0000000000400040
0x00000000000001f8 0x00000000000001f8 R 0x8
INTERP 0x0000000000000238 0x0000000000400238 0x0000000000400238
0x000000000000001c 0x000000000000001c R 0x1
[Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
LOAD 0x0000000000000000 0x0000000000400000 0x0000000000400000
0x0000000000000948 0x0000000000000948 R E 0x200000
LOAD 0x0000000000000d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000294 0x0000000000000298 RW 0x200000
DYNAMIC 0x0000000000000da0 0x0000000000600da0 0x0000000000600da0
0x0000000000000240 0x0000000000000240 RW 0x8
NOTE 0x0000000000000254 0x0000000000400254 0x0000000000400254
0x0000000000000044 0x0000000000000044 R 0x4
GNU_EH_FRAME 0x0000000000000828 0x0000000000400828 0x0000000000400828
0x000000000000003c 0x000000000000003c R 0x4
GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 RW 0x10
GNU_RELRO 0x0000000000000d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000270 0x0000000000000270 R 0x1
Section to Segment mapping:
Segment Sections...
00
01 .interp
02 .interp .note.ABI-tag .note.gnu.build-id .gnu.hash .dynsym .dynstr .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
03 .init_array .fini_array .dynamic .got .got.plt .data .bss
04 .dynamic
05 .note.ABI-tag .note.gnu.build-id
06 .eh_frame_hdr
07
08 .init_array .fini_array .dynamic .got
Output of patchelf
master branch
This is the version that segfaults upon execution.
Elf file type is EXEC (Executable file)
Entry point 0x4006a0
There are 11 program headers, starting at offset 64
Program Headers:
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
PHDR 0x0000000000000040 0x00000000003ff040 0x00000000003ff040
0x0000000000000268 0x0000000000000268 R 0x8
GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 RW 0x10
LOAD 0x0000000000000000 0x00000000003ff000 0x00000000003ff000
0x0000000000001594 0x0000000000001594 RW 0x1000
INTERP 0x00000000000005e0 0x00000000003ff5e0 0x00000000003ff5e0
0x000000000000001c 0x000000000000001c R 0x1
[Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
NOTE 0x0000000000000600 0x00000000003ff600 0x00000000003ff600
0x0000000000000020 0x0000000000000020 R 0x4
NOTE 0x0000000000000620 0x00000000003ff620 0x00000000003ff620
0x0000000000000024 0x0000000000000024 R 0x4
LOAD 0x0000000000001594 0x0000000000400594 0x0000000000400594
0x00000000000003b4 0x00000000000003b4 R E 0x1000
GNU_EH_FRAME 0x0000000000001828 0x0000000000400828 0x0000000000400828
0x000000000000003c 0x000000000000003c R 0x4
LOAD 0x0000000000001d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000294 0x0000000000000298 RW 0x1000
GNU_RELRO 0x0000000000001d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000270 0x0000000000000270 R 0x1
DYNAMIC 0x0000000000001da0 0x0000000000600da0 0x0000000000600da0
0x0000000000000240 0x0000000000000240 RW 0x8
Section to Segment mapping:
Segment Sections...
00
01
02 .dynstr .dynsym .gnu.hash .interp .note.ABI-tag .note.gnu.build-id
03 .interp
04 .note.ABI-tag
05 .note.gnu.build-id
06 .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
07 .eh_frame_hdr
08 .init_array .fini_array .dynamic .got .got.plt .data .bss
09 .init_array .fini_array .dynamic .got
10 .dynamic
Output of patchelf
from this branch
Elf file type is EXEC (Executable file)
Entry point 0x4006a0
There are 11 program headers, starting at offset 64
Program Headers:
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
PHDR 0x0000000000000040 0x00000000003ff040 0x00000000003ff040
0x0000000000000268 0x0000000000000268 R 0x8
GNU_STACK 0x0000000000001000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 RW 0x10
LOAD 0x0000000000000000 0x00000000003ff000 0x00000000003ff000
0x0000000000001000 0x0000000000001000 RW 0x1000
INTERP 0x00000000000005e0 0x00000000003ff5e0 0x00000000003ff5e0
0x000000000000001c 0x000000000000001c R 0x1
[Requesting program interpreter: /lib64/ld-linux-x86-64.so.2]
NOTE 0x0000000000000600 0x00000000003ff600 0x00000000003ff600
0x0000000000000020 0x0000000000000020 R 0x4
NOTE 0x0000000000000620 0x00000000003ff620 0x00000000003ff620
0x0000000000000024 0x0000000000000024 R 0x4
LOAD 0x0000000000001000 0x0000000000400000 0x0000000000400000
0x0000000000000948 0x0000000000000948 R E 0x1000
GNU_EH_FRAME 0x0000000000001828 0x0000000000400828 0x0000000000400828
0x000000000000003c 0x000000000000003c R 0x4
LOAD 0x0000000000001d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000294 0x0000000000000298 RW 0x1000
GNU_RELRO 0x0000000000001d90 0x0000000000600d90 0x0000000000600d90
0x0000000000000270 0x0000000000000270 R 0x1
DYNAMIC 0x0000000000001da0 0x0000000000600da0 0x0000000000600da0
0x0000000000000240 0x0000000000000240 RW 0x8
Section to Segment mapping:
Segment Sections...
00
01
02 .dynstr .dynsym .gnu.hash .interp .note.ABI-tag .note.gnu.build-id
03 .interp
04 .note.ABI-tag
05 .note.gnu.build-id
06 .gnu.version .gnu.version_r .rela.dyn .rela.plt .init .plt .text .fini .rodata .eh_frame_hdr .eh_frame
07 .eh_frame_hdr
08 .init_array .fini_array .dynamic .got .got.plt .data .bss
09 .init_array .fini_array .dynamic .got
10 .dynamic
It's a weird one because I've never been able to reproduce the issue you mentioned - it seems like a certain kernel option is needed or something to make it actually reject the ELF, since it doesn't happen on my default Ubuntu install using an identical binary (sha256-matching) to what doesn't work for some others. I even deployed the latest patchelf to a few thousand users and the only feedback was from RHEL users. WSL2 too initially, but Microsoft already had a beta with the issue fixed which has now been rolled out for everyone. Very odd, but at least you are able to reproduce it. I agree the general fix would be to create some empty space between the sections just to make sure they're on different pages since permissions etc work under that assumption - it seems like some of the WSL2 users who weren't affected are those who have it where the two sections are the same permission. |
I've seen the same; I'm not sure what combination of kernel version/configuration triggers it, but I too have only observed it on RHEL (and its free derivatives). |
… the original flags in both" This reverts commit f4f1848.
Yocto project ran into this issue with patchelf 0.16.1 and 0.17.0 with many different binaries segfaulting. Applying the patch here seems to help. |
bors merge |
Sorry for the delay. I will make a patch release with this. |
I have created a new release for this. |
Thanks for creating the release. Maybe the |
Oh, shoot. I released 0.17.2 to fix this. |
Thanks! |
Fix for #446
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 109b771 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, thisPT_LOAD
section (taken from a Python 3.10 binary):is split into the following two sections:
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 withsi_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 thePT_LOAD
segments and makes the output files executable again.Fix for incorrect segment flags
Another thing I fixed while in here is the unconditional setting of the flags in the newly-generated segment to
RW
. Since this segment is formed by splitting one of the segments from the input file into two, it seems most accurate to preserve that section's flags for both of the two post-split segments.