Fix heap-buffer-overflow from non-instruction-aligned FUNC symbols#1106
Fix heap-buffer-overflow from non-instruction-aligned FUNC symbols#1106
Conversation
Reject ELF FUNC symbols in executable sections whose st_value is not a multiple of sizeof(EbpfInst) (8 bytes). A malformed ELF with such a symbol causes get_program_name_and_size() to produce non-aligned program boundaries. When read_programs() advances offset by a non-aligned symbol_size, compute_reachable_program_span() uses truncating integer division (offset / sizeof(EbpfInst)), inflating the computed span and causing vector_of<EbpfInst> to memcpy past the section data buffer. The root-cause fix validates FUNC symbol alignment in get_program_name_and_size(), which is shared by both read_programs() and ElfObjectState::discover_programs(). A defense-in-depth bounds check before the vector_of call in read_programs() guards against future regressions in span computation. Add a test that constructs a minimal ELF with a FUNC symbol at an unaligned offset and verifies it is cleanly rejected. Signed-off-by: Michael Agun <danielagun@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Michael Agun <danielagun@microsoft.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds input validation to the ELF reader to reject non-instruction-aligned STT_FUNC symbol offsets and ensure program spans remain within section boundaries. A corresponding test validates this validation behavior. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/test_elf_loader.cpp`:
- Around line 584-638: Add a companion test mirroring TEST_CASE("ELF loader
rejects non-instruction-aligned FUNC symbol", "[elf][hardening]") that proves
the aligned case is accepted: create the same ELF (.text, 32 exit instructions,
strtab/symtab) but when adding the second STT_FUNC symbol (currently added via
sym_writer.add_symbol for "prog_b") use an 8-byte aligned offset (e.g., 8)
instead of 7 and an appropriate size, then call read_elf(in_stream, "memory",
".text", "", options, &g_ebpf_platform_linux) and assert it succeeds (use
REQUIRE_NOTHROW or equivalent) so the test verifies the aligned-FUNC path is
accepted; reuse the same helpers/variables (text_sec, sym_writer, str_writer,
read_elf) and give the test a clear name like "ELF loader accepts
instruction-aligned FUNC symbol".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f9e7f9f-9d5d-456e-88c0-e01e2a0dfabe
📒 Files selected for processing (2)
src/io/elf_reader.cppsrc/test/test_elf_loader.cpp
Companion to the rejection test — verifies that FUNC symbols at 8-byte-aligned offsets are accepted without error, ensuring the alignment validation does not reject well-formed ELF files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Michael Agun <danielagun@microsoft.com>
2 participants
Summary
Reject STT_FUNC symbols in executable sections whose st_value is not a multiple of sizeof(EbpfInst) (8 bytes). Without this validation, a malformed ELF can cause a heap-buffer-overflow in vector_of during program loading.
Fixes #1105
Changes
Root-cause fix — get_program_name_and_size() (elf_reader.cpp):
Defense-in-depth — ProgramReader::read_programs() (elf_reader.cpp):
Test — test_elf_loader.cpp:
get_program_name_and_size() Root cause analysis
The overflow originates from compute_reachable_program_span() performing truncating integer division (program_offset / sizeof(EbpfInst)) on a non-aligned byte offset. But the non-aligned offset enters the system through get_program_name_and_size(), which uses FUNC symbol values as program boundaries without alignment validation.
Fixing at this shared helper protects all downstream consumers, including the discover_programs() path in elf_loader.cpp which has the same offset += size loop.