Enforce map-function compatibility checks#1127
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements bidirectional map-function compatibility enforcement for eBPF helpers. New type contracts define which map types each helper permits, prototypes are populated with these restrictions, and assertions extracted during call analysis are validated during abstract interpretation to reject invalid map accesses. ChangesMap-Function Compatibility Validation
Possibly Related PRs
🚥 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f657b8d1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crab/ebpf_checker.cpp`:
- Around line 294-295: The bit-shift uses uint64_t{1} << *map_type which is UB
if *map_type >= 64 because EbpfMapDescriptor::type isn't clamped; before
performing the shift in the check around s.allowed_map_types, first validate
*map_type is less than 64 (or otherwise within the mask width) and throw_fail
with a clear message if out of range, or rewrite the condition to short-circuit
(e.g. require *map_type < 64 && (s.allowed_map_types & (1ULL << *map_type))) so
the shift never happens with an invalid value; update the check that currently
calls throw_fail("map type " + std::to_string(*map_type) + " is not allowed for
" + s.helper_name) accordingly.
In `@src/spec/function_prototypes.hpp`:
- Line 11: The helper map_type_bit performs a left shift that is UB when type >=
64; update map_type_bit to guard against out‑of‑range shifts (e.g. return 0 or
another sentinel when type >= 64) by checking the input before shifting so the
expression uint64_t{1} << type is only executed for type < 64; modify the
constexpr function map_type_bit(uint32_t type) to include that bounds check to
make it safe for runtime-derived map IDs.
🪄 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: c23c0ce0-6582-4f6d-9ef0-136992c233b7
📒 Files selected for processing (8)
src/crab/ebpf_checker.cppsrc/ir/assertions.cppsrc/ir/call_resolver.cppsrc/ir/syntax.hppsrc/linux/gpl/spec_prototypes.cppsrc/printing.cppsrc/result.cppsrc/spec/function_prototypes.hpp
Add allowed_map_types bitmask to EbpfHelperPrototype, threaded through CallContract and checked via a new ValidMapType assertion. This encodes the function→map direction of the kernel's check_map_func_compatibility (v6.18): each helper that requires specific map types now declares them, and the checker rejects mismatches at verification time. Covers 34 helpers across perf_event, ringbuf, storage, redirect, queue/ stack, cgroup, and reuseport families. Map type UNSPEC (0) is treated as unknown and silently allowed to avoid false positives from incomplete ELF metadata. Closes #1123 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
map_type_bit() and the ValidMapType checker now handle map types >= 64 without undefined behavior. map_type_bit returns 0 for out-of-range types; the checker skips the check (same as UNSPEC). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/crab/ebpf_checker.cpp`:
- Around line 298-300: The check around map_type currently returns early for
missing/zero/>=64 values which silently accepts the path; change it to
fail-closed: when map_type is not present, ==0, or >=64 treat the call as
invalid and explicitly reject the path using the checker’s existing rejection
mechanism (e.g., return false / call the verifier-reject helper used elsewhere
in ebpf_checker.cpp) instead of just returning; ensure you update the branch
that references map_type to use this reject behavior so malformed or future map
IDs do not bypass verification.
🪄 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: 9e10954e-9bbe-473f-bb43-e0efb2220585
📒 Files selected for processing (8)
src/crab/ebpf_checker.cppsrc/ir/assertions.cppsrc/ir/call_resolver.cppsrc/ir/syntax.hppsrc/linux/gpl/spec_prototypes.cppsrc/printing.cppsrc/result.cppsrc/spec/function_prototypes.hpp
Map types >= 64 now fail verification rather than skipping the compatibility check, preventing malformed or future map IDs from bypassing the constraint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Elazar Gershuni <elazarg@gmail.com>
|
Addressed CodeRabbit review: map types >= 64 now fail-closed (e77e5e1) instead of silently skipping the check. |
1 participant
Summary
allowed_map_typesbitmask toEbpfHelperPrototypeandCallContractValidMapTypeassertion emitted when a helper with restricted map types receives a map fd argumentcheck_map_func_compatibilityfunction→map directionDetails
The kernel enforces bidirectional map-function compatibility: certain map types may only be used with specific helpers, and certain helpers may only receive specific map types. Prevail had no such check — any map type was accepted by any helper with a
PTR_TO_MAPargument (excepttail_call, which usesPTR_TO_MAP_OF_PROGRAMS).This PR encodes the function→map direction. Map type UNSPEC (0) is treated as unknown and silently allowed to avoid false positives from incomplete ELF metadata. The reverse direction (map→function, e.g., "PERF_EVENT_ARRAY may not be passed to map_update_elem") is left for a follow-up.
Test plan
valid_map_type(r1, ringbuf_output))Closes #1123
Ref: #68
🤖 Generated with Claude Code