Skip to content

Add bounds checking for apob_read #2484

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.

Sign up for GitHub

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

Draft
jamesmunns wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −24
Draft

Add bounds checking for apob_read #2484

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions task/host-sp-comms/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1269,6 +1269,10 @@ impl ServerImpl {
fn apob_read(&mut self, sequence: u64, offset: u64, size: u64) {
use drv_hf_api::ApobReadError;
use host_sp_messages::ApobReadResult;

// NOTE: This ONLY checks we are not out of bounds for `usize`,
// we will check if the requested size can be honored later in
// the `fill` closure.
let Ok(size) = usize::try_from(size) else {
self.tx_buf.encode_response(
sequence,
Expand All @@ -1285,33 +1289,37 @@ impl ServerImpl {
);
return;
};

// closure for performing the actual apob read
let fill = |buf: &mut [u8]| {
// Did the user pass in a reasonable size?
let Some(rbuf) = buf.get_mut(..size) else {
Copy link
Copy Markdown
Collaborator

@cbiffle cbiffle May 4, 2026

Choose a reason for hiding this comment

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

Do you prefer let/else to ok_or here?

Copy link
Copy Markdown
Contributor Author

@jamesmunns jamesmunns May 4, 2026

Choose a reason for hiding this comment

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

tbh I have a slight/weak preference to let-else here, but some of that is baseless preference, and some of that is habit because the .into() of ? can sometimes codegen slightly larger. That being said, I probably wouldn't refactor ok_or if I saw it.

return Err(SpToHost::ApobRead(ApobReadResult::InvalidSize));
};

// If successful: return the number of bytes read
let err = match self.hf.apob_read(offset, rbuf) {
Ok(n) => return Ok(n),
Err(e) => e,
};

// Something went wrong, ringbuf the error, and convert
// the error types
ringbuf_entry!(Trace::ApobReadError { offset, err });
let read_err = match err {
ApobReadError::NotImplemented => ApobReadResult::NotImplemented,
Copy link
Copy Markdown
Collaborator

@cbiffle cbiffle May 4, 2026

Choose a reason for hiding this comment

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

You may not know the answer to this either, since you just moved this code, but...why are these types distinct?

Copy link
Copy Markdown
Contributor Author

@jamesmunns jamesmunns May 4, 2026

Choose a reason for hiding this comment

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

use drv_hf_api::ApobReadError;
use host_sp_messages::ApobReadResult;

I think they are just different types from different sources, IPC vs IPCC types? I think there are probably just two different wire schemas we're marshalling between here, not 100% certain though.

ApobReadError::InvalidState => ApobReadResult::InvalidState,
ApobReadError::NoValidApob => ApobReadResult::NoValidApob,
ApobReadError::InvalidOffset => ApobReadResult::InvalidOffset,
ApobReadError::InvalidSize => ApobReadResult::InvalidSize,
ApobReadError::ReadFailed => ApobReadResult::ReadFailed,
};
Err(SpToHost::ApobRead(read_err))
};
self.tx_buf.try_encode_response(
sequence,
&SpToHost::ApobRead(ApobReadResult::Ok),
|buf| match self.hf.apob_read(offset, &mut buf[..size]) {
Ok(n) => Ok(n),
Err(err) => {
ringbuf_entry!(Trace::ApobReadError { offset, err });
Err(SpToHost::ApobRead(match err {
ApobReadError::NotImplemented => {
ApobReadResult::NotImplemented
}
ApobReadError::InvalidState => {
ApobReadResult::InvalidState
}
ApobReadError::NoValidApob => {
ApobReadResult::NoValidApob
}
ApobReadError::InvalidOffset => {
ApobReadResult::InvalidOffset
}
ApobReadError::InvalidSize => {
ApobReadResult::InvalidSize
}
ApobReadError::ReadFailed => ApobReadResult::ReadFailed,
}))
}
},
fill,
);
}

Expand Down
Loading