-
Notifications
You must be signed in to change notification settings - Fork 313
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
feat: Added bounds check for SkBuff::load_bytes
and relevant integration test.
#1218
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for aya-rs-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@RyanAlameddine, this pull request is now in conflict and requires a rebase. |
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.
Reviewed 5 of 5 files at r1, 2 of 2 files at r2.
Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @RyanAlameddine)
-- commits
line 2 at r3:
nit: per the contributing guide this might be better phrased as:
test: Add integration test for SkBuffContext::load_bytes
Adds a test program `read_one` that triggers a verifier error when
attempting to read one byte from the SkBuffContext.
-- commits
line 5 at r3:
nit. as above:
fix(aya-ebpf): Add bounds check to SkBuff::load_bytes
This resolves the verifier error triggered by the test case added in the previous commit
-- commits
line 8 at r3:
Please remove this commit. Instead, fix the formatting on each commit individually.
ebpf/aya-ebpf/src/programs/sk_buff.rs
line 93 at r3 (raw file):
let len = len.checked_sub(offset).ok_or(-1)?; let len = len.min(dst.len()); if !check_bounds_signed(len as c_long, 1, dst.len() as c_long) {
This could use a comment. Something like:
// 0 byte reads will trip the verifier. We need to ensure that the valid range of values for len is at least 1.
Also, please cast dst.len() as i64
to match the signature of check_bounds_signed
- don't use c_long
.
Alternative to #1187, attempting to resolve the
invalid zero-sized read
error found on discord and in my personal projects.This PR contains:
The correct inclusive bounds check.
An integration test which fails before the update and succeeds after.
This change is