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

Avoid overlapping program header table with section header table #457 #460

Merged
merged 7 commits into from
Feb 24, 2023

Conversation

brenoguim
Copy link
Collaborator

This patch does two things to fix the #457 . One could argue they should be separate fixes. They are togethere because they were both needed for the issue in #457

Please take all changes I make with a grain of salt. I just started learning more about ELF and this codebase, so the solution could be way off.

I observed two issues:
1- Program table overlapping with section table
The tool knows that it might need to insert an extra PT_LOAD for the sections that are relocated to the end of the file. And since growing the program table could overlap with other sections, it checks and marks these overlapping sections as replaced. This makes room for the program table to grow. However, in this case, the the program table will grow into the section table which is right after. This patch detects this scenario and moves the section table to the end (before the replaced sections).

2- Multiple PT_LOAD
Everytime a string is changed (in the issue was through --set-interpreter, but in my test uses --set-soname), the string table needs to be sent to the end of the file to be updated. To do that, the tool adds a new PT_LOAD to the program table. If you repeat this process enough times, this will corrupt the library. So this patch detects that a trailing PT_LOAD already exists and extends it.
After this patch, I was able to patch the elf a thousand times and have a runnable binary.
Unfortunately, I could not understand why adding multiple PT_LOADs eventually corrupt the binary. yet.

…S#457

This patch checks if the section header table is placed right after the
program header table such that it would overlap when we add a new entry
in the program header table.
If that is the case, move the section header table to the end of the file.

Moreover, there is no need to add a new PT_LOAD segment everytime.
Check if the last segment is already a PT_LOAD with the same characteristics
and adjacent. Extend it in this case.
src/patchelf.cc Outdated Show resolved Hide resolved
Co-authored-by: Jörg Thalheim <[email protected]>
@brenoguim
Copy link
Collaborator Author

brenoguim commented Feb 19, 2023

Unfortunately, I could not understand why adding multiple PT_LOADs eventually corrupt the binary. yet.

We already knew if you rewrite it twice, the PHT overlaps with SHT.
After the fix that moves the SHT to the end, you can rewrite it about 55 times.

Each of these 55 times move the .interp section to the end and add a new LOAD. The previous loads become useless, except the first that is still needed to load the SHT.

After 55 rewrites, the PHT grows so big that it displaces the next coliding section, the .text section. So both .interp and .text sections are moved. This seems to be the problem.

In the rewriteSectionsExecutable the code seems to be concerned about moving a PROGBITS section, but not int the rewriteSectionsLibrary. Not sure what is the correct thing there.

It's important to use --no-sort to reproduce this second problem. Without it the tool will still fail, but in a different way.

@brenoguim
Copy link
Collaborator Author

Maybe a better fix is to just drop all LOAD entries that do not encompass any section. There is no point in moving the interp section further and further away in the file, leaving a blank space in the middle.

@brenoguim
Copy link
Collaborator Author

After 55 rewrites, the PHT grows so big that it displaces the next coliding section, the .text section. So both .interp and .text sections are moved. This seems to be the problem.

Perhaps it's wrong to displace the .text section because the LOAD segment we add at the end is ReadWrite, while the .text section originally loaded as ReadExec.
I suspect we would have to displace all progbits sections and track the segment attributes such that we create matching attributes. Another option is to shift their offset away from the PHT.

tests/repeated-updates.sh Outdated Show resolved Hide resolved
tests/repeated-updates.sh Outdated Show resolved Hide resolved
@Mic92
Copy link
Member

Mic92 commented Feb 24, 2023

bors merge

@bors bors bot merged commit 65bf3d9 into NixOS:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants