-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix stack access with bpf2bpf calls #721
base: main
Are you sure you want to change the base?
Conversation
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]>
WalkthroughThe pull request introduces significant modifications to multiple files, primarily focusing on the management of stack sizes and control flow graphs (CFG) in the context of eBPF. Key changes include the introduction of new constants for stack size limits, updates to function logic for handling function macros, and enhancements to stack variable management. The changes also include the addition of new test cases to validate stack behavior during function calls and invalid access scenarios, ensuring that 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 (
|
edc0ff4
to
c2b67c7
Compare
Signed-off-by: Dave Thaler <[email protected]>
c2b67c7
to
8aaff6a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
🧹 Outside diff range comments (4)
src/crab/bitset_domain.cpp (1)
Line range hint
1-63
: Consider passingnon_numerical_bytes
by const reference for performance optimization.The static analysis tool suggests passing the
non_numerical_bytes
parameter 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_t
class 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
i
andelem_size
in 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_set
method.The changes correctly update the loop range and array indexing to use
EBPF_TOTAL_STACK_SIZE
in theto_set
method, 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_set
method.The changes correctly update array indexing and string construction to use
EBPF_TOTAL_STACK_SIZE
, maintaining consistency throughout theto_set
method 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_SIZE
toEBPF_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_SIZE
has been successfully replaced withEBPF_TOTAL_STACK_SIZE
throughout 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_FRAMES
limits the call depth,EBPF_SUBPROGRAM_STACK_SIZE
allocates space for each call, andEBPF_TOTAL_STACK_SIZE
defines 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::bitset
withEBPF_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_SIZE
constant.The change from
EBPF_STACK_SIZE
toEBPF_SUBPROGRAM_STACK_SIZE
aligns 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_SIZE
is correctly defined and used consistently throughout the codebase, and that there are no remaining instances ofEBPF_STACK_SIZE
that should have been updated.✅ Verification successful
Verified. The constant
EBPF_SUBPROGRAM_STACK_SIZE
is correctly defined insrc/ebpf_base.h
and 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_SIZE
toEBPF_TOTAL_STACK_SIZE
appears 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_SIZE
that 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_SIZE
to-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_SIZE
should be replaced withEBPF_TOTAL_STACK_SIZE
.✅ Verification successful
Verified: Stack access bounds checking update is consistent
The replacement of
EBPF_STACK_SIZE
withEBPF_TOTAL_STACK_SIZE
at line 341 insrc/asm_unmarshal.cpp
is consistent with its usage throughout the codebase.EBPF_STACK_SIZE
does 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_SIZE
In lines 292-297, the stack offsets are calculated using
EBPF_TOTAL_STACK_SIZE
. Ensure that replacingEBPF_STACK_SIZE
withEBPF_TOTAL_STACK_SIZE
correctly 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 ofoffset
On line 299,
offset
is 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 inwhile
LoopThe
while
loop 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
memcpy
OperationsThe pointer arithmetic in lines 281-282 may lead to incorrect memory access. The expression
memory_bytes.data() + offset + memory_bytes.size() - EBPF_TOTAL_STACK_SIZE
is complex and could result in out-of-bounds reads. Please verify that this calculation correctly computes the intended memory address withinmemory_bytes
and 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_offset
ensure 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_offset
does not exceed total stack sizeThe added constraint
inv += r.stack_offset <= EBPF_TOTAL_STACK_SIZE;
correctly enforces that thestack_offset
remains within the bounds of the total stack size. This helps prevent stack overflows and ensures memory safety.
927-935
: Do not assumer10_interval
is a singleton before dereferencingAt line 930, the code assumes that
r10_interval
is a singleton when callingr10_interval.singleton()->cast_to<int32_t>();
without checkingr10_interval.is_singleton()
. Ifr10_interval
is not a singleton, this could lead to a null pointer dereference or undefined behavior.
1199-1203
: Do not assumer10_interval
is a singleton before dereferencingSimilar to a previous issue, at line 1200, the code assumes
r10_interval
is a singleton without checking before dereferencing. This could result in errors ifr10_interval
is not a singleton.
2045-2048
: Do not assumer10_interval
is 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_interval
is not a singleton.
Signed-off-by: Dave Thaler <[email protected]>
e32314c
to
8a1c36a
Compare
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_SIZE
toEBPF_TOTAL_STACK_SIZE
is 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_SIZE
and update them to reflect the newEBPF_TOTAL_STACK_SIZE
constant 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_cells
andstore
, 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_cells
method (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
store
andsplit_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_FRAMES
constant 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_SIZE
constant 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_SIZE
constant 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:
r2
andr3
have the same type, which is crucial for the subtraction operation.meta_offset
andpacket_size
are defined, providing necessary context for packet-related operations.r10
is 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
r10
are 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
r1
andr2
, which creates a more comprehensive test scenario.- Adding a precondition for
r10
to 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
r1
are correct and important:
- The new
svalue
anduvalue
ranges ([3810, 3990]) accurately reflect the result of subtracting the precondition ranges ofr2
fromr1
.- 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
r10
are 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_SIZE
instead 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.hpp
are well-aligned with the PR objectives of improving stack space management for bpf2bpf calls. The addition of thehavoc_subprogram_stack
method and the update to thecheck_access_stack
method 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_stack
method.- Update any existing documentation for the
check_access_stack
method to reflect the newcall_stack_depth
parameter.- 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.hpp
are consistent with the PR's goal of improving stack space management in bpf2bpf calls. The newcall_stack_depth()
method inlabel_t
and thecall_stack_depth
member inValidAccess
provide 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
r10
register 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
r10
register 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
r10
register information in this test case forbpf_ringbuf_output
is 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
r10
register information in this test case maintains consistency with previous cases. More importantly, this test case specifically checks the behavior ofbpf_ringbuf_output
with 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
r10
register 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_stack
test 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_stack
writing 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_stack
implementation 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_stack
writes to shared memory:✅ Verification successful
Security Concern Verified: No Pointer Leakage Detected
After thorough investigation, there are no instances where
bpf_get_stack
interacts 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_SIZE
toEBPF_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_SIZE
has 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_SIZE
have been successfully replaced withEBPF_TOTAL_STACK_SIZE
throughout 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_SIZE
to-EBPF_TOTAL_STACK_SIZE
aligns 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_SIZE
and 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_SIZE
toEBPF_TOTAL_STACK_SIZE
has been consistently applied across the codebase. No instances ofEBPF_STACK_SIZE
were found, andEBPF_TOTAL_STACK_SIZE
is 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_SIZE
to0
, 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 + i
properly 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_set
method to match total stack size.Updating the loop to iterate from
-EBPF_TOTAL_STACK_SIZE
to0
ensures that all stack elements are included in the set generation.
44-44
: Confirmed accurate indexing into_set
method.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
value
now 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_access
improves code maintainabilityThe introduction of the
make_valid_access
helper method centralizes the creation ofValidAccess
objects, enhancing code readability and reducing duplication across the codebase.
108-108
: Refactored to usemake_valid_access
for nullable readable memoryUsing
make_valid_access
withor_null = true
for readable memory simplifies the code and ensures consistency in handling nullable pointers.
113-113
: Refactored to usemake_valid_access
for readable memoryConsistent usage of the
make_valid_access
method improves code clarity and maintainability by standardizing how readable memory accesses are asserted.
118-118
: Refactored to usemake_valid_access
for writable memoryThis change enhances consistency by utilizing
make_valid_access
when creating assertions for writable memory accesses, reducing code duplication.
146-146
: Utilizedmake_valid_access
in condition handling for null comparisonRefactoring to use
make_valid_access
in 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_access
for bothcond.left
andreg_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_SIZE
Changing the condition to use
-EBPF_SUBPROGRAM_STACK_SIZE
ensures 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 inmemcpy
OperationsIn lines 281-282, the pointer arithmetic within the
memcpy
functions 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_bytes
to prevent potential buffer overflows or underflows.
292-292
: Consistent Update toEBPF_TOTAL_STACK_SIZE
in Stack Offset CalculationsThe updates in lines 292 and 295-297 appropriately replace
EBPF_STACK_SIZE
withEBPF_TOTAL_STACK_SIZE
, ensuring that stack offset calculations align with the new total stack size semantics.Also applies to: 295-297
299-299
: Adjustment ofoffset
Initialization and Loop ConditionThe changes in lines 299 and 309 correctly update the
offset
initialization 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_SIZE
In 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_offset
does not exceedEBPF_TOTAL_STACK_SIZE
, which helps prevent stack overflows.
1399-1400
: Verify the use ofMAX_CALL_STACK_FRAMES
inValidAccess
In the
Comparable
operator,ValidAccess
is called withMAX_CALL_STACK_FRAMES
as 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_FRAMES
accurately represents the intended call stack depth in this context. Ifs.call_stack_depth
or another appropriate value is available, it might be more suitable.
1691-1691
: Confirm correct usage ofs.call_stack_depth
incheck_access_stack
In the
ValidAccess
handler for stack types:check_access_stack(inv, lb, ub, s.call_stack_depth);
The inclusion of
s.call_stack_depth
ensures that the access check accounts for the current call stack depth, which is important for correct stack access validation.
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
87755ad
to
c5ba6d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_depth
member to theValidAccess
struct is appropriate. It's correctly typed asint
and 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.h
forEBPF_TOTAL_STACK_SIZE
is appropriateThe inclusion of
ebpf_base.h
ensures that the constantEBPF_TOTAL_STACK_SIZE
is correctly referenced in this file.
12-12
: Updatingbits_t
to useEBPF_TOTAL_STACK_SIZE
aligns with the new stack managementChanging
bits_t
tostd::bitset<EBPF_TOTAL_STACK_SIZE>
reflects the updated total stack size and ensures that the bitset accurately represents the entire stack space.
67-67
: Adjustingwidth
withEBPF_TOTAL_STACK_SIZE
ensures valid rangeThe calculation
width = std::min(width, (int)(EBPF_TOTAL_STACK_SIZE - lb));
correctly limitswidth
to prevent exceeding the total stack size, maintaining safe operations.
85-85
: Loop condition correctly utilizesEBPF_TOTAL_STACK_SIZE
The 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
: Limitingn
withEBPF_TOTAL_STACK_SIZE
maintains stack integrityThe adjustment
n = std::min(n, (int)(EBPF_TOTAL_STACK_SIZE - lb));
properly restrictsn
to the available stack space fromlb
, ensuring that operations remain within valid bounds.
99-99
: Confiningwidth
usingEBPF_TOTAL_STACK_SIZE
safeguards stack operationsBy calculating
width = std::min(width, (int)(EBPF_TOTAL_STACK_SIZE - lb));
, the code ensures thatwidth
does not exceed the remaining stack space, which is essential for correct functionality.
112-112
: Safely assigningub
withEBPF_TOTAL_STACK_SIZE
The statement
ub = std::min(ub, (int)EBPF_TOTAL_STACK_SIZE);
guarantees thatub
stays 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_offset
remains within valid bounds.
1205-1214
: Integration of Call Stack Depth in Stack Access ChecksThe updated
check_access_stack
method now correctly accounts forcall_stack_depth
when verifying stack access bounds, enhancing the accuracy of the access checks.
1691-1691
: Proper Use ofcall_stack_depth
inValidAccess
AssertionsThe inclusion of
s.call_stack_depth
in stack access validation withinValidAccess
ensures that stack safety checks are precise for different call stack levels.
2057-2063
: Confirm Singleton Assumption forr10_interval
The code assumes that
r10_interval
is a singleton when performing stack store operations. Confirm thatr10_interval
is guaranteed to be a singleton in this context to prevent potential issues.As
r10
is read-only and initialized to a fixed value, verify thatr10_interval
will always be a singleton. If confirmed, no changes are necessary.
2925-2927
: Correct Setup of Stack Pointer ConstraintsThe constraints on
r10.svalue
and the assignment tor10.stack_offset
insetup_entry
are 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_depth
inhavoc_subprogram_stack
aligns 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_depth
calculation based on theprefix
:
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
Bug Fixes
Documentation
Refactor