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

Add bounds check on SkBuff::load_bytes #1187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

djahandarie
Copy link

@djahandarie djahandarie commented Mar 2, 2025

There have been some reports on the Discord that people run into a verifier issue with use of load_bytes:

It seems to be that it thinks it's possible that the length could be zero. At first I tried resolving it with a len !=0 check but that didn't work. Adding .max(1) did work, but that's arguably not the right semantics as it should probably error if the program really passed in 0, not just arbitrarily read one byte.

alessandrod told me about the existence of check_bounds_signed, which I've used to add a check here. This seems to also work.


This change is Reviewable

Copy link

netlify bot commented Mar 2, 2025

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1fd4f0a
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/67c3ce1ead99300008f2923d
😎 Deploy Preview https://deploy-preview-1187--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 the aya-bpf This is about aya-bpf (kernel) label Mar 2, 2025
@tamird tamird requested a review from alessandrod March 2, 2025 21:42
@tamird
Copy link
Member

tamird commented Mar 2, 2025

Looks like voodoo to me, will let @alessandrod have a look.

@alessandrod
Copy link
Collaborator

as per discord, can we add an integration test please

@@ -90,6 +90,10 @@ impl SkBuff {
let len = usize::try_from(self.len()).map_err(|core::num::TryFromIntError { .. }| -1)?;
let len = len.checked_sub(offset).ok_or(-1)?;
let len = len.min(dst.len());
let in_bounds = check_bounds_signed(len as c_long, 0, dst.len() as c_long + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be done without a let binding.

Suggested change
let in_bounds = check_bounds_signed(len as c_long, 0, dst.len() as c_long + 1);
if !check_bounds_signed(len as c_long, 0, dst.len() as c_long + 1) {
return Err(-1);
}

Also why dst.len() as c_long + 1 - surely that's out of bounds?

Copy link

mergify bot commented Mar 4, 2025

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

@mergify mergify bot added the needs-rebase label Mar 4, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants