Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RyanAlameddine
Copy link

@RyanAlameddine RyanAlameddine commented Mar 15, 2025

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 Reviewable

Copy link

netlify bot commented Mar 15, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e803680
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67d524be572f27000861c75c
😎 Deploy Preview https://deploy-preview-1218--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added aya-bpf This is about aya-bpf (kernel) test A PR that improves test cases or CI labels Mar 15, 2025
Copy link

mergify bot commented Mar 19, 2025

@RyanAlameddine, this pull request is now in conflict and requires a rebase.

@mergify mergify bot added the needs-rebase label Mar 19, 2025
Copy link
Member

@dave-tucker dave-tucker left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya-bpf This is about aya-bpf (kernel) needs-rebase test A PR that improves test cases or CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants