services: flash client/server - #365
Conversation
Signed-off-by: Chris Frantz <cfrantz@google.com>
Signed-off-by: Chris Frantz <cfrantz@google.com>
Signed-off-by: Chris Frantz <cfrantz@google.com>
The client does not compilebazelisk build --config=virt_ast10x0 //services/flash/... That function does not exist anywhere. // util/error/lib.rs:62-79
pub struct ErrorCode(pub NonZero<u32>);
impl ErrorCode {
pub const fn new(val: u32) -> Self { … }
pub fn kernel_error(e: pw_status::Error) -> Self { … }
} |
|
|
||
| use hal_flash::{Flash, FlashAddress}; | ||
| use services_flash_opcode::*; | ||
| use userspace::time::Instant; |
There was a problem hiding this comment.
The client crate is reaching around the ipc_util shim to grab Instant from the backend directly,
re-introducing the exact dependency the shim was built to hide.
// services/flash/client.rs
- use userspace::time::Instant;
- use util_ipc::{IpcChannel, IpcHandle};
+ use util_ipc::{IpcChannel, IpcHandle, Instant};
| /// before calling this method. | ||
| pub fn handle_one(&mut self, ipc: &IpcHandle, data: &mut [u8]) -> Result<(), ErrorCode> { | ||
| let len = ipc.read(0, data).map_err(ErrorCode::kernel_error)?; | ||
| let (opcode, reqrsp) = data.split_at_mut(core::mem::size_of::<Opcode>()); |
There was a problem hiding this comment.
The opcode is read without checking the message is long enough to contain one.
It is a minor breach for API misuse since data buffer is caller controlled. The server does not defend against a sub-4-byte message. If the client sends fewer than 4 bytes, the opcode is assembled partly from the new message and partly from stale bytes left over from the previous client's request.
| ) -> Result<&'a [u8], ErrorCode> { | ||
| let req_data = data.get(..reqsz).ok_or(error::IPC_ERROR_BAD_REQ_LEN)?; | ||
| let op = EraseOp::read_from_bytes(req_data).map_err(|_| error::IPC_ERROR_BAD_REQ_LEN)?; | ||
| let Some(size) = PowerOf2Usize::new(op.size as usize) else { |
There was a problem hiding this comment.
The handle_erase method only checks if size is a power of two.
It does not check:
- Whether size is a power-of-2 the chip actually supports (the erasable_sizes_bitmap is never read here).
- Whether the address is aligned to that size.
- Whether the address is even inside the device.
|
The overall security guideline we should enforce across all services: Validate untrusted input at the IPC trust boundary, not in the hardware driver implementation. The flash IPC server is the point where client-controlled address/size/length values first enter the service. Every handler must fully establish the driver's documented preconditions — supported size (bitmap membership), address alignment, and in-bounds range — before calling into BlockingFlash/FlashDriver. |
|
|
||
| impl core::ops::AddAssign<usize> for FlashAddress { | ||
| fn add_assign(&mut self, other: usize) { | ||
| self.offset += other as u32; |
There was a problem hiding this comment.
This is an uncheked add
| type Output = Self; | ||
| fn add(self, other: usize) -> Self { | ||
| Self { | ||
| offset: self.offset + other as u32, |
There was a problem hiding this comment.
This is an uncheked add
| } | ||
| } | ||
|
|
||
| impl FlashAddress { |
There was a problem hiding this comment.
impl FlashAddress {
/// Advances the address by `n` bytes, returning `None` on overflow.
pub fn checked_add(self, n: usize) -> Option<Self> {
u32::try_from(n).ok()
.and_then(|n| self.offset.checked_add(n))
.map(Self::new)
}
}
| /// | ||
| /// Starts the asynchronous erase operation and blocks the thread using `self.blocking` | ||
| /// until the operation completes. | ||
| fn erase(&mut self, start_addr: FlashAddress, size: PowerOf2Usize) -> Result<(), Self::Error> { |
There was a problem hiding this comment.
BlockingFlash calls complete_op() after the wait.
self.driver.start_erase(start_addr, size)?;
self.blocking.wait_for_notification(); // returns () — success or timeout indistinguishable
self.driver.complete_op() // assumes the op finishedEven if the wait returned early, the very next line assumes completion and finalizes the operation. There is no branch for "timed out, so quiesce/abort instead." So the control flow, not just the signatures, assumes the wait always succeeds.
|
|
||
| /// Handles a single IPC request. | ||
| /// | ||
| /// This method performs a non-blocking read on the IPC handle. The caller |
There was a problem hiding this comment.
The comments about the ipc read being non-blocking are technically true, but the method handle_one: emphatically blocking. After the read returns, it dispatches through handle_op → handle_erase/handle_program → Flash::erase/program. For the real BlockingFlash, those call self.blocking.wait_for_notification()
2 participants
This PR depends on #364.