Fix/5259 x86 x86 64 missing elf relocs#6100
Conversation
…32/128/16 and IRELATIVE)
This adds conversion and patching support for the missing x86 and x86_64 ELF relocations reported in the issue. Closes rizinorg#5259
There was a problem hiding this comment.
Please add a test by subimitting a binary to rizin-testbins with the relocs. I'd recommend to check out tests of compiler projects like LLVM to find such binary source code. Or write it yourself of course.
Then you can add a test here. Please mark it ready for review when this is done.
Also, please link to the document you used as reference.
|
Hello @Rot127, thanks for reviewing. For the test bins I will write up a quick C file using __thread to trigger the TLS relocs and then i will compile it for both x86 and x86_64, and open a PR over on rizin-testbins and once thats done then I'll add the YAML tests to this PR. For the math/conversions, I just pulled the formulas straight from the System V ABI Docs to make sure I got the DTPMOD/DTPOFF offsets exactly right. x86_64: [System V ABI AMD64 Architecture Processor Supplement (PDF)] i386: [System V ABI Intel386 Architecture Processor Supplement (PDF)] Now I will work on getting those test binaries compiled. Will mark this as ready for review as soon as the tests are passing locally. |
There was a problem hiding this comment.
Please also check if the patching works
|
Hey @Rot127, thanks for the guidance. I've created the custom C binaries to force the TLS relocations, and added external variables/functions to catch the GOT/PLT and IRELATIVE relocations too. They are submitted along with the source code to rizin-testbins here:- I've also added the automated test block to test/db/formats/elf/elf-rel-x86_64 in this PR as requested. I know the CI will fail until the testbins are merged but I have manually run Rizin against the new binaries locally and verified that all the nhandled warnings are completely resolved. |
|
Hello @Rot127 !! I just dug into librz/bin/format/elf/elf_relocs_patching.c to check on the patching. It turns out the mathematical patching logic for all of these (R_X86_64_GLOB_DAT, JUMP_SLOT, DTPMOD64, DTPOFF64, TPOFF64, and IRELATIVE) is actually already implemented natively in the patch_reloc_x86_64 switch statement! Since the patching logic was already there, parsing the names to resolve the Unhandled warnings was the only missing piece. I verified it locally and the patching works smoothly. Let me know if everything looks good to go |
I can't find it. Please show me where. |
|
Sorry for the mix-up earlier, @Rot127 I've just pushed a commit implementing the correct System V ABI math (fs−>S+fs−>A) for both patch_reloc_x86_64 and patch_reloc_x86_32. |
|
@inevitableflash ping, please also rebase. |
Your checklist for this pull request
RZ_APIfunction and struct this PR changes.RZ_API).Detailed description
I saw issue #5259 and since I already had some context from working on similar ARM64 relocations recently, I thought I could tackle the x86/x86_64 ones too.
When I first looked at the elf_relocs_conversion.c file, I wasn't completely sure about the exact formulas needed for the TLS relocations. So, I spent some time looking up the official System V ABI documentation for both i386 and AMD64 online to make sure I got the math right (specially understanding how DTPMOD and DTPOFF differ).
While going through the code, I noticed that R_386_PLT32 was completely missing from the switch statement, so I added that as a new case. For the rest of the missing ones listed in the issue, I updated them from UNHANDL to the proper ADD() macros using the ABI specs I found.
After getting the conversions down, I went into elf_relocs_patching.c and added the corresponding patching cases (writing 32-bit values for i386 and the correct 64-bit/32-bit values for x86_64).
I also made sure to run clang-format on both files before committing so the formatting is strictly in line with the project's style.
...
Test plan
I built the project locally to ensure everything compiles cleanly: [ninja -C build]
Then, to verify the warnings were actually gone, I tested it against standard system binaries that heavily use TLS (like libc). I ran the following command before my changes to see the unhandled relocation warnings, and then ran it again after my changes to confirm they disappeared:
rz-bin -r /usr/lib/x86_64-linux-gnu/libc.so.6 2>&1 | grep "Unhandled"
rz-bin -r /usr/lib/i386-linux-gnu/libc.so.6 2>&1 | grep "Unhandled"
The specific warnings for relocations 16, 17, 18, 37, 42 (on x86_64) and the corresponding i386 ones no longer show up.
...
Closing issues
Closes #5259
...