Extend rbran's parser robustness fixes with file-backed bounds and configurable limits (ELF) - #8356
Open
Weitao-Sun wants to merge 10 commits into
Open
Extend rbran's parser robustness fixes with file-backed bounds and configurable limits (ELF)#8356Weitao-Sun wants to merge 10 commits into
Weitao-Sun wants to merge 10 commits into
Conversation
Replace magic-number loop guards in MIPS GOT processing with segment-derived limits and configurable budgets. Fix integer underflow when localMipsSyms is zero. Correct string table validation to compare against file length rather than virtual address space. Add IsRangeBackedByFile helper that validates every byte in the given range is file-backed, and guard against overflow when a program header's declared file extent is used to bound the GOT entry count. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Mirrors the Linux kernel pattern (fs/binfmt_elf.c) of bounding by total byte size rather than entry count, so the same limit applies equally to 32-bit and 64-bit MIPS ELF files regardless of entry size. The setting files.elf.maxMipsGotMB (default 4 MB) is divided by the per-entry size at load time to derive the effective entry limit. Ref: https://github.com/torvalds/linux/blob/master/fs/binfmt_elf.c Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
emesare
requested changes
Aug 19, 2026
| return false; | ||
| if (size > UINT64_MAX - start) | ||
| return false; | ||
| for (uint64_t offset = start; offset < start + size; offset++) |
Member
There was a problem hiding this comment.
This seems quite expensive, would it not be okay to just check the start and end? I see you mention it in the PR description but what is this guarding against?
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback — I've adopted your suggested fix, pushed in d186a75.
Replace the per-GOT-entry IsRangeBackedByFile() loop (which called IsOffsetBackedByFile() once per byte per entry) with a single bound, maxGotEntries, computed once from the already-known file-backed extent of the segment containing gotStart. Each entry then only needs an index comparison against that bound. Also fix the segment lookup used to compute that extent: it was comparing the unadjusted hdr.virtualAddress against gotStart, which already has imageBaseAdjustment applied, so the match against the containing PT_LOAD segment silently failed whenever the image's load address differs from its declared one. Addresses review feedback from emesare on #8356. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # view/elf/elfview.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Split out from #8303 per review feedback, containing the ELF portion only. (Mach-O is being merged in #8303; PE is a separate PR.)
Builds on rbran's parser robustness fixes with the following improvements:
ReadStringTable: rejects sections whose declared size exceeds the actual file length, and fixes an off-by-one in the offset check, preventing reads past the end of the buffer.Init: fixes an integer underflow whenlocalMipsSymsis zero.ApplyTypesToParentStringTable: boundssection.sizeto a configurable setting (files.elf.maxStringTableTypeSizeMB, default 4 MB) rather than trusting the declared section size directly.IsRangeBackedByFile, which validates every byte in a range is file-backed (rather than only the first/last byte), and uses it to guard the MIPS GOT entry count against aPT_LOADsegment's declared file extent exceeding the actual file length (with overflow checking).files.elf.maxMipsGotMB(default 4 MB, max 64 MB), mirroringfs/binfmt_elf.c's byte-size-based approach so the same limit applies equally regardless of 32/64-bit entry size.