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

Support ARM ELF THM_PC22 / THM_JUMP24 relocs #4503

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

timiv
Copy link
Contributor

@timiv timiv commented May 19, 2024

This revision adds proper support for the following ARM ELF relocations:

  • RZ_ARM_THM_PC22
  • RZ_ARM_THM_JUMP24

These require a rather special encoding as some bits of the relative offset are encoded in a non-common fashion.

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

As of now, rizin implements a rather rudimentary handling of ARM ELF Relocation: it just overwrites the the patch address with the value calculated for the symbol. While this works in some cases, there are others that do not, most notably ARM_THM_PC22 and ARM_THM_JUMP24 that use a quite different encoding.

With this change, the relocations are correctly handled and lead to properly encoded values. Note that this change doesn't handle possible overflows for which some thunk would need to be inserted.

The implementation was heavily inspired by lld's implementation, although the special cases (e.g. BL/BLX patching, missing J1J2 support) are not considered in this change.

I did not extend the documentation as it is not that detailed / does not even exist here.

Test plan

To test the change, it would be sufficient to check that after relocation, the bl instruction to the proper target exists and is not replaced by some unrelated instruction.

Example:

Previously:

┌ sym.mbedtls_md5_init();
│           0x08000034      push  {r3, lr}                             ; RELOC TARGET 32 .text.mbedtls_md5_init @ 0x08000034 ; [04] -r-x section size 12 named .text.mbedtls_md5_init
│           0x08000036      movs  r1, 0
│           0x08000038      movs  r2, 0x58                             ; 'X'
│           ;-- memset:
│           0x0800003a      ldr   r0, [r1, 0xc]                        ; RELOC 32 memset
│           0x0800003c      lsrs  r0, r0, 0x20
└           0x0800003e      pop   {r3, pc}

With this change:

┌ sym.mbedtls_md5_init();
│           0x08000034      push  {r3, lr}                             ; RELOC TARGET 32 .text.mbedtls_md5_init @ 0x08000034 ; [04] -r-x section size 12 named .text.mbedtls_md5_init
│           0x08000036      movs  r1, 0
│           0x08000038      movs  r2, 0x58                             ; 'X'
│           ;-- memset:
│           0x0800003a      bl    memset                               ; RELOC 24 memset
└           0x0800003e      pop   {r3, pc}

I added a test for a binary that contains THM_PC22.

Closing issues

This revision adds proper support for the following ARM ELF relocations:

- RZ_ARM_THM_PC22
- RZ_ARM_THM_JUMP24

These require a rather special encoding as some bits of the relative
offset are encoded in a non-common fashion.
@XVilka
Copy link
Member

XVilka commented May 20, 2024

To add a test you need:

  1. Add a binary to https://github.com/rizinorg/rizin-testbins
  2. Create a test in test/db/formats/elf/ files, e.g. in test/db/formats/elf/elf-relarm

timiv added 2 commits May 20, 2024 11:59
Due to a  misconfiguration some fallback style was used that incorrectly formatted the modified source file.
Add a simple test for ARM_THM_PC22 relocations.
@timiv
Copy link
Contributor Author

timiv commented May 20, 2024

Thanks @XVilka!

I added a test bin with rizinorg/rizin-testbins#151 as a prerequisite for this PR.

@XVilka XVilka merged commit c7464ab into rizinorg:dev May 20, 2024
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants