harden ELF relocation parsing and mmap safety#2
Conversation
Y-0x
commented
May 26, 2026
Y-0x
commented
- Prevent OOB reads and UB in SLEB128 decoding
- Add bounds validation during Android relocation unpacking
- Prevent integer mismatch in relocation count decoding
- Replace MAP_FIXED with MAP_FIXED_NOREPLACE for safer mappings
Fix an Out-of-Bounds (OOB) read bug in sleb128_decode(). - Added early return on buffer overrun. - Added shift overflow check to prevent UB.
Added boundary check to prevent out of bounds heap write in elfutil_unpack_android_relocs() This ensures that parsed relocations do not exceed the pre-allocated buffer size. Also added free()
- Replace MAP_FIXED with MAP_FIXED_NOREPLACE in plti_internal_set_got_entry(). - This prevents unintentional overwriting of existing memory mappings if the hint address is already occupied. - Added a fallback mechanism to allocate memory safely without a hint
|
All Contributors have signed the CLA. The PR is now allowed to be merged. |
|
I have read the CLA Document and I hereby sign the CLA |
| if (decoder->current >= decoder->end) { | ||
| LOGF("Failed to decode SLEB128: buffer overrun"); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
https://android.googlesource.com/platform/bionic/+/refs/heads/main/linker/linker_sleb128.h#40
The code comes from AOSP
There was a problem hiding this comment.
since our LOGF doesn't abort like AOSP's async_safe_fatal, the return 0 is necessary to prevent OOB reads. Do you think we should keep it for safety, or would you prefer a different way to handle this?
| if (shift > size) { | ||
| LOGF("SLEB128 shift overflow"); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
https://android.googlesource.com/platform/bionic/+/refs/heads/main/linker/linker_sleb128.h#40
The code comes from AOSP
| int64_t num_relocs_signed = sleb128_decode(&decoder); | ||
| if (num_relocs_signed <= 0) return false; | ||
| uint64_t num_relocs = (uint64_t)num_relocs_signed; |
There was a problem hiding this comment.
I'll see about that, but there should be no case where it will ever go below 0
There was a problem hiding this comment.
If sleb128_decode returns -1 due to corruption
uint64_t num_relocs = (int64_t)(-1); // → 0xFFFFFFFFFFFFFFFF
if (num_relocs <= 0) // unsigned compare: always false
The check silently passes and calloc(0xFFFFFFFFFFFFFFFF, ...) gets called. The type mismatch is the bug regardless of intent.
| if (out_index >= num_relocs) { | ||
| LOGE("Android reloc: out_index exceeded num_relocs"); | ||
| free(entries); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Follow the project coding style. There should be empty new lines between the LOGE, free and return, moreover the "Android reloc: " shouldn't exist
There was a problem hiding this comment.
Understood, will fix the formatting.
| void *backup_addr = mmap(hint, vma_len, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0); | ||
| void *backup_addr = MAP_FAILED; | ||
| if (hint) | ||
| backup_addr = mmap(hint, vma_len, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED_NOREPLACE, -1, 0); |
There was a problem hiding this comment.
https://man7.org/linux/man-pages/man2/mmap.2.html
MAX_FIXED_NOREPLACE exists only in 4.17+. That would reduce compatibility.
There was a problem hiding this comment.
I've replaced MAP_FIXED with a fallback approach. By attempting the mmap with a hint first, and falling back to NULL if it fails
|
Thanks for the thorough review and for pointing out the AOSP references. It really makes sense to align with the standard implementation to ensure stability and compatibility, especially regarding the sleb128 logic. |
2 participants
