Skip to content
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

Allocate PHT & SHT at the end of the *.elf file #544

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Patryk27
Copy link
Member

@Patryk27 Patryk27 commented Feb 6, 2024

Closes #531.
Closes #482.
Closes #244.

Upstream-wise, affects NixOS/nixpkgs#226339.
(didn't want to write close so that merging this merge request doesn't close that issue at once)

Abstract

Patching an *.elf file might require extending its program header table, which can then cause it to run out of its originally allocated space (both in terms of file offset and virtual memory).

Currently patchelf solves this problem by finding which sections would overlap PHT and then by moving them to the end of *.elf file:

while( i < rdi(hdr()->e_shnum) && rdi(shdrs.at(i).sh_offset) <= pht_size ) {

As compared to similar logic for binaries:

if ((rdi(shdr.sh_type) == SHT_PROGBITS && sectionName != ".interp")

... the logic for libraries is missing a crucial check: it doesn't take into account whether that particular section can be shuffled around - in particular, sections with SHT_PROGBITS can't!

As luck would have it, libnode.so (e.g. shipped together with pcloud) does have .rodata (a section with SHT_PROGBITS active) right at the beginning of the file:

; readelf -S libnode.so

There are 29 section headers, starting at offset 0x152bf70:

Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .rodata           PROGBITS         0000000000000240  00000240
       00000000003796a8  0000000000000000 AMS       0     0     32
/* ... */

... which patchelf happily moves around, breaking RIP-relative addressing in the assembly (which, after patching, tries to access the ZZZZ-ed memory).

This commit fixes the issue by changing the logic from:

if there's no space for PHT, shuffle sections around

... to, perhaps a bit more wasteful in terms of storage:

just always allocate PHT at the end of the file

As far as I've checked, the reason why PHT was so strictly kept at the beginning was an old Linux bug:

/* Even though this file is of type ET_DYN, it could actually be

Bug in Linux kernel, in fs/binfmt_elf.c:

... which is not present anymore (not sure when precisely was it fixed, though - the original entry in the BUGS file is dated 2005).

Seizing the day, I'm also including another fix (for binaries), so that merging this pull request will solve all pcloud-related problems.

@@ -37,9 +37,9 @@ if [ "$(echo "$readelfData" | grep -c "PHDR")" != 1 ]; then
fi

virtAddr=$(echo "$readelfData" | grep "PHDR" | awk '{print $3}')
if [ "$virtAddr" != "0x00000034" ]; then
if [ "$virtAddr" != "0x01040000" ]; then
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously PHDR was always kept right after EHDR, while now it's allocated at the end of the file.

@@ -34,7 +34,7 @@ load_segments_after=$(${READELF} -W -l libbar.so | grep -c LOAD)
# To be even more strict, check that we don't add too many extra LOAD entries
###############################################################################
echo "Segments before: ${load_segments_before} and after: ${load_segments_after}"
if [ "${load_segments_after}" -gt $((load_segments_before + 2)) ]
if [ "${load_segments_after}" -gt $((load_segments_before + 3)) ]
Copy link
Member Author

@Patryk27 Patryk27 Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because now we might sometimes allocate a brand-new LOAD segment just for the PHDR's sake.

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)
Copy link
Member Author

@Patryk27 Patryk27 Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic used to be run only if neededSpace <= startOffset, under the assumption that otherwise (when neededSpace > startOffset), we call shiftFile(), which will automatically create appropriate PT_LOAD segment:

/* Add a segment that maps the new program/section headers and

But that was done under the assumption that shiftFile()'s startOffset will land inside a loadable segment that happens to contain PHDR, which is not necessarily true! Some binaries have a dedicated small LOAD segment just for PHDR, making shiftFile() then pick the next available LOAD segment.

I believe this is the same case this merge request tried to cover (or at least both make pcloud a valid binary):

#264

I've also checked it on the reproduction from #264 (comment) - I don't have aarch64-linux at hand, though, so I've just patched the file and ran eu-elflint (which now doesn't complain about the string sections there).

Also, this is now tested as a part of short-first-segment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, in a hindsight, now I'm thinking whether this approach can't cause some segments to overlap each other? (maybe that's why tests on a few other architectures fail, as I now see)

@Patryk27 Patryk27 changed the title Draft: Allocate PHT & SHT at the end of the *.elf file Allocate PHT & SHT at the end of the *.elf file Feb 8, 2024
@Patryk27
Copy link
Member Author

Patryk27 commented Feb 8, 2024

Btw, I've got another merge request in the works, one that fixes #75 and hopefully will be able to handle #520 as well, but I've ultimately decided to move it out of this pull request, since it's technically a separate changeset (and less important, at least from my point of view) -- I'll prepare it in the upcoming weeks.

@jvolkman
Copy link

jvolkman commented Mar 7, 2024

Another issue is that strip doesn't like binaries with with the PHT and SHT bits at the end, and will either fail or generate invalid binaries as output. One workaround is to just strip first, but I guess this is not possible in all workflows.

See #117 and #10.

Definitely seems like a strip/libbfd bug since, as far as can tell, nothing in the ELF spec requires these tables to be at the top. I spent a couple days stepping through bfd/elf.c back when I was working on this (which also places the updated headers at the end), but I was ultimately unable to decipher what was going on.

tests/short-first-segment.sh Outdated Show resolved Hide resolved
@Mic92 Mic92 force-pushed the fixes branch 5 times, most recently from 5f982f1 to 8dad294 Compare November 18, 2024 10:12
@Mic92
Copy link
Member

Mic92 commented Nov 18, 2024

@Patryk27 rebased CI and it looks like this causes several regressions on non-x86 platforms.

@Mic92
Copy link
Member

Mic92 commented Nov 18, 2024

It should be possible to reproduce this locally using boot.binfmt.emulatedSystems and than either with a cross compiler from nixpkgs or with the docker image used in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants