Skip to content

harden ELF relocation parsing and mmap safety #2

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

Closed
Y-0x wants to merge 6 commits into from
Closed

harden ELF relocation parsing and mmap safety #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29e851d
Fix: prevent OOB read and UB in sleb128_decode
Y-0x May 26, 2026
5cdb867
Bug: add heap overflow guard in elfutil_unpack_android_relocs
Y-0x May 26, 2026
2827dac
Bug: prevent integer mismatch in num_relocs decoding
Y-0x May 26, 2026
e5af71e
Fix: prevent memory corruption using MAP_FIXED_NOREPLACE
Y-0x May 26, 2026
6561dda
Style: spaces and remove Android reloc
Y-0x May 26, 2026
f7b5570
Improve: compatibility of the code to run below kernel 4.17
Y-0x May 26, 2026
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
23 changes: 19 additions & 4 deletions src/elf_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,18 @@ static int64_t sleb128_decode(struct sleb128_decoder *decoder) {
const size_t size = sizeof(int64_t) * CHAR_BIT;

do {
if (decoder->current >= decoder->end)
LOGF("Failed to decode SLEB128: buffer overrun");
if (decoder->current >= decoder->end) {
LOGF("Failed to decode SLEB128: buffer overrun");
return 0;
}
Comment on lines +83 to +86

@ThePedroo ThePedroo May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://android.googlesource.com/platform/bionic/+/refs/heads/main/linker/linker_sleb128.h#40

The code comes from AOSP

@Y-0x Y-0x May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


byte = *decoder->current++;
value |= ((int64_t)(byte & 0x7F)) << shift;
shift += 7;
if (shift > size) {
LOGF("SLEB128 shift overflow");
return 0;
}
Comment on lines +91 to +94

@ThePedroo ThePedroo May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://android.googlesource.com/platform/bionic/+/refs/heads/main/linker/linker_sleb128.h#40

The code comes from AOSP

@Y-0x Y-0x May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^
|

} while (byte & 0x80);

if (shift < size && (byte & 0x40)) {
Expand Down Expand Up @@ -327,8 +333,9 @@ static bool elfutil_unpack_android_relocs(const struct elf_image *elf, struct an
struct sleb128_decoder decoder;
sleb128_decoder_init(&decoder, (const uint8_t *)elf->rel_android_, elf->rel_android_size_);

uint64_t num_relocs = sleb128_decode(&decoder);
if (num_relocs <= 0) return false;
int64_t num_relocs_signed = sleb128_decode(&decoder);
if (num_relocs_signed <= 0) return false;
uint64_t num_relocs = (uint64_t)num_relocs_signed;
Comment on lines +336 to +338

@ThePedroo ThePedroo May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see about that, but there should be no case where it will ever go below 0

@Y-0x Y-0x May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


size_t out_index = 0;
void *entries = calloc(num_relocs, elf->rel_android_is_rela_ ? sizeof(ElfW(Rela)) : sizeof(ElfW(Rel)));
Expand Down Expand Up @@ -397,6 +404,14 @@ static bool elfutil_unpack_android_relocs(const struct elf_image *elf, struct an
if (elf->rel_android_is_rela_ && group_flags_reloc == RELOCATION_GROUP_HAS_ADDEND_FLAG)
r_addend += sleb128_decode(&decoder);

if (out_index >= num_relocs) {
LOGE("out_index exceeded num_relocs");

free(entries);

return false;
}
Comment on lines +407 to +413

@ThePedroo ThePedroo May 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow the project coding style. There should be empty new lines between the LOGE, free and return, moreover the "Android reloc: " shouldn't exist

@Y-0x Y-0x May 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, will fix the formatting.


if (elf->rel_android_is_rela_) {
ElfW(Rela) *rela = (ElfW(Rela) *)entries;
rela[out_index].r_offset = current_offset;
Expand Down
4 changes: 3 additions & 1 deletion src/plti.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,9 @@ static bool plti_internal_set_got_entry(struct elf_info *info, uintptr_t got_add
if (!stash) {
/* INFO: Don't call LOGE -- or else it will try to access GOT while it's being modified */
void *hint = find_high_backup_hint(vma_len);
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, -1, 0);
if (backup_addr == MAP_FAILED) {
backup_addr = mmap(NULL, vma_len, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
if (backup_addr == MAP_FAILED) {
Expand Down
Loading