Fix stack access with bpf2bpf calls #721
Conversation
WalkthroughThe pull request introduces significant modifications across multiple files, focusing on stack size management and control flow graphs (CFG) in eBPF. Key changes include the addition of constants for stack size limits, updates to function logic for handling function macros, and enhancements to local stack variable management. New test cases validate stack behavior during function calls and invalid access scenarios, ensuring the integrity of stack operations is maintained. Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
dthaler
force-pushed
the
bpf2bpf-stack
branch
2 times, most recently
from
c2b67c7 to
8aaff6a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range comments (4)
src/crab/bitset_domain.cpp (1)
Line range hint
1-63: Consider passingnon_numerical_bytesby const reference for performance optimization.The static analysis tool suggests passing the
non_numerical_bytesparameter by const reference in theoperator<<function. This change can improve performance by avoiding unnecessary copying of the parameter.Consider applying the following change:
-std::ostream& operator<<(std::ostream& o, const bitset_domain_t& b) { +std::ostream& operator<<(std::ostream& o, const bitset_domain_t& b, const std::vector<bool>& non_numerical_bytes) {Note: This change may require adjustments to the function call sites and the
bitset_domain_tclass definition. Please verify the impact on the overall codebase before implementing this optimization.🧰 Tools
🪛 cppcheck
[performance] 18-18: Function parameter 'non_numerical_bytes' should be passed by const reference.
(passedByValue)
src/crab/array_domain.cpp (3)
Line range hint
4-4: Consider creating issues for TODO itemsThere are several TODO comments throughout the file. It would be beneficial to create GitHub issues to track these improvements and ensure they're not overlooked in future development.
Would you like me to create GitHub issues for each TODO item found in this file?
Line range hint
70-70: Review the use of global stateThe code uses a global array map (
global_array_map) and thread-local storage. While this might be necessary for the current design, it's worth considering if there are alternatives that could improve thread safety and scalability, especially if this code might be used in a multi-threaded context in the future.
Line range hint
576-576: Enhance warning messages for better debuggingThe CRAB_WARN statements throughout the code provide valuable debugging information. Consider enhancing some of these messages to provide more context or specific details about the state when the warning is triggered. For example, in this line, you could include the actual values of
iandelem_sizein the warning message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (12)
- src/asm_cfg.cpp (0 hunks)
- src/asm_unmarshal.cpp (1 hunks)
- src/assertions.cpp (1 hunks)
- src/crab/array_domain.cpp (1 hunks)
- src/crab/bitset_domain.cpp (2 hunks)
- src/crab/bitset_domain.hpp (4 hunks)
- src/crab/ebpf_domain.cpp (7 hunks)
- src/ebpf_base.h (1 hunks)
- src/ebpf_yaml.cpp (4 hunks)
- src/spec_type_descriptors.hpp (0 hunks)
- test-data/calllocal.yaml (1 hunks)
- test-data/stack.yaml (1 hunks)
💤 Files with no reviewable changes (2)
- src/asm_cfg.cpp
- src/spec_type_descriptors.hpp
🧰 Additional context used
🪛 cppcheck
src/crab/bitset_domain.cpp
[performance] 18-18: Function parameter 'non_numerical_bytes' should be passed by const reference.
(passedByValue)
🔇 Additional comments (23)
src/crab/bitset_domain.cpp (6)
17-17: LGTM! Consistent update of stack size reference.The change correctly updates the output statement to use
EBPF_TOTAL_STACK_SIZE, maintaining consistency with the previous modifications and ensuring correct output of stack indices.
20-25: LGTM! Consistent updates throughout the function.The changes correctly update array indexing and output statements to use
EBPF_TOTAL_STACK_SIZE, maintaining consistency throughout the function and ensuring correct handling of non-numerical bytes and stack indices.
43-44: LGTM! Consistent updates in theto_setmethod.The changes correctly update the loop range and array indexing to use
EBPF_TOTAL_STACK_SIZEin theto_setmethod, maintaining consistency with the modifications in theoperator<<function and ensuring correct iteration over the entire stack space.
47-47: LGTM! Consistent update in string construction.The change correctly updates the string construction to use
EBPF_TOTAL_STACK_SIZE, maintaining consistency with the previous modifications and ensuring correct representation of stack indices in the resulting set.
50-55: LGTM! Consistent updates throughout theto_setmethod.The changes correctly update array indexing and string construction to use
EBPF_TOTAL_STACK_SIZE, maintaining consistency throughout theto_setmethod and ensuring correct handling of non-numerical bytes and representation of stack indices in the resulting set.
9-10: LGTM! Verify constant usage across the codebase.The changes correctly update the stack size references from
EBPF_STACK_SIZEtoEBPF_TOTAL_STACK_SIZE, aligning with the PR objective of allocating 512 bytes of stack space for each call level.Let's verify the constant usage across the codebase:
✅ Verification successful
Verified:
EBPF_STACK_SIZEhas been successfully replaced withEBPF_TOTAL_STACK_SIZEthroughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of EBPF_TOTAL_STACK_SIZE constant # Test: Search for EBPF_TOTAL_STACK_SIZE usage rg --type cpp "EBPF_TOTAL_STACK_SIZE" # Test: Check if EBPF_STACK_SIZE is still used rg --type cpp "EBPF_STACK_SIZE"Length of output: 3251
src/ebpf_base.h (1)
41-47: Summary: Excellent additions for improved stack managementThese new constants provide a solid foundation for better stack management in eBPF programs, directly addressing the objectives outlined in the PR. The
MAX_CALL_STACK_FRAMESlimits the call depth,EBPF_SUBPROGRAM_STACK_SIZEallocates space for each call, andEBPF_TOTAL_STACK_SIZEdefines the overall stack limit. These changes should effectively resolve the issues with local stack variables in bpf2bpf calls mentioned in the linked issue #720.src/crab/bitset_domain.hpp (5)
67-67: LGTM! Consistent with the EBPF_TOTAL_STACK_SIZE change.The update to use EBPF_TOTAL_STACK_SIZE in the width calculation is correct and consistent with the overall changes in this PR.
85-85: LGTM! Consistent upper bound check.The update to use EBPF_TOTAL_STACK_SIZE for the upper bound check is correct and aligns with the PR objectives.
92-92: LGTM! Consistent width calculation.The update to use EBPF_TOTAL_STACK_SIZE in the width calculation for the reset method is correct and consistent with the PR changes.
Line range hint
1-124: Overall LGTM! Verify EBPF_TOTAL_STACK_SIZE definition.The changes in this file consistently replace EBPF_STACK_SIZE with EBPF_TOTAL_STACK_SIZE, which aligns with the PR objectives. The logic of the bitset_domain_t class remains intact, with only the constant being updated.
To ensure the correctness of these changes, please verify the definition of EBPF_TOTAL_STACK_SIZE in ebpf_base.h:
#!/bin/bash # Description: Check the definition of EBPF_TOTAL_STACK_SIZE rg --type cpp "#define\s+EBPF_TOTAL_STACK_SIZE" ebpf_base.hEnsure that EBPF_TOTAL_STACK_SIZE is defined correctly and its value is appropriate for the intended stack space allocation in bpf2bpf calls.
12-12: LGTM! Verify impact on memory usage.The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE is consistent with the PR objectives.
Please verify that this change doesn't significantly increase memory usage in the rest of the codebase. Run the following script to check for other occurrences of
std::bitsetwithEBPF_STACK_SIZE:test-data/calllocal.yaml (1)
337-371: Excellent addition of the "call local with stack" test case.This new test case is well-structured and directly addresses the PR objectives. It effectively verifies:
- The behavior of stack variables during nested function calls.
- The isolation of stack changes in subfunctions from the caller's stack.
- Correct calculation of values across multiple stack frames.
The test case is crucial for ensuring the proper implementation of allocating separate stack space for each call level, as described in the PR objectives.
src/assertions.cpp (1)
172-172: LGTM. Verify theEBPF_SUBPROGRAM_STACK_SIZEconstant.The change from
EBPF_STACK_SIZEtoEBPF_SUBPROGRAM_STACK_SIZEaligns with the PR objective of allocating separate stack space for each call level. This modification ensures that the assertion correctly checks the stack access bounds for individual subprograms.To ensure the constant is correctly defined, please run the following script:
This script will help verify that
EBPF_SUBPROGRAM_STACK_SIZEis correctly defined and used consistently throughout the codebase, and that there are no remaining instances ofEBPF_STACK_SIZEthat should have been updated.✅ Verification successful
Verified. The constant
EBPF_SUBPROGRAM_STACK_SIZEis correctly defined insrc/ebpf_base.hand is used consistently throughout the codebase. Additionally, there are no remaining instances ofEBPF_STACK_SIZE, ensuring that the replacement aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and value of EBPF_SUBPROGRAM_STACK_SIZE # Search for the definition of EBPF_SUBPROGRAM_STACK_SIZE echo "Searching for EBPF_SUBPROGRAM_STACK_SIZE definition:" rg --type cpp "define\s+EBPF_SUBPROGRAM_STACK_SIZE" -A 1 # Check if EBPF_SUBPROGRAM_STACK_SIZE is used consistently echo "Checking usage of EBPF_SUBPROGRAM_STACK_SIZE:" rg --type cpp "EBPF_SUBPROGRAM_STACK_SIZE" # Search for any remaining usage of EBPF_STACK_SIZE echo "Checking for any remaining usage of EBPF_STACK_SIZE:" rg --type cpp "EBPF_STACK_SIZE"Length of output: 1241
test-data/stack.yaml (1)
575-597: Overall assessment of the new test caseThe addition of the "invalid stack access" test case is a valuable contribution to the test suite. It aligns well with the PR objectives of improving stack space management and handling of local variables in bpf2bpf calls. This test case will help ensure that out-of-bounds write operations are properly detected and prevented.
With the suggested improvements, this test case will provide a robust check for invalid stack accesses, enhancing the overall quality and reliability of the eBPF implementation.
src/crab/array_domain.cpp (1)
819-819: LGTM! This change aligns with the PR objectives.The modification from
EBPF_STACK_SIZEtoEBPF_TOTAL_STACK_SIZEappears to address the issue of stack space allocation for bpf2bpf calls. This change likely ensures that each call level has its own allocated stack space, preventing potential overwrites of local variables between different call levels.To ensure consistency, let's check for any other occurrences of
EBPF_STACK_SIZEthat might need to be updated:src/asm_unmarshal.cpp (1)
341-341: Improved stack access bounds checkingThe condition for checking stack access bounds has been updated from
-EBPF_STACK_SIZEto-EBPF_TOTAL_STACK_SIZE. This change appears to be related to the PR objectives, which mention allocating 512 bytes of stack space for each call level instead of reusing the same 512 bytes across all call levels.To ensure this change is consistent with other parts of the codebase, let's verify the usage of
EBPF_TOTAL_STACK_SIZE:This will help us confirm that the change is applied consistently and that there are no other instances where
EBPF_STACK_SIZEshould be replaced withEBPF_TOTAL_STACK_SIZE.✅ Verification successful
Verified: Stack access bounds checking update is consistent
The replacement of
EBPF_STACK_SIZEwithEBPF_TOTAL_STACK_SIZEat line 341 insrc/asm_unmarshal.cppis consistent with its usage throughout the codebase.EBPF_STACK_SIZEdoes not appear to be used elsewhere, ensuring that the update does not introduce inconsistencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of EBPF_TOTAL_STACK_SIZE and EBPF_STACK_SIZE echo "Occurrences of EBPF_TOTAL_STACK_SIZE:" rg "EBPF_TOTAL_STACK_SIZE" --type cpp echo "Occurrences of EBPF_STACK_SIZE:" rg "EBPF_STACK_SIZE" --type cppLength of output: 3409
src/ebpf_yaml.cpp (4)
292-297: Verify Stack Offset Calculations withEBPF_TOTAL_STACK_SIZEIn lines 292-297, the stack offsets are calculated using
EBPF_TOTAL_STACK_SIZE. Ensure that replacingEBPF_STACK_SIZEwithEBPF_TOTAL_STACK_SIZEcorrectly reflects the total stack size, especially considering the changes for handling stack sizes in bpf2bpf calls. This is crucial to prevent inconsistencies in stack handling across recursive calls.
299-299: Check the Initialization ofoffsetOn line 299,
offsetis initialized usingint offset = EBPF_TOTAL_STACK_SIZE - gsl::narrow<int>(memory_bytes.size());. Please confirm that this calculation accurately represents the starting offset for stack operations, particularly whenmemory_bytes.size()can vary. Incorrect initialization could lead to misaligned memory accesses.
309-311: Validate Termination Condition inwhileLoopThe
whileloop starting at line 309 uses the conditionoffset < EBPF_TOTAL_STACK_SIZE. Verify that this termination condition is appropriate and that it ensures all necessary stack variables are processed without exceeding the stack boundaries. This is important to prevent potential infinite loops or stack overflows.
281-282:⚠️ Potential issuePotential Pointer Arithmetic Issue in
memcpyOperationsThe pointer arithmetic in lines 281-282 may lead to incorrect memory access. The expression
memory_bytes.data() + offset + memory_bytes.size() - EBPF_TOTAL_STACK_SIZEis complex and could result in out-of-bounds reads. Please verify that this calculation correctly computes the intended memory address withinmemory_bytesand that it does not cause buffer overruns or underruns.src/crab/ebpf_domain.cpp (2)
Line range hint
822-833: Constraints on offsets are correctly updatedThe added constraints for
r.stack_offset,r.shared_offset, andr.packet_offsetensure that these offsets remain within valid ranges, enhancing the correctness of the domain calculations.
915-916: Clarified comments improve code readabilityThe updated comments clearly explain the purpose of creating variables specific to the new call stack frame, which aids in understanding the subsequent code.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/crab/ebpf_domain.cpp (7 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/crab/ebpf_domain.cpp (4)
822-822: Ensuringstack_offsetdoes not exceed total stack sizeThe added constraint
inv += r.stack_offset <= EBPF_TOTAL_STACK_SIZE;correctly enforces that thestack_offsetremains within the bounds of the total stack size. This helps prevent stack overflows and ensures memory safety.
927-935: Do not assumer10_intervalis a singleton before dereferencingAt line 930, the code assumes that
r10_intervalis a singleton when callingr10_interval.singleton()->cast_to<int32_t>();without checkingr10_interval.is_singleton(). Ifr10_intervalis not a singleton, this could lead to a null pointer dereference or undefined behavior.
1199-1203: Do not assumer10_intervalis a singleton before dereferencingSimilar to a previous issue, at line 1200, the code assumes
r10_intervalis a singleton without checking before dereferencing. This could result in errors ifr10_intervalis not a singleton.
2045-2048: Do not assumer10_intervalis a singleton before dereferencingAt line 2047, the code dereferences
r10_interval.singleton()without verifying that it is indeed a singleton. This might cause issues ifr10_intervalis not a singleton.
dthaler
force-pushed
the
bpf2bpf-stack
branch
from
e32314c to
8a1c36a
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range comments (4)
test-data/call.yaml (1)
Line range hint
297-411: LGTM: Consistent and valuable improvements to BPF function test casesThe changes across multiple test cases in this file are consistent and valuable. They primarily focus on ensuring that the stack pointer (r10) remains unchanged after various BPF function calls, which is crucial for maintaining stack integrity. The additions also improve test coverage for handling different input types and error conditions.
For consistency, consider applying similar improvements to other test cases in this file that haven't been modified in this change. This would ensure uniform coverage across all BPF function tests.
test-data/jump.yaml (1)
Line range hint
496-553: Summary: Good additions to map_fd test cases, with one point needing clarificationThe new test cases for map_fd operations are generally well-constructed and provide good coverage for equality and inequality scenarios. They help ensure correct handling of map_fds in various situations, including self-comparisons and comparisons between different map_fds.
However, the last test case (assume map_fd1 < map_fd2) raises an important question about the intended behavior of map_fd comparisons. It's crucial to clarify whether such comparisons should be allowed and, if so, how they should be handled by the verifier.
Action item:
- Clarify the intended behavior for map_fd comparisons and update either the test case or the verifier logic accordingly.
Consider documenting the allowed operations for map_fds in a central location, such as a README or a documentation file. This will help maintain consistency across the codebase and make it easier for developers to understand the expected behavior of map_fds.
src/crab/array_domain.cpp (2)
Line range hint
819-824: Consider updating related comments or documentationThe constant name change from
EBPF_STACK_SIZEtoEBPF_TOTAL_STACK_SIZEis a good improvement for clarity. To maintain consistency, it would be beneficial to review and update any related comments or documentation that might reference the old constant name.Please check if there are any comments or documentation (including in other files or external documents) that mention
EBPF_STACK_SIZEand update them to reflect the newEBPF_TOTAL_STACK_SIZEconstant name.
Line range hint
1-824: Address TODOs and consider refactoring complex methodsWhile reviewing the entire file, I noticed several areas that could benefit from further attention:
There are multiple TODO comments throughout the file. It would be beneficial to address these or create issues to track them.
Some methods, like
get_overlap_cellsandstore, have complex implementations that might benefit from refactoring for improved readability and maintainability.Consider the following actions:
Review and address the TODO comments, particularly:
- Line 4: "TODO: check array is the array associated to this offset map"
- Line 286: "TODO: we can apply here "Value Recomposition" a la Mine'06"
- Line 314: "TODO: we can be more precise here"
Evaluate complex methods for potential refactoring. For example, the
get_overlap_cellsmethod (around line 186) could potentially be split into smaller, more focused functions to improve readability and maintainability.Consider adding more inline comments to explain complex logic, especially in methods like
storeandsplit_and_find_var.Would you like me to create GitHub issues to track these refactoring and improvement tasks?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (19)
- src/asm_cfg.cpp (0 hunks)
- src/asm_syntax.hpp (2 hunks)
- src/asm_unmarshal.cpp (1 hunks)
- src/assertions.cpp (5 hunks)
- src/crab/array_domain.cpp (1 hunks)
- src/crab/bitset_domain.cpp (2 hunks)
- src/crab/bitset_domain.hpp (4 hunks)
- src/crab/ebpf_domain.cpp (8 hunks)
- src/crab/ebpf_domain.hpp (2 hunks)
- src/ebpf_base.h (1 hunks)
- src/ebpf_yaml.cpp (4 hunks)
- src/spec_type_descriptors.hpp (0 hunks)
- test-data/atomic.yaml (21 hunks)
- test-data/call.yaml (5 hunks)
- test-data/calllocal.yaml (1 hunks)
- test-data/callx.yaml (6 hunks)
- test-data/jump.yaml (2 hunks)
- test-data/stack.yaml (1 hunks)
- test-data/subtract.yaml (3 hunks)
💤 Files with no reviewable changes (2)
- src/asm_cfg.cpp
- src/spec_type_descriptors.hpp
🧰 Additional context used
📓 Learnings (4)
src/crab/ebpf_domain.cpp (3)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:2924-2924 Timestamp: 2024-10-11T20:48:03.575Z Learning: When code in a pull request is unchanged and an issue is pre-existing, avoid raising comments about it.Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:2060-2060 Timestamp: 2024-10-11T20:49:12.091Z Learning: `r10` is read-only and always set to a fixed value, so in `do_mem_store`, `r10_interval` can be assumed to be a singleton without additional checks.Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:0-0 Timestamp: 2024-10-11T20:57:35.938Z Learning: In the function `restore_callee_saved_registers` in `src/crab/ebpf_domain.cpp`, the variable `r10_interval` is always a singleton.src/ebpf_base.h (1)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/ebpf_base.h:43-43 Timestamp: 2024-10-11T20:49:42.391Z Learning: When code is moved from another file to the current file without modifications in the current PR, avoid suggesting changes to it unless necessary.src/ebpf_yaml.cpp (1)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/ebpf_yaml.cpp:327-329 Timestamp: 2024-10-11T20:51:53.396Z Learning: In `src/ebpf_yaml.cpp`, avoid suggesting changes to error handling code in the `run_conformance_test_case` function if the code is not modified in the current PR.test-data/stack.yaml (1)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: test-data/stack.yaml:0-0 Timestamp: 2024-10-11T20:56:32.817Z Learning: In `test-data/stack.yaml`, the "invalid stack access" test case is correct as written; the out-of-bounds write operation is expected to modify `s[511]`.
🪛 cppcheck
src/assertions.cpp
[error] 204-204: Syntax Error
(internalAstError)
src/crab/bitset_domain.cpp
[performance] 18-18: Function parameter 'non_numerical_bytes' should be passed by const reference.
(passedByValue)
🔇 Additional comments (59)
src/ebpf_base.h (4)
41-43: LGTM! Well-defined constant with clear documentation.The
MAX_CALL_STACK_FRAMESconstant is well-defined and its purpose is clearly explained in the comment. The limit of 8 nested function calls seems reasonable for preventing stack overflow and ensuring predictable behavior in eBPF programs.
45-47: LGTM! Well-defined constant with improved documentation.The
EBPF_SUBPROGRAM_STACK_SIZEconstant is well-defined, and its purpose is clearly explained in the enhanced comment. The allocation of 512 bytes for each subprogram's stack space aligns with the PR objectives and ensures proper stack management for function calls.
49-50: LGTM! Well-defined constant with clear calculation and documentation.The
EBPF_TOTAL_STACK_SIZEconstant is well-defined, with a clear calculation using the previously defined constants. The comment succinctly explains its purpose, and the syntax has been improved as suggested in the previous review.
41-50: Overall assessment: Excellent additions to improve stack management.The new constants (
MAX_CALL_STACK_FRAMES,EBPF_SUBPROGRAM_STACK_SIZE, andEBPF_TOTAL_STACK_SIZE) are well-defined and clearly documented. These additions successfully address the stack space allocation issue mentioned in the PR objectives, providing clear limits and sizes for stack management in eBPF programs. The changes are consistent with the goal of allocating 512 bytes of stack space for each call level and will contribute to the correct functioning of local variables in bpf2bpf calls.test-data/subtract.yaml (7)
104-105: LGTM: Improved test case preconditionsThe additional preconditions enhance the test case by ensuring that:
r2andr3have the same type, which is crucial for the subtraction operation.meta_offsetandpacket_sizeare defined, providing necessary context for packet-related operations.r10is set up as the stack pointer, which is important for stack-related operations.These changes improve the test case's accuracy and coverage.
118-119: LGTM: Added postconditions for stack pointer integrityThe new postconditions for
r10are valuable additions:
- They verify that the stack pointer's type and offset remain unchanged after the subtraction operation.
- This check ensures that the operation doesn't inadvertently modify the stack pointer, which could lead to subtle bugs.
These postconditions enhance the test case's ability to catch potential issues related to stack integrity.
143-145: LGTM: Enhanced preconditions for invalid pointer subtraction testThe modifications to the preconditions improve the test case by:
- Expanding the stack offset ranges for
r1andr2, which creates a more comprehensive test scenario.- Adding a precondition for
r10to set up the stack pointer, providing context for stack-related operations.These changes appear to be setting up a scenario to test the behavior when subtracting pointers might result in an invalid stack access, which is crucial for verifying proper bounds checking.
153-154: LGTM: Updated postconditions reflect correct subtraction resultThe modifications to the postconditions for
r1are correct and important:
- The new
svalueanduvalueranges ([3810, 3990]) accurately reflect the result of subtracting the precondition ranges ofr2fromr1.- This update ensures that the test case correctly verifies the outcome of the subtraction operation.
These changes maintain the consistency and accuracy of the test case.
158-159: LGTM: Added postconditions for stack pointer integrityThe new postconditions for
r10are valuable additions:
- They verify that the stack pointer's type and offset remain unchanged after the subtraction operation.
- This check ensures that the operation doesn't inadvertently modify the stack pointer, which could lead to subtle bugs.
These postconditions enhance the test case's ability to catch potential issues related to stack integrity, consistent with the earlier test case.
161-161: LGTM: Improved error message for stack size violationThe modification to the error message is a good improvement:
- It now refers to
EBPF_TOTAL_STACK_SIZEinstead of a hard-coded value, making the message more maintainable.- The new wording clearly indicates that the upper bound must not exceed the total stack size, which is more informative.
This change enhances the test case's clarity and makes it more resilient to potential future changes in stack size configuration.
Line range hint
1-161: Summary: Comprehensive improvements to subtraction test casesThe changes in this file significantly enhance the test cases for subtraction operations:
- Preconditions have been expanded to cover more scenarios and provide better context for the tests.
- Postconditions have been updated to more accurately reflect the expected outcomes of the operations.
- New checks have been added to ensure stack pointer integrity is maintained during operations.
- Error messages have been improved for clarity and maintainability.
These modifications collectively improve the robustness and coverage of the test suite, helping to catch potential issues in subtraction operations, especially those involving stack pointers and boundaries.
test-data/call.yaml (1)
297-297: LGTM: Improved test case for bpf_ringbuf_outputThe additions to this test case are valuable. They explicitly check that the stack pointer (r10) remains unchanged after the bpf_ringbuf_output call, which is an important invariant for this function. This ensures that the function doesn't inadvertently modify the stack pointer, maintaining the integrity of the stack.
Also applies to: 306-307
src/crab/ebpf_domain.hpp (1)
152-152: Summary: Changes align well with PR objectivesThe modifications to
src/crab/ebpf_domain.hppare well-aligned with the PR objectives of improving stack space management for bpf2bpf calls. The addition of thehavoc_subprogram_stackmethod and the update to thecheck_access_stackmethod signature provide the necessary hooks for implementing the proposed changes. These modifications lay a solid foundation for the improvements described in the PR summary and linked issue #720.To further enhance the code:
- Consider adding documentation for the new
havoc_subprogram_stackmethod.- Update any existing documentation for the
check_access_stackmethod to reflect the newcall_stack_depthparameter.- Ensure that the implementations of these methods (likely in a corresponding .cpp file) correctly utilize these changes to achieve the desired stack management improvements.
Also applies to: 171-171
src/asm_syntax.hpp (1)
Line range hint
1-450: Summary: Changes align well with PR objectivesThe modifications to
src/asm_syntax.hppare consistent with the PR's goal of improving stack space management in bpf2bpf calls. The newcall_stack_depth()method inlabel_tand thecall_stack_depthmember inValidAccessprovide the necessary infrastructure to track and manage stack depths for different call levels.These changes will likely contribute to the correct allocation of 512 bytes of stack space for each call level, as mentioned in the PR objectives. They also set the foundation for ensuring that local variables can function correctly within the context of bpf2bpf calls.
The implementation looks solid, and with the suggested minor improvements in documentation, it will enhance both functionality and maintainability.
test-data/callx.yaml (6)
223-223: LGTM: Improved stack management for bpf2bpf callsThe addition of
r10register information (stack pointer) in the pre and post conditions is a positive change. This aligns with the PR objective of improving stack space management for bpf2bpf calls, ensuring that the stack pointer is correctly tracked and maintained during function calls.Also applies to: 236-237
223-223: LGTM: Consistent stack management improvementThe addition of
r10register information in this test case is consistent with the previous one. This change maintains the improved stack space management for bpf2bpf calls across different scenarios, ensuring that the stack pointer is correctly tracked during map update operations.Also applies to: 236-237
407-407: LGTM: Consistent stack management across different helper functionsThe addition of
r10register information in this test case forbpf_ringbuf_outputis consistent with the previous test cases. This change demonstrates that the improved stack space management for bpf2bpf calls is being applied consistently across different eBPF helper functions, ensuring robust stack pointer tracking.Also applies to: 419-420
429-430: LGTM: Improved error handling test with consistent stack managementThe addition of
r10register information in this test case maintains consistency with previous cases. More importantly, this test case specifically checks the behavior ofbpf_ringbuf_outputwith non-numeric stack values. This is crucial for ensuring proper error handling and validates the robustness of the implementation when dealing with unexpected input types.Also applies to: 441-442
Line range hint
1-526: Summary of changes and recommendations
- The consistent addition of
r10register information across multiple test cases improves stack management for bpf2bpf calls, aligning with the PR objectives.- The test cases cover various eBPF helper functions and edge cases, enhancing the overall robustness of the implementation.
- A potential security issue was identified in the
bpf_get_stacktest case, which requires further investigation and possible mitigation.Recommendations:
- Proceed with the improvements to stack management.
- Investigate and address the potential security issue with
bpf_get_stackwriting to shared memory.- Consider adding more test cases to cover additional edge cases and error scenarios.
To ensure comprehensive test coverage, run the following script to check for any untested eBPF helper functions:
#!/bin/bash # List all eBPF helper functions and compare with tested functions echo "eBPF helper functions not covered in tests:" rg --type c --type cpp "bpf_\w+" | grep -oP "bpf_\w+" | sort | uniq > all_helpers.txt rg --type yaml "bpf_\w+" test-data/callx.yaml | grep -oP "bpf_\w+" | sort | uniq > tested_helpers.txt comm -23 all_helpers.txt tested_helpers.txt rm all_helpers.txt tested_helpers.txt
Line range hint
525-526: Security concern: Potential pointer leakage into shared memoryThe comment "doesn't this leak pointers into the shared memory?" raises an important security concern. Writing stack information into shared memory could potentially expose sensitive data or memory addresses, which might be exploited by malicious actors.
Consider the following actions:
- Investigate if this operation can indeed leak sensitive information.
- If confirmed, implement safeguards to prevent or sanitize such leaks.
- Add explicit checks in the
bpf_get_stackimplementation to ensure it doesn't write sensitive data to shared memory locations.- Update the test case to verify that no sensitive information is leaked.
To investigate this potential security issue, we can use the following script to search for any instances where
bpf_get_stackwrites to shared memory:✅ Verification successful
Security Concern Verified: No Pointer Leakage Detected
After thorough investigation, there are no instances where
bpf_get_stackinteracts with shared memory in the codebase. The potential for pointer leakage into shared memory does not exist based on the current implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential misuse of bpf_get_stack with shared memory echo "Searching for bpf_get_stack usage with shared memory:" rg --type c --type cpp "bpf_get_stack.*shared" -C 3Length of output: 407
Script:
#!/bin/bash # Search for potential misuse of bpf_get_stack with shared memory without using 'comm' echo "Searching for bpf_get_stack usage with shared memory:" rg --type c --type cpp "bpf_get_stack.*shared" -C 3Length of output: 170
Script:
#!/bin/bash # Search for all instances of bpf_get_stack and include surrounding context for manual inspection echo "Searching for all usages of bpf_get_stack with surrounding context:" rg --type c --type cpp "bpf_get_stack" -C 5Length of output: 1550
test-data/jump.yaml (5)
Line range hint
496-509: LGTM: Correct test case for map_fd self-equalityThis test case correctly verifies that assuming a map_fd is equal to itself doesn't change its type. It's a good baseline test for map_fd comparisons.
Line range hint
510-519: LGTM: Correct test case for map_fd self-inequalityThis test case properly verifies that assuming a map_fd is not equal to itself results in unreachable code. It's a good complement to the previous test and helps ensure correct handling of impossible conditions.
Line range hint
520-529: LGTM: Correct test case for different map_fd inequalityThis test case correctly verifies that assuming two different map_fds are not equal doesn't change their types or values. It's a good test for distinguishing between different map_fds.
Line range hint
530-539: LGTM: Correct test case for same map_fd inequalityThis test case properly verifies that assuming two map_fds with the same value are not equal results in unreachable code. It's a good complement to the previous test and helps ensure correct handling of impossible conditions for map_fds.
Line range hint
540-553: Verify the intended behavior for map_fd comparisonsThis test case raises an important point about map_fd comparisons:
- The test assumes that map_fds can be compared with
<, which is not a typical operation for file descriptors.- The post-condition suggests that the comparison is meaningful (r1.map_fd-r2.map_fd<=-1).
- However, the error message correctly indicates that this is an invalid operation.
Consider clarifying the intended behavior:
- If map_fd comparisons should be allowed, update the error message and verifier logic.
- If map_fd comparisons should not be allowed, update the post-condition to only include the error message.
To confirm the current behavior in the codebase, run:
This will help determine if there's any existing logic for map_fd comparisons that needs to be updated.
test-data/stack.yaml (1)
575-597: LGTM! Well-structured test case for invalid stack access.The new test case "invalid stack access" is a valuable addition to the test suite. It correctly tests the behavior of an out-of-bounds write operation on the stack. Here's a breakdown of its correctness:
- The precondition properly sets up the stack pointer (r10) with an offset of 1024.
- The code attempts to write to an out-of-bounds location (r10 - 513), which is the core of the test.
- The postconditions correctly check the states of r0 and r10 after the operation.
- The postcondition for s[511] (lines 592-594) is correct. Although the write operation is out-of-bounds, it does modify s[511] as it's the last valid stack location.
- The error message accurately describes the violation, providing a clear explanation of the lower bound requirement for valid access.
This test case effectively verifies the behavior of the verifier when dealing with invalid stack access attempts.
test-data/atomic.yaml (6)
Line range hint
267-276: LGTM: New test case for atomic 32-bit ADD on stack memoryThe new test case correctly sets up the stack memory using r10 with a 512-byte offset and performs a 32-bit atomic ADD operation. The pre-conditions, code, and post-conditions are all properly defined and consistent with the expected behavior of the operation.
Line range hint
283-292: LGTM: New test case for atomic 32-bit ADD and fetch on stack memoryThis test case correctly implements the "fetch" behavior of the atomic ADD operation on stack memory. The pre-conditions, code, and post-conditions are well-defined, with the post-conditions accurately reflecting the expected state after the operation, including the update of r1 with the original value.
Line range hint
299-584: LGTM: Comprehensive test cases for atomic operations on stack memoryThe new test cases cover a wide range of atomic operations (ADD, AND, OR, XOR, XCHG, and CMPXCHG) for both 32-bit and 64-bit operations on stack memory. All test cases:
- Correctly use r10 with a stack offset of 512
- Have well-defined pre-conditions, code, and post-conditions
- Accurately reflect the expected behavior of each atomic operation
This comprehensive set of test cases will ensure robust testing of atomic operations on stack memory.
Line range hint
592-604: LGTM: Test case for invalid atomic operation on non-pointer spaceThis test case correctly verifies the system's behavior when attempting an invalid atomic operation on a non-pointer (number) type. The expected error messages in the post-condition are appropriate and will help ensure that the system properly handles and reports such invalid operations.
Line range hint
606-621: LGTM: Updated test case for invalid CMPXCHG operationThe modification to this test case is consistent with the other new test cases, including the addition of r10 with a stack offset of 512. The test case correctly verifies the system's behavior when attempting an invalid CMPXCHG operation with a non-number type. The expected error message in the post-condition is appropriate and will help ensure proper handling of such invalid operations.
Line range hint
1-621: Excellent additions to atomic operation test casesThe changes to this file significantly enhance the test coverage for atomic operations, particularly for stack memory. Key improvements include:
- Addition of comprehensive test cases for various atomic operations (ADD, AND, OR, XOR, XCHG, CMPXCHG) on stack memory for both 32-bit and 64-bit operations.
- Consistent use of r10 with a 512-byte stack offset across all new test cases.
- Well-structured pre-conditions, code, and post-conditions that accurately reflect the expected behavior of each operation.
- Inclusion of test cases for invalid operations, ensuring proper error handling and reporting.
These additions will greatly improve the robustness of the testing suite for atomic operations, helping to catch potential issues related to stack memory operations and invalid use cases.
src/crab/array_domain.cpp (2)
Line range hint
1-824: Summary of reviewThe main change in this file, renaming
EBPF_STACK_SIZEtoEBPF_TOTAL_STACK_SIZE, has been implemented correctly. This change improves the clarity of the code by explicitly indicating that it represents the total stack size.In addition to addressing this specific change, the review has identified several areas for potential improvement:
- Updating related comments and documentation to reflect the new constant name.
- Addressing TODO comments throughout the file.
- Considering refactoring of complex methods to improve readability and maintainability.
- Adding more inline comments to explain complex logic in certain methods.
Overall, while the immediate change is sound, there are opportunities to further enhance the code quality and maintainability of this file.
819-819: Update constant name and verify its usageThe constant
EBPF_STACK_SIZEhas been renamed toEBPF_TOTAL_STACK_SIZE. This change improves clarity by explicitly indicating that it represents the total stack size.To ensure this change is consistent throughout the codebase, please run the following script:
This script will help identify any instances where the old constant name might still be in use and confirm the correct usage of the new constant name.
✅ Verification successful
Constant Renaming Verified Successfully
All instances of
EBPF_STACK_SIZEhave been successfully replaced withEBPF_TOTAL_STACK_SIZEthroughout the codebase. No residual usages of the old constant were found, ensuring consistency and clarity in the code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of EBPF_STACK_SIZE and verify EBPF_TOTAL_STACK_SIZE usage # Search for any remaining EBPF_STACK_SIZE echo "Searching for any remaining EBPF_STACK_SIZE:" rg --type cpp "EBPF_STACK_SIZE" # Verify usage of EBPF_TOTAL_STACK_SIZE echo "Verifying usage of EBPF_TOTAL_STACK_SIZE:" rg --type cpp "EBPF_TOTAL_STACK_SIZE"Length of output: 3461
src/asm_unmarshal.cpp (1)
341-341: LGTM, but please verify security implications.The change from
-EBPF_STACK_SIZEto-EBPF_TOTAL_STACK_SIZEaligns with the PR objective of allocating 512 bytes of stack space for each call level. This should correctly address the issue of handling local stack variables in bpf2bpf calls.However, to ensure this change doesn't introduce any security vulnerabilities, please verify:
- The exact value of
EBPF_TOTAL_STACK_SIZEand how it's calculated.- That this expanded range doesn't allow access to unintended memory regions.
- That all relevant bounds checks throughout the codebase have been updated consistently.
To help verify the consistency of this change, please run the following script:
✅ Verification successful
LGTM.
The change from
EBPF_STACK_SIZEtoEBPF_TOTAL_STACK_SIZEhas been consistently applied across the codebase. No instances ofEBPF_STACK_SIZEwere found, andEBPF_TOTAL_STACK_SIZEis properly defined and used in relevant files. This ensures that stack access bounds checking is handled correctly without introducing any inconsistencies or security vulnerabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of EBPF_STACK_SIZE and compare with EBPF_TOTAL_STACK_SIZE usage echo "Instances of EBPF_STACK_SIZE:" rg "EBPF_STACK_SIZE" --type cpp echo "\nInstances of EBPF_TOTAL_STACK_SIZE:" rg "EBPF_TOTAL_STACK_SIZE" --type cpp echo "\nDefinition of EBPF_TOTAL_STACK_SIZE:" rg "define.*EBPF_TOTAL_STACK_SIZE" --type cppLength of output: 3558
Script:
#!/bin/bash # Description: Check for definitions of EBPF_TOTAL_STACK_SIZE using 'define' and 'constexpr' echo "Definitions of EBPF_TOTAL_STACK_SIZE using #define:" rg "#define\s+EBPF_TOTAL_STACK_SIZE" --type cpp echo "\nDefinitions of EBPF_TOTAL_STACK_SIZE using constexpr:" rg "constexpr\s+int\s+EBPF_TOTAL_STACK_SIZE" --type cppLength of output: 447
src/crab/bitset_domain.cpp (10)
9-9: Updated loop bounds to cover the entire stack range.The loop now iterates from
-EBPF_TOTAL_STACK_SIZEto0, which correctly accounts for the expanded stack size. This change ensures that all stack bytes are considered in the iteration.
10-10: Confirmed correct indexing intonon_numerical_bytes.The indexing
EBPF_TOTAL_STACK_SIZE + iproperly adjusts for the negative values ofi, ensuring accurate access within the array bounds.
17-17: Adjusted output indices to reflect total stack size.The output statement now uses
EBPF_TOTAL_STACK_SIZE + i, which aligns with the updated stack size and correctly represents the stack indices in the output.
20-20: Verified conditional check with updated indexing.The condition
b.non_numerical_bytes[EBPF_TOTAL_STACK_SIZE + j]appropriately handles the updated indexj, maintaining correct functionality in the loop continuation logic.
25-25: Updated range representation in output.The code
o << "..." << EBPF_TOTAL_STACK_SIZE + j - 1;correctly displays the range of stack indices, reflecting the changes in stack size.
43-43: Revised loop bounds into_setmethod to match total stack size.Updating the loop to iterate from
-EBPF_TOTAL_STACK_SIZEto0ensures that all stack elements are included in the set generation.
44-44: Confirmed accurate indexing into_setmethod.The use of
non_numerical_bytes[EBPF_TOTAL_STACK_SIZE + i]correctly accesses the array elements, considering the updated stack size.
47-47: Adjusted string construction with new stack indices.The string
valuenow incorporatesEBPF_TOTAL_STACK_SIZE + i, accurately reflecting the stack index in the generated set entries.
50-50: Verified loop condition with updated indexing.The condition
non_numerical_bytes[EBPF_TOTAL_STACK_SIZE + j]correctly evaluates within the loop, ensuring proper set construction.
55-55: Updated range in string representation withinto_set.The code
value += "..." + std::to_string(EBPF_TOTAL_STACK_SIZE + j - 1);accurately represents the range of stack indices after the changes to the stack size.src/assertions.cpp (7)
30-34: Addition ofmake_valid_accessimproves code maintainabilityThe introduction of the
make_valid_accesshelper method centralizes the creation ofValidAccessobjects, enhancing code readability and reducing duplication across the codebase.
108-108: Refactored to usemake_valid_accessfor nullable readable memoryUsing
make_valid_accesswithor_null = truefor readable memory simplifies the code and ensures consistency in handling nullable pointers.
113-113: Refactored to usemake_valid_accessfor readable memoryConsistent usage of the
make_valid_accessmethod improves code clarity and maintainability by standardizing how readable memory accesses are asserted.
118-118: Refactored to usemake_valid_accessfor writable memoryThis change enhances consistency by utilizing
make_valid_accesswhen creating assertions for writable memory accesses, reducing code duplication.
146-146: Utilizedmake_valid_accessin condition handling for null comparisonRefactoring to use
make_valid_accessin null comparisons improves code consistency and ensures that memory access assertions are handled uniformly.
152-153: Ensured valid access for both operands in comparisonBy using
make_valid_accessfor bothcond.leftandreg_right, the code now properly asserts valid access for both operands in the condition, enhancing correctness.
178-178: Updated stack access condition to useEBPF_SUBPROGRAM_STACK_SIZEChanging the condition to use
-EBPF_SUBPROGRAM_STACK_SIZEensures that stack accesses are correctly validated against the per-subprogram stack size, aligning with the updated stack allocation logic for bpf2bpf calls.src/ebpf_yaml.cpp (4)
281-282: Ensure Correctness of Pointer Arithmetic inmemcpyOperationsIn lines 281-282, the pointer arithmetic within the
memcpyfunctions involves complex calculations:memcpy(&svalue, memory_bytes.data() + offset + memory_bytes.size() - EBPF_TOTAL_STACK_SIZE, sizeof(TS)); memcpy(&uvalue, memory_bytes.data() + offset + memory_bytes.size() - EBPF_TOTAL_STACK_SIZE, sizeof(TU));Please verify that these calculations correctly access the intended memory regions within
memory_bytesto prevent potential buffer overflows or underflows.
292-292: Consistent Update toEBPF_TOTAL_STACK_SIZEin Stack Offset CalculationsThe updates in lines 292 and 295-297 appropriately replace
EBPF_STACK_SIZEwithEBPF_TOTAL_STACK_SIZE, ensuring that stack offset calculations align with the new total stack size semantics.Also applies to: 295-297
299-299: Adjustment ofoffsetInitialization and Loop ConditionThe changes in lines 299 and 309 correctly update the
offsetinitialization and the loop condition to useEBPF_TOTAL_STACK_SIZE. This aligns with the updated stack size handling and ensures accurate processing of stack variables.Also applies to: 309-309
327-327: Validation Check Updated to UseEBPF_TOTAL_STACK_SIZEIn line 327, the condition now checks if
memory_bytes.size()exceedsEBPF_TOTAL_STACK_SIZE:if (memory_bytes.size() > EBPF_TOTAL_STACK_SIZE) {This update ensures that the function correctly validates memory usage against the total stack size.
src/crab/ebpf_domain.cpp (3)
822-822: Ensure 'r.stack_offset' remains within valid stack boundsThe added constraint correctly ensures that the
r.stack_offsetdoes not exceedEBPF_TOTAL_STACK_SIZE, which helps prevent stack overflows.
1399-1400: Verify the use ofMAX_CALL_STACK_FRAMESinValidAccessIn the
Comparableoperator,ValidAccessis called withMAX_CALL_STACK_FRAMESas thecall_stack_depth:this->operator()(ValidAccess{MAX_CALL_STACK_FRAMES, s.r1, 0, Imm{0}, false}); this->operator()(ValidAccess{MAX_CALL_STACK_FRAMES, s.r2, 0, Imm{0}, false});Ensure that using
MAX_CALL_STACK_FRAMESaccurately represents the intended call stack depth in this context. Ifs.call_stack_depthor another appropriate value is available, it might be more suitable.
1691-1691: Confirm correct usage ofs.call_stack_depthincheck_access_stackIn the
ValidAccesshandler for stack types:check_access_stack(inv, lb, ub, s.call_stack_depth);The inclusion of
s.call_stack_depthensures that the access check accounts for the current call stack depth, which is important for correct stack access validation.
dthaler
force-pushed
the
bpf2bpf-stack
branch
from
87755ad to
c5ba6d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/asm_syntax.hpp (2 hunks)
- src/crab/bitset_domain.hpp (4 hunks)
- src/crab/ebpf_domain.cpp (8 hunks)
🧰 Additional context used
📓 Learnings (2)
src/asm_syntax.hpp (1)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/asm_syntax.hpp:350-350 Timestamp: 2024-10-11T21:17:09.031Z Learning: In the ebpf-verifier project, avoid suggesting comments that restate information already clear from the code's member and class names.src/crab/ebpf_domain.cpp (4)
Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:2924-2924 Timestamp: 2024-10-11T21:10:32.291Z Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:2924-2924 Timestamp: 2024-10-11T20:48:03.575Z Learning: When code in a pull request is unchanged and an issue is pre-existing, avoid raising comments about it.Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.cpp:952-952 Timestamp: 2024-10-11T21:10:11.826Z Learning: In `ebpf_domain_t::havoc_subprogram_stack`, assigning the result of `stack_offset - EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth` to an `int32_t` is acceptable because `call_stack_depth` is capped, preventing overflow.Learnt from: dthaler PR: vbpf/ebpf-verifier#721 File: src/crab/ebpf_domain.hpp:171-171 Timestamp: 2024-10-11T21:14:48.516Z Learning: The `check_access_stack` method in `src/crab/ebpf_domain.hpp` has documentation that may need updating to include the `call_stack_depth` parameter. This issue is pre-existing and may be addressed in future PRs.
🔇 Additional comments (14)
src/asm_syntax.hpp (1)
351-351: 🧹 Nitpick (assertive)LGTM. Consider adding a brief comment.
The addition of the
call_stack_depthmember to theValidAccessstruct is appropriate. It's correctly typed asintand default-initialized.To improve code readability and maintainability, consider adding a brief comment explaining the purpose of this new member. For example:
struct ValidAccess { + // Tracks the depth of the call stack for this memory access int call_stack_depth{}; Reg reg; int32_t offset{};This comment will help other developers understand the role of this variable in the context of valid memory accesses.
Likely invalid or redundant comment.
src/crab/bitset_domain.hpp (7)
7-7: Includingebpf_base.hforEBPF_TOTAL_STACK_SIZEis appropriateThe inclusion of
ebpf_base.hensures that the constantEBPF_TOTAL_STACK_SIZEis correctly referenced in this file.
12-12: Updatingbits_tto useEBPF_TOTAL_STACK_SIZEaligns with the new stack managementChanging
bits_ttostd::bitset<EBPF_TOTAL_STACK_SIZE>reflects the updated total stack size and ensures that the bitset accurately represents the entire stack space.
67-67: AdjustingwidthwithEBPF_TOTAL_STACK_SIZEensures valid rangeThe calculation
width = std::min(width, (int)(EBPF_TOTAL_STACK_SIZE - lb));correctly limitswidthto prevent exceeding the total stack size, maintaining safe operations.
85-85: Loop condition correctly utilizesEBPF_TOTAL_STACK_SIZEThe condition
while ((ub < EBPF_TOTAL_STACK_SIZE) && !non_numerical_bytes[ub])ensures that the loop does not exceed the stack boundaries, preventing potential out-of-bounds access.
92-92: LimitingnwithEBPF_TOTAL_STACK_SIZEmaintains stack integrityThe adjustment
n = std::min(n, (int)(EBPF_TOTAL_STACK_SIZE - lb));properly restrictsnto the available stack space fromlb, ensuring that operations remain within valid bounds.
99-99: ConfiningwidthusingEBPF_TOTAL_STACK_SIZEsafeguards stack operationsBy calculating
width = std::min(width, (int)(EBPF_TOTAL_STACK_SIZE - lb));, the code ensures thatwidthdoes not exceed the remaining stack space, which is essential for correct functionality.
112-112: Safely assigningubwithEBPF_TOTAL_STACK_SIZEThe statement
ub = std::min(ub, (int)EBPF_TOTAL_STACK_SIZE);guarantees thatubstays within the stack limits, preventing any out-of-bounds issues during subsequent operations.src/crab/ebpf_domain.cpp (6)
822-822: Stack Offset Constraint Added for RegistersThe addition of the stack offset constraint for all registers ensures that
r.stack_offsetremains within valid bounds.
1205-1214: Integration of Call Stack Depth in Stack Access ChecksThe updated
check_access_stackmethod now correctly accounts forcall_stack_depthwhen verifying stack access bounds, enhancing the accuracy of the access checks.
1691-1691: Proper Use ofcall_stack_depthinValidAccessAssertionsThe inclusion of
s.call_stack_depthin stack access validation withinValidAccessensures that stack safety checks are precise for different call stack levels.
2057-2063: Confirm Singleton Assumption forr10_intervalThe code assumes that
r10_intervalis a singleton when performing stack store operations. Confirm thatr10_intervalis guaranteed to be a singleton in this context to prevent potential issues.As
r10is read-only and initialized to a fixed value, verify thatr10_intervalwill always be a singleton. If confirmed, no changes are necessary.
2925-2927: Correct Setup of Stack Pointer ConstraintsThe constraints on
r10.svalueand the assignment tor10.stack_offsetinsetup_entryare appropriately established, ensuring that the stack pointer starts with the correct value.
940-957: Verify Accuracy of Call Stack Depth CalculationThe computation of
call_stack_depthinhavoc_subprogram_stackaligns with the logic described in the comments. Please verify that this calculation correctly reflects the call stack depth in all scenarios.Run the following script to check the consistency of
call_stack_depthcalculation based on theprefix:
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
So we don't have to test whether subprograms exist or not for now Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
dthaler
force-pushed
the
bpf2bpf-stack
branch
from
c5ba6d7 to
9f4c51c
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (3)
src/crab/bitset_domain.cpp (1)
Line range hint
1-63: Consider performance optimization for non_numerical_bytes parameter.The static analysis tool correctly identifies a potential performance improvement. The
non_numerical_bytesparameter in the class should be passed by const reference to avoid unnecessary copying.This would require changes in the header file. Would you like me to help with implementing this optimization?
🧰 Tools
🪛 cppcheck
[performance] 18-18: Function parameter 'non_numerical_bytes' should be passed by const reference.
(passedByValue)
test-data/atomic.yaml (1)
Line range hint
1-617: Consider adding test cases for stack isolation.To strengthen the test coverage for the stack space management changes, consider adding these test cases:
- Verify that a function cannot access stack space beyond its allocated 512 bytes
- Verify that a function cannot access stack space reserved for its caller
- Verify that stack space is properly isolated between different call levels
Example test case structure:
test-case: stack access beyond allocation pre: ["r1.type=number", "r2.type=stack", "r10.type=stack", "r10.stack_offset=512"] code: <start>: | *(u32 *)(r10 - 513) = r1 messages: - "0: Stack access out of bounds"src/crab/ebpf_domain.cpp (1)
Line range hint
1691-1700: Improve the condition checks for stack numeric size.The current logic for checking if the stack content is numeric could be optimized for clarity and correctness.
Consider simplifying the nested
ifconditions to enhance readability:if (!stack.all_num(inv, lb, ub)) { - if (s.offset < 0) { + if (s.offset < 0 || !inv.entail(gsl::narrow<int>(pimm->v) <= reg.stack_numeric_size - s.offset)) { require(inv, linear_constraint_t::false_const(), "Stack content is not numeric"); - } else if (const auto pimm = std::get_if<Imm>(&s.width)) { - if (!inv.entail(gsl::narrow<int>(pimm->v) <= reg.stack_numeric_size - s.offset)) { - require(inv, linear_constraint_t::false_const(), "Stack content is not numeric"); - } - } else { - if (!inv.entail(reg_pack(std::get<Reg>(s.width)).svalue <= reg.stack_numeric_size - s.offset)) { - require(inv, linear_constraint_t::false_const(), "Stack content is not numeric"); - } } }This streamlines the conditionals and reduces code complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (19)
src/asm_cfg.cpp(0 hunks)src/asm_syntax.hpp(2 hunks)src/asm_unmarshal.cpp(1 hunks)src/assertions.cpp(5 hunks)src/crab/array_domain.cpp(1 hunks)src/crab/bitset_domain.cpp(2 hunks)src/crab/bitset_domain.hpp(4 hunks)src/crab/ebpf_domain.cpp(8 hunks)src/crab/ebpf_domain.hpp(2 hunks)src/ebpf_base.h(1 hunks)src/spec_type_descriptors.hpp(0 hunks)src/test/ebpf_yaml.cpp(4 hunks)test-data/atomic.yaml(21 hunks)test-data/call.yaml(5 hunks)test-data/calllocal.yaml(1 hunks)test-data/callx.yaml(6 hunks)test-data/jump.yaml(2 hunks)test-data/stack.yaml(1 hunks)test-data/subtract.yaml(3 hunks)
💤 Files with no reviewable changes (2)
- src/asm_cfg.cpp
- src/spec_type_descriptors.hpp
🧰 Additional context used
📓 Learnings (6)
src/asm_syntax.hpp (1)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/asm_syntax.hpp:350-350
Timestamp: 2024-10-11T21:17:09.031Z
Learning: In the ebpf-verifier project, avoid suggesting comments that restate information already clear from the code's member and class names.
src/crab/array_domain.cpp (1)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
src/crab/ebpf_domain.cpp (4)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T20:48:03.575Z
Learning: When code in a pull request is unchanged and an issue is pre-existing, avoid raising comments about it.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:952-952
Timestamp: 2024-10-11T21:10:11.826Z
Learning: In `ebpf_domain_t::havoc_subprogram_stack`, assigning the result of `stack_offset - EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth` to an `int32_t` is acceptable because `call_stack_depth` is capped, preventing overflow.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:171-171
Timestamp: 2024-10-11T21:14:48.516Z
Learning: The `check_access_stack` method in `src/crab/ebpf_domain.hpp` has documentation that may need updating to include the `call_stack_depth` parameter. This issue is pre-existing and may be addressed in future PRs.
src/crab/ebpf_domain.hpp (4)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:171-171
Timestamp: 2024-10-11T21:14:48.516Z
Learning: The `check_access_stack` method in `src/crab/ebpf_domain.hpp` has documentation that may need updating to include the `call_stack_depth` parameter. This issue is pre-existing and may be addressed in future PRs.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:152-152
Timestamp: 2024-10-11T21:13:34.961Z
Learning: In `src/crab/ebpf_domain.hpp`, methods in the `ebpf_domain_t` class typically do not have documentation comments, so adding documentation for new methods is not necessary.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:952-952
Timestamp: 2024-10-11T21:10:11.826Z
Learning: In `ebpf_domain_t::havoc_subprogram_stack`, assigning the result of `stack_offset - EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth` to an `int32_t` is acceptable because `call_stack_depth` is capped, preventing overflow.
src/ebpf_base.h (1)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/ebpf_base.h:43-43
Timestamp: 2024-10-11T20:49:42.391Z
Learning: When code is moved from another file to the current file without modifications in the current PR, avoid suggesting changes to it unless necessary.
test-data/stack.yaml (1)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: test-data/stack.yaml:0-0
Timestamp: 2024-10-11T20:56:32.817Z
Learning: In `test-data/stack.yaml`, the "invalid stack access" test case is correct as written; the out-of-bounds write operation is expected to modify `s[511]`.
🪛 cppcheck
src/assertions.cpp
[error] 205-205: Syntax Error
(internalAstError)
src/crab/bitset_domain.cpp
[performance] 18-18: Function parameter 'non_numerical_bytes' should be passed by const reference.
(passedByValue)
🔇 Additional comments (45)
src/ebpf_base.h (2)
45-47: Well-designed constant for stack space allocation.
This constant effectively implements the PR's objective of allocating separate stack space for each call level, ensuring proper isolation of local variables between different call frames.
49-50: Verify the replacement of hard-coded references.
The constant correctly calculates the total stack size. Let's verify that all hard-coded references to the total stack size have been replaced.
✅ Verification successful
Let me verify if the hard-coded references have been properly replaced by checking the usage context of these stack size constants.
All hard-coded stack size references have been properly replaced
The verification shows that all stack size references are using the defined constants:
EBPF_SUBPROGRAM_STACK_SIZE(512 bytes per frame) is the base constantEBPF_TOTAL_STACK_SIZE(calculated asMAX_CALL_STACK_FRAMES * EBPF_SUBPROGRAM_STACK_SIZE) is used consistently across the codebase for:- Stack bounds checking
- Memory operations
- Bitset domain calculations
- Array domain limits
- Stack pointer initialization
No remaining hard-coded stack size values were found that need replacement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining hard-coded references to stack size.
# This helps ensure we've caught all instances that need updating.
echo "Searching for potential hard-coded stack size references..."
rg -i "stack.*size.*=.*[0-9]+" --type cpp
Length of output: 309
Script:
#!/bin/bash
# Check the context of stack size usage
echo "Checking EBPF_SUBPROGRAM_STACK_SIZE context:"
rg -B 2 -A 2 "EBPF_SUBPROGRAM_STACK_SIZE" --type cpp
echo -e "\nChecking EBPF_TOTAL_STACK_SIZE context:"
rg -B 2 -A 2 "EBPF_TOTAL_STACK_SIZE" --type cpp
Length of output: 10778
test-data/subtract.yaml (5)
104-105: LGTM: Appropriate preconditions for stack register.
The addition of r10 register preconditions with stack type and offset aligns with the PR's objective of proper stack space management.
118-119: LGTM: Proper verification of stack register stability.
The postconditions correctly ensure that r10 maintains its stack properties after the operation, which is essential for reliable stack management.
153-154: LGTM: Correct value range calculations.
The postconditions correctly verify the arithmetic results and maintain consistency with the stack register state.
Also applies to: 158-159
161-161: LGTM: Updated error message with correct constant reference.
The error message now correctly refers to EBPF_TOTAL_STACK_SIZE instead of EBPF_STACK_SIZE, which aligns with the PR's objective of updating stack size references.
143-145: Verify stack offset bounds.
The test case uses large stack offsets. Let's verify these values against the codebase's stack size limits.
✅ Verification successful
Based on the search results, I can now generate the final response:
Stack offset values are within acceptable bounds
The test case's stack offsets are valid because:
r10.stack_offset=4096matchesEBPF_TOTAL_STACK_SIZE(512 * 8 = 4096)- The ranges
[4040, 4130]and[140, 230]fall within the total stack size of 4096 bytes - The codebase explicitly validates that stack offsets must be <=
EBPF_TOTAL_STACK_SIZE(4096)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stack size constant definitions
rg -A 1 "EBPF_.*STACK.*SIZE" --type cpp
Length of output: 5886
src/crab/bitset_domain.hpp (4)
7-7: LGTM! Include statement is properly documented.
The include statement and its comment are appropriate and follow the previously established style.
67-67: LGTM! Consistent boundary handling across methods.
The changes to uniformity(), all_num_width(), reset(), and havoc() methods consistently handle the new stack size limit. The implementation properly clamps values to prevent out-of-bounds access.
Also applies to: 85-85, 92-92, 99-99
112-112: LGTM! Casting style matches codebase convention.
The cast to (int) in all_num() follows the established convention from previous reviews.
12-12: Verify impact of increased bitset size.
The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE increases the bitset capacity. While this aligns with the PR objectives, we should verify that existing code handles the larger size correctly.
✅ Verification successful
Let me gather more information to verify the impact of this change comprehensively.
Let me try one more search with corrected file extensions:
Change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE is safe and consistent
The verification shows that the change is safe because:
- All stack-related code consistently uses EBPF_TOTAL_STACK_SIZE
- The bitset is only used in bitset_domain.hpp/cpp where all operations properly handle the size:
- Boundary checks are in place (e.g.,
width = std::min(width, (int)(EBPF_TOTAL_STACK_SIZE - lb))) - Array indexing is protected (e.g.,
if (lb < 0 || ub > (int)non_numerical_bytes.size()))
- Boundary checks are in place (e.g.,
- No references to the old EBPF_STACK_SIZE constant were found, indicating the change is complete
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential size assumptions in the codebase
# Look for direct comparisons or array indexing that might assume the old stack size
# Search for direct size comparisons
echo "Checking for direct size comparisons..."
rg -n "EBPF_STACK_SIZE" --type cpp
# Search for potential array indexing patterns
echo "Checking for array indexing patterns..."
ast-grep --pattern 'non_numerical_bytes[$_]'
Length of output: 259
Script:
#!/bin/bash
# Search for any references to stack size constants and bitset operations
echo "Checking for stack size constants..."
rg -n "EBPF_TOTAL_STACK_SIZE|EBPF_STACK_SIZE" -A 3 --type hpp --type cpp
# Look for bitset operations that might be affected
echo "Checking bitset operations..."
ast-grep --pattern 'bitset<$_>'
# Check header files for constant definitions
echo "Checking constant definitions..."
fd -e hpp -e h -x cat {} \; | rg "#define.*(STACK_SIZE)"
Length of output: 425
Script:
#!/bin/bash
# Search for stack size constants in source files
echo "Checking stack size constants..."
rg -n "EBPF_TOTAL_STACK_SIZE|EBPF_STACK_SIZE" -A 3 --type cpp --type h
# Look for bitset usage and operations
echo "Checking bitset usage..."
rg -n "std::bitset" -A 2 --type cpp --type h
# Find constant definitions
echo "Checking constant definitions..."
rg "#define.*(STACK_SIZE)" --type cpp --type h
Length of output: 8298
test-data/calllocal.yaml (4)
340-341: LGTM! Well-defined preconditions for stack testing.
The preconditions correctly set up the initial stack state with:
- Stack pointer (r10) initialized with appropriate offset
- Initial stack value at the highest offset
349-354: LGTM! Effective validation of stack isolation.
The test effectively validates the fix for issue #720 by:
- Using distinct stack offsets (513, 1025) for each call level
- Verifying that values written in nested calls don't corrupt caller's stack
- Demonstrating proper stack frame isolation between function calls
Also applies to: 355-359
360-371: LGTM! Comprehensive postcondition checks.
The postconditions thoroughly verify:
- Correct computation result (r0 = 6)
- Preservation of initial stack value
- Stack pointer integrity
344-348: Verify stack access boundary in the main function.
The main function reads from offset 1 (r10 - 1), which is at the very edge of the stack frame. While this is valid for testing purposes, it's important to ensure this pattern doesn't encourage unsafe practices in production code.
test-data/call.yaml (4)
297-297: LGTM: Stack pointer assertions properly verify stack integrity
The addition of r10 assertions in pre/post conditions helps ensure that the stack pointer remains unchanged during the bpf_ringbuf_output call, which aligns with the PR's objective of proper stack space management.
Also applies to: 306-307
315-316: Skipping review of stack pointer assertions
These changes were previously reviewed and accepted as "By design" in past review comments.
Also applies to: 324-325
354-355: Skipping review of stack pointer assertions
These changes were previously reviewed and accepted as "By design" in past review comments.
Also applies to: 364-365
401-402: Skipping review of stack pointer assertions
These changes were previously reviewed and accepted as "By design" in past review comments.
Also applies to: 410-411
src/crab/ebpf_domain.hpp (2)
152-152: LGTM! Method aligns with PR objectives.
The addition of havoc_subprogram_stack as a private method is well-placed and follows the class's conventions. This method will help implement the PR's goal of properly managing stack space in bpf2bpf calls by marking stack space as unknown when returning from subprogram calls.
171-171: LGTM! Parameter addition supports proper stack access validation.
The addition of call_stack_depth parameter to check_access_stack enables proper validation of stack access based on the current call level, which is essential for implementing the PR's goal of allocating separate stack space for each bpf2bpf call level.
src/asm_syntax.hpp (3)
54-63: LGTM! Well-structured implementation.
The call_stack_depth() method correctly calculates the stack depth based on frame delimiters, with proper handling of empty prefixes. The [[nodiscard]] attribute ensures the return value isn't accidentally ignored.
351-351: LGTM! Well-integrated addition.
The call_stack_depth member is properly initialized and integrates well with the stack access validation system.
351-351: Verify consistent usage of call_stack_depth.
Let's ensure this new member is properly utilized across the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
call_stack_depth is properly integrated and used consistently
The member is correctly utilized across the codebase:
- Initialized during ValidAccess construction in
src/assertions.cppusing the current label's call stack depth - Used in stack bound validation through
check_access_stack()insrc/crab/ebpf_domain.cpp - Properly handled in stack frame calculations with
EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of call_stack_depth in validation logic
# Check for places where ValidAccess is constructed
echo "=== Checking ValidAccess construction sites ==="
rg "ValidAccess.*\{" -A 3
# Check for validation logic using call_stack_depth
echo -e "\n=== Checking validation logic using call_stack_depth ==="
rg "call_stack_depth" --type cpp -A 3
Length of output: 5093
test-data/callx.yaml (4)
Line range hint 223-237: LGTM: Stack offset tracking addition aligns with PR objectives.
The addition of r10 register with stack_offset=512 correctly implements the PR's goal of allocating dedicated stack space for each call level. This ensures proper isolation of stack variables between caller and callee.
Line range hint 407-420: LGTM: Consistent stack tracking in ringbuf test case.
The r10 register configuration matches the previous test case, maintaining consistency in stack space allocation across different helper function tests.
Line range hint 429-442: LGTM: Stack tracking for non-numeric value test case.
The r10 register setup is consistent with other test cases, properly testing stack space isolation when dealing with non-numeric values.
Line range hint 508-525: Security concern: Potential pointer leak in bpf_get_stack test.
The test case comment "doesn't this leak pointers into the shared memory?" raises a valid security concern. When writing stack traces to shared memory, there's a risk of leaking sensitive pointer information.
Let's verify if there are any security measures in place:
✅ Verification successful
Let me gather more information about the security measures and implementation details around bpf_get_stack.
Let me try a different search strategy to understand the security implications.
Let me try one final search to understand the test context and implementation details.
Based on the analysis of the test cases and implementation, I can now provide a final response about the security concern regarding bpf_get_stack writing into shared memory.
Security concern about pointer leaks in bpf_get_stack is properly mitigated.
The test cases show that:
- The return value of
bpf_get_stackis typed asnumber(not a pointer), as seen in both test cases:post: - r0.type=number
- The verifier enforces strict type checking and pointer safety:
- It prevents pointer leaks through type constraints
- It has explicit checks for pointer subtraction, comparison, and arithmetic
- It enforces memory safety through region-based access control
The test comment "doesn't this leak pointers into the shared memory?" appears to be documentation of the security consideration that was already addressed in the implementation, rather than a real issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any security measures around bpf_get_stack
rg -A 5 "bpf_get_stack.*shared"
Length of output: 1455
Script:
#!/bin/bash
# Search for bpf_get_stack implementation and security checks
rg -A 10 "bpf_get_stack" --type c --type rust
# Look for any sanitization or security measures
rg -A 5 "sanitize.*stack|stack.*sanitize"
# Look for any pointer masking or scrubbing
rg -A 5 "mask.*pointer|scrub.*pointer"
Length of output: 130
Script:
#!/bin/bash
# Look for any security-related files or documentation
fd -t f "security|SECURITY|secure"
# Search for any test files related to stack or pointer security
fd -t f "test.*stack|stack.*test"
# Look for any validator or verifier implementations
rg -A 10 "verifier|validator" --type rust --type c
Length of output: 647
Script:
#!/bin/bash
# Look for test cases and implementation details around bpf_get_stack
rg -l "bpf_get_stack"
# Check the content of these files
fd . -t f -e yaml -x head -n 50 {}
# Look for any pointer or memory safety related code
rg "pointer|memory|safety" -A 5 --type yaml
Length of output: 47562
src/assertions.cpp (4)
30-34: Well-designed helper method for ValidAccess creation!
The new make_valid_access method effectively centralizes the creation of ValidAccess objects while properly handling call stack depth. This aligns well with the PR's objective of improving stack space management in bpf2bpf calls.
108-108: LGTM - Consistent usage of make_valid_access in Call operator
The changes properly utilize the new helper method while maintaining the correct access types for different memory access scenarios (readable, writable).
Also applies to: 113-113, 118-118
203-206: Good update to stack size boundary check
The change from EBPF_STACK_SIZE to EBPF_SUBPROGRAM_STACK_SIZE correctly implements the PR's objective of allocating separate stack space for each call level. The boundary check properly validates both lower and upper bounds.
🧰 Tools
🪛 cppcheck
[error] 205-205: Syntax Error
(internalAstError)
210-211: LGTM - Proper access type handling
The changes maintain correct access type specification while adopting the new helper method.
test-data/stack.yaml (1)
576-597: LGTM! Well-structured test case for invalid stack access.
The test case effectively validates the stack access boundaries in the context of bpf2bpf calls. It correctly verifies that:
- Writing beyond the allowed range (r10-513) triggers an appropriate error
- The error message properly references the call stack depth
- The stack pointer (r10) and its offset remain unchanged
src/test/ebpf_yaml.cpp (3)
286-287: LGTM: Stack offset calculation updated correctly.
The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE aligns with the PR's objective to properly handle stack space across call levels.
297-302: LGTM: Stack invariant calculations updated for total stack size.
The changes correctly update all stack offset calculations and invariant strings to use EBPF_TOTAL_STACK_SIZE, ensuring proper tracking of stack variables across all call levels.
Also applies to: 304-304, 314-314
332-334: LGTM: Stack overflow check updated correctly.
The memory size overflow check is properly updated to use EBPF_TOTAL_STACK_SIZE, maintaining the safety of stack operations.
test-data/jump.yaml (2)
496-496: LGTM: Stack offset assertion aligns with PR objectives.
The addition of the stack offset assertion for r10 is consistent with the PR's goal of standardizing stack size handling.
536-537: LGTM: Post-condition stack assertions are consistent.
The added post-condition assertions for r10's type and stack offset maintain consistency with the pre-conditions and align with the PR's standardization goals.
test-data/atomic.yaml (2)
Line range hint 267-584: LGTM: Stack offset handling is correctly implemented.
The addition of r10 stack offset information (512 bytes) across all stack-related test cases is consistent with the PR's objective of allocating 512 bytes of stack space for each call level. The implementation maintains proper stack space management while preserving the existing test coverage.
Line range hint 585-617: LGTM: Error cases properly validate invalid operations.
The error test cases effectively verify that:
- Atomic operations are rejected on non-pointer space
- CMPXCHG operations are rejected when comparing against non-number types
The error messages are clear and help ensure proper type checking in the verifier.
src/crab/array_domain.cpp (1)
819-819: LGTM! Consider verifying related constraints.
The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE aligns with the PR's objective of properly managing stack space across call levels.
Based on the learnings, let's verify if there are other similar constraints that might need updating:
✅ Verification successful
Based on the search results, I can now provide a final response. The codebase shows consistent usage of stack size constants:
LGTM! The change is consistent with codebase usage
The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE is correct and aligns with the codebase's usage. Key findings:
EBPF_TOTAL_STACK_SIZEis defined insrc/ebpf_base.hasMAX_CALL_STACK_FRAMES * EBPF_SUBPROGRAM_STACK_SIZE- All stack size validations and bounds checks throughout the codebase use
EBPF_TOTAL_STACK_SIZE - The constant is used consistently in array domain, bitset domain, and eBPF domain implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for other occurrences of EBPF_STACK_SIZE that might need updating
# Test: Look for EBPF_STACK_SIZE usage. Expect: No instances that should use EBPF_TOTAL_STACK_SIZE instead
# Search for EBPF_STACK_SIZE usage
rg "EBPF_STACK_SIZE" -A 3 -B 3
Length of output: 30
Script:
#!/bin/bash
# Let's try a different approach to find any references to stack size constants
# First, let's check for any header files that might define these constants
fd -e h -e hpp | xargs rg "EBPF_(STACK|TOTAL)_SIZE"
# Also search in source files
fd -e c -e cpp | xargs rg "EBPF_(STACK|TOTAL)_SIZE"
# Let's also check if there are any numeric constants (512, 512*N) that might be related to stack size
rg "(512|STACK_SIZE)" -A 2 -B 2
Length of output: 48435
src/asm_unmarshal.cpp (1)
341-341: LGTM! Stack bounds check updated for multi-level call support.
The change from EBPF_STACK_SIZE to EBPF_TOTAL_STACK_SIZE properly implements the stack space allocation for each call level, ensuring that local variables can function correctly within bpf2bpf calls.
src/crab/ebpf_domain.cpp (3)
2924-2924: Note potential pre-existing issue with stack pointer constraint.
The inequality inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue; may impose a lower bound where an upper bound is intended.
As per previous discussions, this issue is pre-existing and not introduced in this PR. No action is required at this time.
1381-1382: Confirm that havoc_subprogram_stack is called appropriately on exit.
The invocation of havoc_subprogram_stack(prefix); within the Exit handler ensures that the stack state is correctly reset when exiting a subprogram.
Please verify that havoc_subprogram_stack properly handles all necessary stack cleanup in various exit scenarios by running:
#!/bin/bash
# Description: Verify `havoc_subprogram_stack` is called in all exit paths.
# Check for all exit points in the code where `havoc_subprogram_stack` should be invoked.
rg -A 2 'void ebpf_domain_t::operator\(\(const Exit&' src/crab/ebpf_domain.cppThis script ensures that havoc_subprogram_stack is consistently used across all exit implementations.
1210-1212: Ensure consistent handling when interval is not a singleton.
In the check_access_stack function, when interval is not a singleton, the code skips the lower bound check. This could potentially allow access outside the intended stack boundaries if interval includes invalid values.
Please run the following script to verify that interval being non-singleton does not introduce security issues:
This script searches for all instances where is_singleton() is checked and ensures that appropriate measures are in place when it is not.
✅ Verification successful
Based on the search results, I can now provide a final response:
The non-singleton interval case is safely handled by design
The code's behavior is correct and secure for the following reasons:
- The upper bound check
ub <= EBPF_TOTAL_STACK_SIZEis always enforced regardless of whether the interval is a singleton or not, providing a critical safety boundary. - The lower bound check is only skipped for non-singleton intervals because:
- R10's stack offset is initialized to a fixed value:
EBPF_TOTAL_STACK_SIZE(as seen in the last lines of the output) - Any modifications to R10 that would make it non-singleton would trigger other security checks in the codebase
- The upper bound check remains in place as a safety net
- R10's stack offset is initialized to a fixed value:
The current implementation maintains memory safety even when the interval is not a singleton, as the critical upper bound check is always enforced.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that non-singleton `r10_stack_offset` intervals do not lead to incorrect stack access.
# Check if `r10_stack_offset` is always a singleton before `check_access_stack` is called.
ast-grep --pattern $'if ($_interval.is_singleton()) {
$$$
}' src/crab/ebpf_domain.cpp
Length of output: 226
Script:
#!/bin/bash
# Let's search for the context around check_access_stack and interval usage
# First, find the check_access_stack function definition and its usage
rg -A 10 "check_access_stack" src/crab/ebpf_domain.cpp
# Then, let's look for interval.is_singleton usage
rg -A 5 "interval\.is_singleton\(\)" src/crab/ebpf_domain.cpp
# Also search for any validation or assertions related to stack offset
rg -A 5 "stack_offset" src/crab/ebpf_domain.cpp
Length of output: 10639
Signed-off-by: Dave Thaler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
src/crab/ebpf_domain.cpp(8 hunks)
🧰 Additional context used
📓 Learnings (1)
src/crab/ebpf_domain.cpp (6)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T20:48:03.575Z
Learning: When code in a pull request is unchanged and an issue is pre-existing, avoid raising comments about it.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2060-2060
Timestamp: 2024-10-11T20:49:12.091Z
Learning: `r10` is read-only and always set to a fixed value, so in `do_mem_store`, `r10_interval` can be assumed to be a singleton without additional checks.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2057-2063
Timestamp: 2024-11-04T20:46:54.335Z
Learning: In the `do_mem_store` function, `r10_interval` is always a singleton because `r10` is immutable and always a singleton.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:952-952
Timestamp: 2024-10-11T21:10:11.826Z
Learning: In `ebpf_domain_t::havoc_subprogram_stack`, assigning the result of `stack_offset - EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth` to an `int32_t` is acceptable because `call_stack_depth` is capped, preventing overflow.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:171-171
Timestamp: 2024-10-11T21:14:48.516Z
Learning: The `check_access_stack` method in `src/crab/ebpf_domain.hpp` has documentation that may need updating to include the `call_stack_depth` parameter. This issue is pre-existing and may be addressed in future PRs.
🔇 Additional comments (5)
src/crab/ebpf_domain.cpp (5)
822-822: Constraint on r.stack_offset is appropriate.
The invariant inv += r.stack_offset <= EBPF_TOTAL_STACK_SIZE; correctly ensures that the stack_offset of each register does not exceed the total stack size.
940-956: havoc_subprogram_stack method implementation is correct.
The havoc_subprogram_stack function appropriately calculates the call_stack_depth and havocs the stack space used by subprograms.
1381-1381: Proper cleanup in Exit operation.
Adding havoc_subprogram_stack(prefix); and restore_callee_saved_registers(prefix); ensures that the stack and callee-saved registers are correctly restored upon exit.
1691-1691: Passing s.call_stack_depth to check_access_stack.
The inclusion of s.call_stack_depth in the check_access_stack call ensures accurate stack access validation based on the current call stack depth.
2057-2063: Assumption of singleton r10_interval is justified.
Since r10 is immutable and always a singleton, the code correctly assumes r10_interval is a singleton without additional checks.
|
@elazarg I've rebased, gone through all coderabbit feedback, and Alan previously reviewed and approved. Any other changes needed? |
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range comments (1)
src/test/ebpf_yaml.cpp (1)
Line range hint
295-318: Ensure proper alignment to avoid undefined behavior instack_contents_invariantThe current logic attempts to adjust
offsetto align with various type sizes using modulus operations. However, this method might not guarantee proper alignment for all types, potentially leading to undefined behavior when accessing unaligned memory.Consider modifying the alignment checks to ensure that
offsetis correctly aligned for each type before callingadd_stack_variable. Additionally, you can adjustadd_stack_variableto handle unaligned memory access safely, as suggested in the previous comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
src/asm_syntax.hpp(2 hunks)src/assertions.cpp(5 hunks)src/crab/ebpf_domain.cpp(8 hunks)src/test/conformance_check.cpp(1 hunks)src/test/ebpf_yaml.cpp(3 hunks)src/test/ebpf_yaml.hpp(1 hunks)
🧰 Additional context used
📓 Learnings (3)
src/asm_syntax.hpp (1)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/asm_syntax.hpp:350-350
Timestamp: 2024-10-11T21:17:09.031Z
Learning: In the ebpf-verifier project, avoid suggesting comments that restate information already clear from the code's member and class names.
src/assertions.cpp (2)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/assertions.cpp:171-171
Timestamp: 2024-11-04T20:44:39.115Z
Learning: When reviewing code in the ebpf-verifier project, avoid suggesting minor changes to code that is not new in the PR unless the changes are critical.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/assertions.cpp:230-231
Timestamp: 2024-11-04T20:46:27.288Z
Learning: Avoid making suggestions on code that is not new in the current PR, unless it addresses a significant issue.
src/crab/ebpf_domain.cpp (4)
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T20:48:03.575Z
Learning: When code in a pull request is unchanged and an issue is pre-existing, avoid raising comments about it.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:952-952
Timestamp: 2024-10-11T21:10:11.826Z
Learning: In `ebpf_domain_t::havoc_subprogram_stack`, assigning the result of `stack_offset - EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth` to an `int32_t` is acceptable because `call_stack_depth` is capped, preventing overflow.
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.hpp:171-171
Timestamp: 2024-10-11T21:14:48.516Z
Learning: The `check_access_stack` method in `src/crab/ebpf_domain.hpp` has documentation that may need updating to include the `call_stack_depth` parameter. This issue is pre-existing and may be addressed in future PRs.
🪛 cppcheck
src/assertions.cpp
[error] 205-205: Syntax Error
(internalAstError)
src/test/ebpf_yaml.cpp
[style] 321-321: The function 'run_conformance_test_case' is never used.
(unusedFunction)
🔇 Additional comments (14)
src/test/conformance_check.cpp (2)
24-24: Good modernization using std::byte!
The change from uint8_t to std::byte improves type safety and makes the intent clearer by using the proper type for raw byte manipulation.
25-25: LGTM!
The vector type change maintains consistency with the function signature.
src/asm_syntax.hpp (2)
54-63: LGTM! Well-implemented stack depth calculation.
The implementation correctly calculates the call stack depth by counting '/' delimiters in the stack frame prefix. The method is properly marked with [[nodiscard]] and uses efficient string traversal via std::ranges::count.
351-351: LGTM! Verify the integration with access validation.
The call_stack_depth member is correctly added to track stack depth during access validation. Let's verify its usage in the access validation logic.
✅ Verification successful
LGTM! The call_stack_depth member is properly integrated and used
The verification shows that call_stack_depth is correctly used throughout the codebase:
- Used in
check_access_stack()to calculate stack bounds based on call depth - Properly initialized in
make_valid_access()using the label's call stack depth - Applied in stack access validation to ensure proper stack space allocation (
EBPF_SUBPROGRAM_STACK_SIZE * call_stack_depth)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of call_stack_depth in access validation
# Search for ValidAccess construction and usage
rg -l "ValidAccess" | xargs rg "call_stack_depth" -B 2 -A 2
# Search for potential validation checks
ast-grep --pattern 'if ($access.call_stack_depth $$$) { $$$ }'
Length of output: 3951
src/assertions.cpp (4)
30-34: Well-structured helper method addition!
The new make_valid_access method effectively centralizes the creation of ValidAccess objects and properly handles call stack depth, which is crucial for the stack space management improvements in bpf2bpf calls.
108-108: Consistent usage of make_valid_access with appropriate access types!
The changes properly differentiate between read and write access types, maintaining the security constraints for different memory access patterns in function calls.
Also applies to: 113-113, 118-118
230-231: Consistent usage of make_valid_access!
The change maintains consistency with other modifications in the file by using the centralized helper method.
202-206: Verify stack size constant change impact
The switch from EBPF_STACK_SIZE to EBPF_SUBPROGRAM_STACK_SIZE is crucial for implementing per-call-level stack space allocation. Let's verify this change's correctness.
✅ Verification successful
Stack size constant change is correctly implemented and used consistently
The verification shows that EBPF_SUBPROGRAM_STACK_SIZE is:
- Properly defined as 512 bytes in
ebpf_base.h - Used to calculate total stack size as
MAX_CALL_STACK_FRAMES * EBPF_SUBPROGRAM_STACK_SIZE - Correctly used in stack bounds checking in
assertions.cpp - Properly integrated in the domain logic in
ebpf_domain.cppfor:- Stack offset calculations considering call depth
- Memory havoc operations
- Lower bound validations
- Has corresponding test coverage in
test-data/stack.yaml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and definition of EBPF_SUBPROGRAM_STACK_SIZE
# Check if the constant is properly defined
rg "EBPF_SUBPROGRAM_STACK_SIZE" -A 2
# Look for any related test cases
fd "stack.*\.yaml" --exec rg -l "EBPF_SUBPROGRAM_STACK_SIZE" {} \;
Length of output: 1621
🧰 Tools
🪛 cppcheck
[error] 205-205: Syntax Error
(internalAstError)
src/test/ebpf_yaml.cpp (2)
272-280: Function vector_of implementation approved
The vector_of function correctly converts a std::vector<std::byte> to a std::vector<T>, ensuring that the size of the byte vector aligns with the size of the target type T. The checks for size alignment and the use of reinterpret_cast are appropriate here.
Line range hint 321-374: Review the unused function run_conformance_test_case
The function run_conformance_test_case is defined but appears to be unused in the codebase. Maintaining unused code can increase complexity and maintenance efforts.
Run the following script to verify if the function is called elsewhere:
If the function is indeed unused, consider removing it or implementing it in the test suite if it was intended for future use.
src/crab/ebpf_domain.cpp (4)
822-822: Change is appropriate
The constraint on r.stack_offset ensures that the stack offset does not exceed the total stack size.
940-944: Calculation of call_stack_depth is clear and consistent
The updated comment now accurately reflects the calculation of call_stack_depth, accounting for both the current frame and the root frame.
2057-2063: Proper handling of r10_interval as a singleton
The code correctly checks if r10_interval is a singleton before proceeding, ensuring safe and accurate stack operations.
2924-2924:
Correct inequality direction in stack pointer constraint
The constraint inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue; seems logically inverted. To ensure that r10.svalue does not exceed EBPF_TOTAL_STACK_SIZE, consider reversing the inequality:
-inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;
+inv += r10.svalue <= EBPF_TOTAL_STACK_SIZE;This correction enforces the proper upper bound on r10.svalue.
⛔ Skipped due to learnings
Learnt from: dthaler
PR: vbpf/ebpf-verifier#721
File: src/crab/ebpf_domain.cpp:2924-2924
Timestamp: 2024-10-11T21:10:32.291Z
Learning: The constraint `inv += EBPF_TOTAL_STACK_SIZE <= r10.svalue;` in `ebpf_domain_t::setup_entry` at line 2924 in `src/crab/ebpf_domain.cpp` is pre-existing and may need correction in a future PR.
Signed-off-by: Elazar Gershuni <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (3)
src/asm_files.cpp (3)
Line range hint
24-31: LGTM! Well-designed template function with proper type constraints.The function correctly validates input parameters and ensures type safety. The use of concepts with
requiresclause is a good modern C++ practice.Consider adding a compile-time size check to prevent overflow:
template <typename T> requires std::is_trivially_copyable_v<T> static vector<T> vector_of(const char* data, const ELFIO::Elf_Xword size) { + static_assert(sizeof(T) <= std::numeric_limits<ELFIO::Elf_Xword>::max() / 2, + "Type size too large for safe division"); if (size % sizeof(T) != 0 || size > std::numeric_limits<uint32_t>::max() || !data) { throw std::runtime_error("Invalid argument to vector_of"); }
Line range hint
49-120: Consider enhancing error messages in map section parsing.While the error handling is good, the error messages could be more descriptive to aid debugging.
Consider enhancing the error messages with more context:
- throw std::runtime_error("bad maps section"); + throw std::runtime_error("Invalid maps section: " + + (s->get_data() == nullptr ? "null data" : "zero record size")); - throw std::runtime_error("bad maps section size"); + throw std::runtime_error("Maps section size " + std::to_string(s->get_size()) + + " is not divisible by record size " + std::to_string(map_record_size));
Line range hint
452-524: LGTM! Robust BTF handling with proper error checking.The BTF section handling includes comprehensive validation and proper error reporting.
Consider extracting the BTF parsing logic into a separate function for better maintainability:
static void parse_btf_line_info(const ELFIO::section* btf, const ELFIO::section* btf_ext, vector<raw_program>& programs) { if (!btf || !btf_ext) return; auto visitor = [&programs](const string& section /*... other params */) { // Existing visitor logic }; libbtf::btf_parse_line_information(vector_of<std::byte>(*btf), vector_of<std::byte>(*btf_ext), visitor); // Rest of the BTF handling logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
src/asm_files.cpp(2 hunks)src/test/ebpf_yaml.cpp(3 hunks)
🧰 Additional context used
🪛 cppcheck
src/test/ebpf_yaml.cpp
[style] 324-324: The function 'run_conformance_test_case' is never used.
(unusedFunction)
🔇 Additional comments (5)
src/asm_files.cpp (3)
33-36: LGTM! Clean wrapper implementation.
The overload properly delegates to the base implementation, maintaining the same safety guarantees.
Line range hint 38-47: LGTM! Proper cache handling in map creation.
The function correctly checks for existing entries before insertion, preventing overwrites.
Line range hint 122-450: Verify relocation bounds checking.
The relocation handling includes critical security checks, but consider additional validation for relocation types.
Let's verify the relocation type handling:
Consider adding a whitelist of allowed relocation types to prevent potential security issues with unexpected relocation types.
src/test/ebpf_yaml.cpp (2)
272-273: Improve template safety with requires clause
Adding requires std::is_trivially_copyable_v<T> ensures that the vector_of function template only operates on trivially copyable types, preventing unintended usage with incompatible types.
324-325: Potential unused function run_conformance_test_case
Static analysis indicates that the function run_conformance_test_case is never used. This may suggest that the function is obsolete or redundant.
To verify if this function is indeed unused, run the following script:
If no usages are found, consider removing the function to simplify the codebase.
🧰 Tools
🪛 cppcheck
[style] 324-324: The function 'run_conformance_test_case' is never used.
(unusedFunction)
This change allows 512 bytes of stack space per call level, rather than reusing the same 512 bytes of stack space for all call levels. This allows local variables to work correctly.
Any stack space used by a subprogram is havoced when returning from a function call.
Rather than hard coding EBPF_TOTAL_STACK_SIZE everywhere, places that actually generate warnings use r10.stack_offset. This was to avoid having to change all the YAML tests now, and then again in the future if we change (say) the max stack depth. So YAML tests that assumed 512 (stack depth = 1) still do and still work as is.
Added a test case in stack.yaml that verifies that a program cannot use stack space reserved for a subprogram to be called.
Fixes #720
Summary by CodeRabbit
Release Notes
New Features
std::byte.Bug Fixes
Documentation
Refactor
ValidAccessinstances for better maintainability.