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()
Thank you for the thorough review. I will try to address your comments in the next couple of days. |
|
The flash service binds one server to one device. But several flash devices can share a single-owner steering resource that serves one device at a time. Independent servers would race on it, so they can't run as peers. A single server must arbitrate all of them — which requires a per-request device selector so one channel can address each device safely. This is the case for the ASPEED ast1060. There are downsides to this, so we don't need to explore this right away. |
2 participants
This PR depends on #364.