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.

Sign up for GitHub

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

Jump to bottom

Split segment size fix #447

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Split segment size fix #447

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4f1848
shiftFile: when splitting a segment into two pieces, preserve the ori…
Dec 2, 2022
8d2cb4f
Fix bug in file shifting that could cause conflicting PT_LOAD segments
Dec 2, 2022
3a6d771
Revert "shiftFile: when splitting a segment into two pieces, preserve…
Dec 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/patchelf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ static uint64_t roundUp(uint64_t n, uint64_t m)


template<ElfFileParams>
void ElfFile<ElfFileParamNames>::shiftFile(unsigned int extraPages, size_t startOffset)
void ElfFile<ElfFileParamNames>::shiftFile(unsigned int extraPages, size_t startOffset, size_t extraBytes)
{
assert(startOffset >= sizeof(Elf_Ehdr));

Expand All @@ -463,6 +463,9 @@ void ElfFile<ElfFileParamNames>::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) {
Expand All @@ -473,6 +476,7 @@ void ElfFile<ElfFileParamNames>::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);
Expand Down Expand Up @@ -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));
Copy link
Contributor

@Bo98 Bo98 Dec 29, 2022 •
edited
Loading

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.

Copy link
Author

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

wri(phdr.p_flags, splitFlags);
Copy link
Contributor

@Bo98 Bo98 Dec 29, 2022 •
edited
Loading

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.

Copy link
Author

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.

Copy link
Author

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.

wri(phdr.p_align, getPageSize());
}

Expand Down Expand Up @@ -912,12 +916,14 @@ void ElfFile<ElfFileParamNames>::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();
Expand Down
2 changes: 1 addition & 1 deletion src/patchelf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down