From 509e85cfe49190ba4dbb9a2fc2b73dd79561cf57 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Tue, 25 Aug 2026 20:50:59 +0200 Subject: [PATCH 1/8] Wire accepted RequestUpdate to the orchestrator's UpdateRequest event The PLDM firmware-device loop now notifies an UpdateEventSink once per accepted RequestUpdate, detected as the FD's only Idle -> non-Idle transition, after the success response is sent. Rejected requests (already in update mode, bad transfer size) leave the state unchanged and never notify. The sink trait stays PLDM-flavored so this crate never depends on the orchestrator stack. The mapping to Event::UpdateRequest lives in the new orchestrator-pldm-adapter crate as UpdateRequestLatch, following the same rule that keeps HAL adapters out of orchestrator-capabilities. The latch is a bool, not a counter: the FD rejects a second RequestUpdate while one is in progress, and an undrained latch across update cycles coalesces into the single UpdateRequest the state machine would act on anyway. The firmware-update host test drives the latch end to end: the accepted RequestUpdate latches exactly one Event::UpdateRequest, the duplicate is rejected with AlreadyInUpdateMode and latches nothing, and no later command in the flow latches anything. Signed-off-by: Christina Quast --- .../orchestrator/pldm-adapter/BUILD.bazel | 24 ++++++ services/orchestrator/pldm-adapter/src/lib.rs | 84 +++++++++++++++++++ services/pldm/BUILD.bazel | 2 + services/pldm/src/firmware_device.rs | 49 ++++++++++- services/pldm/src/lib.rs | 6 +- services/pldm/tests/base_host.rs | 2 +- services/pldm/tests/firmware_update_host.rs | 51 ++++++++++- services/pldm/tests/unexpected_eid_fw_host.rs | 2 +- services/pldm/tests/unexpected_eid_host.rs | 2 +- 9 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 services/orchestrator/pldm-adapter/BUILD.bazel create mode 100644 services/orchestrator/pldm-adapter/src/lib.rs diff --git a/services/orchestrator/pldm-adapter/BUILD.bazel b/services/orchestrator/pldm-adapter/BUILD.bazel new file mode 100644 index 000000000..ce7c90b94 --- /dev/null +++ b/services/orchestrator/pldm-adapter/BUILD.bazel @@ -0,0 +1,24 @@ +# Licensed under the Apache-2.0 license +# SPDX-License-Identifier: Apache-2.0 + +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") + +rust_library( + name = "orchestrator_pldm_adapter", + srcs = [ + "src/lib.rs", + ], + crate_name = "openprot_orchestrator_pldm_adapter", + edition = "2024", + visibility = ["//visibility:public"], + deps = [ + "//services/orchestrator/sm:orchestrator_sm", + "//services/pldm:pldm_service", + ], +) + +# Host tests: build on the host platform, no kernel/QEMU. +rust_test( + name = "orchestrator_pldm_adapter_test", + crate = ":orchestrator_pldm_adapter", +) diff --git a/services/orchestrator/pldm-adapter/src/lib.rs b/services/orchestrator/pldm-adapter/src/lib.rs new file mode 100644 index 000000000..bc9bd0507 --- /dev/null +++ b/services/orchestrator/pldm-adapter/src/lib.rs @@ -0,0 +1,84 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! PLDM-backed adapter for the Boot Orchestrator's update-request input. +//! +//! [`UpdateRequestLatch`] binds the PLDM firmware-device service's +//! [`UpdateEventSink`] seam to the orchestrator's +//! [`Event::UpdateRequest`]: the PLDM run loop notifies the latch when the +//! Update Agent's `RequestUpdate` is accepted, and the orchestrator run loop +//! drains it with [`take`](UpdateRequestLatch::take). This crate depends on +//! both stacks by design — the PLDM service stays orchestrator-free and the +//! orchestrator stays transport-free, the same rule that keeps HAL adapters +//! out of `orchestrator-capabilities`. + +#![cfg_attr(not(test), no_std)] +#![forbid(unsafe_code)] +#![warn(missing_docs)] + +use openprot_orchestrator_sm::Event; +use openprot_pldm_service::firmware_device::UpdateEventSink; + +/// Latches an accepted PLDM `RequestUpdate` until the orchestrator run loop +/// drains it as [`Event::UpdateRequest`]. +/// +/// A `bool` latch, not a counter: the FD rejects a second `RequestUpdate` +/// while an update is in progress (`ALREADY_IN_UPDATE_MODE`), so at most one +/// accepted request can be outstanding per update cycle. Should a completed +/// or cancelled cycle admit a new `RequestUpdate` before the previous latch +/// is drained, the two coalesce into one [`Event::UpdateRequest`] — which is +/// what the state machine would do anyway (an update already being handled +/// defers further requests). +#[derive(Default)] +pub struct UpdateRequestLatch { + pending: bool, +} + +impl UpdateRequestLatch { + /// A latch with nothing pending. + pub const fn new() -> Self { + Self { pending: false } + } + + /// Drain the latch: [`Event::UpdateRequest`] if a `RequestUpdate` was + /// accepted since the last call, else `None`. + pub fn take(&mut self) -> Option { + self.pending.then(|| { + self.pending = false; + Event::UpdateRequest + }) + } +} + +impl UpdateEventSink for UpdateRequestLatch { + fn update_requested(&mut self) { + self.pending = true; + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_latch_yields_nothing() { + assert_eq!(UpdateRequestLatch::new().take(), None); + } + + #[test] + fn accepted_request_yields_one_event() { + let mut latch = UpdateRequestLatch::new(); + latch.update_requested(); + assert_eq!(latch.take(), Some(Event::UpdateRequest)); + assert_eq!(latch.take(), None, "a drained latch must not re-fire"); + } + + #[test] + fn undrained_notifications_coalesce() { + let mut latch = UpdateRequestLatch::new(); + latch.update_requested(); + latch.update_requested(); + assert_eq!(latch.take(), Some(Event::UpdateRequest)); + assert_eq!(latch.take(), None); + } +} diff --git a/services/pldm/BUILD.bazel b/services/pldm/BUILD.bazel index 6f9ddbc4d..b0fb31c0e 100644 --- a/services/pldm/BUILD.bazel +++ b/services/pldm/BUILD.bazel @@ -55,6 +55,8 @@ rust_test( ":pldm_service", "//services/mctp/api:mctp_api", "//services/mctp/server:mctp_server_lib", + "//services/orchestrator/pldm-adapter:orchestrator_pldm_adapter", + "//services/orchestrator/sm:orchestrator_sm", "@rust_crates//:mctp", "@rust_crates//:mctp-lib", "@rust_crates//:pldm-common", diff --git a/services/pldm/src/firmware_device.rs b/services/pldm/src/firmware_device.rs index 4246b3c30..5605c1077 100644 --- a/services/pldm/src/firmware_device.rs +++ b/services/pldm/src/firmware_device.rs @@ -62,6 +62,29 @@ pub const FD_MAX_MSG: usize = 1024; /// within this window is expected and is not treated as an error. const RESPONDER_POLL_TIMEOUT_MILLIS: u32 = 1; +/// Receiver for update-lifecycle notifications out of the PLDM FD state +/// machine. +/// +/// [`FirmwareDevice::run_terminus`] owns the PLDM state machine but has no +/// knowledge of the platform's update orchestration; this trait is the seam +/// between the two. It is deliberately PLDM-flavored (no orchestrator types) +/// so that depending on this crate never pulls in the orchestrator stack — +/// the mapping to an orchestrator event lives in an adapter crate, following +/// the same rule as the orchestrator's HAL adapters. +pub trait UpdateEventSink { + /// The Update Agent's `RequestUpdate` was accepted: the FD moved out of + /// `Idle` (into `LearnComponents`) and the success response has already + /// been sent. Called exactly once per accepted `RequestUpdate`; rejected + /// ones (`ALREADY_IN_UPDATE_MODE`, bad transfer size) never reach here + /// because they leave the FD state unchanged. + fn update_requested(&mut self); +} + +/// Drop update notifications, for callers with no orchestration to notify. +impl UpdateEventSink for () { + fn update_requested(&mut self) {} +} + /// Outcome of [`FirmwareDevice::run_terminus`]. pub enum RunTerminusResult { /// The loop exited normally (currently unreachable: `run_terminus` only @@ -151,6 +174,11 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> /// responder path to stay live even during a stalled FD-initiated /// request should pass a bounded value instead. /// + /// `sink` receives [`UpdateEventSink::update_requested`] once per + /// accepted `RequestUpdate` (the FD's only `Idle` → non-`Idle` + /// transition), after the success response has been sent. Callers with + /// nothing to notify pass `&mut ()`. + /// /// [`should_start_initiator_mode`]: pldm_interface::firmware_device::fd_context::FirmwareDeviceContext pub fn run_terminus( &mut self, @@ -158,8 +186,15 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, + sink: &mut impl UpdateEventSink, ) -> RunTerminusResult { - match self.run_terminus_inner(remote_eid, buf, timeout_millis, requester_timeout_millis) { + match self.run_terminus_inner( + remote_eid, + buf, + timeout_millis, + requester_timeout_millis, + sink, + ) { Ok(()) => RunTerminusResult::Completed, Err(e) => RunTerminusResult::StoppedByError(e), } @@ -171,6 +206,7 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, + sink: &mut impl UpdateEventSink, ) -> Result<(), PldmServiceError> { let mut responder_listener = self .responder_transport @@ -220,6 +256,11 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> timeout_millis }; responder_listener.set_timeout(poll_timeout); + // Sampled around the responder poll: `RequestUpdate` is the only + // command that takes the FD out of `Idle`, so the false→true edge + // of `is_update_mode()` identifies exactly one accepted + // `RequestUpdate` (the initiator phase above never leaves `Idle`). + let was_update_mode = self.cmd_interface.fd_ctx.is_update_mode(); match self.responder_transport.respond_once( &mut responder_listener, buf, @@ -234,7 +275,11 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> .map_err(PldmServiceError::MsgHandler) }, ) { - Ok(()) => {} + Ok(()) => { + if !was_update_mode && self.cmd_interface.fd_ctx.is_update_mode() { + sink.update_requested(); + } + } // A short poll timeout while an initiator request is active // just means no UA command arrived in that window; keep // looping so the transfer can continue. diff --git a/services/pldm/src/lib.rs b/services/pldm/src/lib.rs index bbc5fe417..fcb999adc 100644 --- a/services/pldm/src/lib.rs +++ b/services/pldm/src/lib.rs @@ -60,8 +60,10 @@ //! // `run_terminus` loops forever, interleaving inbound UA commands with any //! // FD-initiated requests (e.g. RequestFirmwareData) once an update begins. //! // It returns only on error; a `timeout_millis`/`requester_timeout_millis` -//! // of `0` blocks indefinitely while idle. -//! if let Err(e) = fd.run_terminus(UA_EID, &mut buf, 0, 0) { +//! // of `0` blocks indefinitely while idle. The final argument is an +//! // `UpdateEventSink` notified once per accepted RequestUpdate; `&mut ()` +//! // drops the notifications. +//! if let Err(e) = fd.run_terminus(UA_EID, &mut buf, 0, 0, &mut ()) { //! // handle or log error //! } //! ``` diff --git a/services/pldm/tests/base_host.rs b/services/pldm/tests/base_host.rs index e71c8b2b7..29e1fc936 100644 --- a/services/pldm/tests/base_host.rs +++ b/services/pldm/tests/base_host.rs @@ -207,7 +207,7 @@ fn base_full_chain_via_firmware_device() { // which point it returns Mctp(TimedOut); that terminating timeout means // "done", not a failure. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"), diff --git a/services/pldm/tests/firmware_update_host.rs b/services/pldm/tests/firmware_update_host.rs index 0ebef4fa1..1e8d8752c 100644 --- a/services/pldm/tests/firmware_update_host.rs +++ b/services/pldm/tests/firmware_update_host.rs @@ -26,6 +26,8 @@ use mctp::Eid; use mctp_lib::Sender; use openprot_mctp_api::Handle; use openprot_mctp_server::Server; +use openprot_orchestrator_pldm_adapter::UpdateRequestLatch; +use openprot_orchestrator_sm::Event; use openprot_pldm_service::firmware_device::{FirmwareDevice, RunTerminusResult}; use openprot_pldm_service::{MctpPldmTransport, PldmServiceError}; use pldm_common::codec::{PldmCodec, PldmCodecWithLifetime}; @@ -51,7 +53,8 @@ use pldm_common::protocol::base::{ }; use pldm_common::protocol::firmware_update::{ ComponentClassification, ComponentResponseCode, Descriptor, FirmwareDeviceState, FwUpdateCmd, - PldmFirmwareString, UpdateOptionFlags, VersionStringType, PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN, + FwUpdateCompletionCode, PldmFirmwareString, UpdateOptionFlags, VersionStringType, + PLDM_FWUP_IMAGE_SET_VER_STR_MAX_LEN, }; use pldm_common::util::fw_component::FirmwareComponent; use pldm_interface::firmware_device::fd_ops::{ComponentOperation, FdOps, FdOpsError}; @@ -312,6 +315,10 @@ fn firmware_update_full_flow_via_requester() { )); let fd_buf = RefCell::new([0u8; 1024]); + // Orchestrator-facing latch: `run_terminus` marks it on each accepted + // RequestUpdate; the assertions below drain it as `Event::UpdateRequest`. + let update_events = RefCell::new(UpdateRequestLatch::new()); + // Run one full UA->FD->UA command round-trip and return the PLDM response // payload (without the MCTP framing byte). let ua_transact = |req_pldm: &[u8]| -> Vec { @@ -332,6 +339,7 @@ fn firmware_update_full_flow_via_requester() { &mut fd_buf.borrow_mut()[..], TIMEOUT_MILLIS, TIMEOUT_MILLIS, + &mut *update_events.borrow_mut(), ) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} @@ -371,6 +379,42 @@ fn firmware_update_full_flow_via_requester() { resp[3], 0, "RequestUpdate completion code should be success" ); + assert_eq!( + update_events.borrow_mut().take(), + Some(Event::UpdateRequest), + "accepted RequestUpdate should latch exactly one orchestrator event" + ); + assert_eq!( + update_events.borrow_mut().take(), + None, + "the latch must not re-fire once drained" + ); + + // ---- Duplicate RequestUpdate: rejected, must not latch an event ---- + instance_id += 1; + let dup_update = RequestUpdateRequest::new( + instance_id, + PldmMsgType::Request, + IMAGE_SIZE, + 1, + 1, + 0, + &comp_ver, + ); + let len = dup_update + .encode(&mut buf) + .expect("encode duplicate RequestUpdate"); + let resp = ua_transact(&buf[..len]); + assert_eq!( + resp[3], + FwUpdateCompletionCode::AlreadyInUpdateMode as u8, + "second RequestUpdate should be rejected while in update mode" + ); + assert_eq!( + update_events.borrow_mut().take(), + None, + "a rejected RequestUpdate must not latch an orchestrator event" + ); // ---- PassComponentTable (Start+End): move to ReadyXfer ---- instance_id += 1; @@ -451,6 +495,11 @@ fn firmware_update_full_flow_via_requester() { ); assert!(fd_ops.verified.get(), "firmware should have been verified"); assert!(fd_ops.applied.get(), "firmware should have been applied"); + assert_eq!( + update_events.borrow_mut().take(), + None, + "no command after the accepted RequestUpdate should latch an event" + ); println!( "Firmware update host test completed: downloaded {} bytes, verified={}, applied={}", diff --git a/services/pldm/tests/unexpected_eid_fw_host.rs b/services/pldm/tests/unexpected_eid_fw_host.rs index fef06823f..055b974be 100644 --- a/services/pldm/tests/unexpected_eid_fw_host.rs +++ b/services/pldm/tests/unexpected_eid_fw_host.rs @@ -228,7 +228,7 @@ fn responder_ignores_fw_commands_from_unexpected_eid() { // "done", not a failure. `UA_EID` is the only EID `run_terminus` is told // to serve, so commands from `ATTACKER_EID` must be ignored below. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"), diff --git a/services/pldm/tests/unexpected_eid_host.rs b/services/pldm/tests/unexpected_eid_host.rs index b3d0302fa..c074f2571 100644 --- a/services/pldm/tests/unexpected_eid_host.rs +++ b/services/pldm/tests/unexpected_eid_host.rs @@ -202,7 +202,7 @@ fn responder_ignores_commands_from_unexpected_eid() { // "done", not a failure. `UA_EID` is the only EID `run_terminus` is told // to serve, so commands from `ATTACKER_EID` must be ignored below. let mut run_fd_once = - || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS) { + || match fd.run_terminus(UA_EID, &mut fd_buf, TIMEOUT_MILLIS, TIMEOUT_MILLIS, &mut ()) { RunTerminusResult::Completed => {} RunTerminusResult::StoppedByError(PldmServiceError::Mctp(e)) if e.is_timeout() => {} RunTerminusResult::StoppedByError(e) => panic!("firmware device failed: {e:?}"), From 8caba431cde867a8540a628e354231fdae1ae662 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Tue, 25 Aug 2026 21:17:03 +0200 Subject: [PATCH 2/8] Group orchestrator adapter crates under adapters/ hal-adapters moves to adapters/hal and the new PLDM adapter to adapters/pldm, so the growing family of producer-to-orchestrator adapter crates (HAL, PLDM, later SPDM) sits under one directory. Crate and target names are unchanged; each adapter keeps its own crate so composing one stack never pulls in another. Signed-off-by: Christina Quast --- docs/src/design/orchestrator/orchestrator-platform.md | 2 +- .../orchestrator/{hal-adapters => adapters/hal}/BUILD.bazel | 0 services/orchestrator/{hal-adapters => adapters/hal}/README.md | 0 .../{hal-adapters => adapters/hal}/src/gpio_boot_monitor.rs | 0 .../{hal-adapters => adapters/hal}/src/hal_boot_control.rs | 0 services/orchestrator/{hal-adapters => adapters/hal}/src/lib.rs | 0 .../orchestrator/{pldm-adapter => adapters/pldm}/BUILD.bazel | 0 .../orchestrator/{pldm-adapter => adapters/pldm}/src/lib.rs | 0 services/pldm/BUILD.bazel | 2 +- 9 files changed, 2 insertions(+), 2 deletions(-) rename services/orchestrator/{hal-adapters => adapters/hal}/BUILD.bazel (100%) rename services/orchestrator/{hal-adapters => adapters/hal}/README.md (100%) rename services/orchestrator/{hal-adapters => adapters/hal}/src/gpio_boot_monitor.rs (100%) rename services/orchestrator/{hal-adapters => adapters/hal}/src/hal_boot_control.rs (100%) rename services/orchestrator/{hal-adapters => adapters/hal}/src/lib.rs (100%) rename services/orchestrator/{pldm-adapter => adapters/pldm}/BUILD.bazel (100%) rename services/orchestrator/{pldm-adapter => adapters/pldm}/src/lib.rs (100%) diff --git a/docs/src/design/orchestrator/orchestrator-platform.md b/docs/src/design/orchestrator/orchestrator-platform.md index 3d6edc464..ddc9e3b5c 100644 --- a/docs/src/design/orchestrator/orchestrator-platform.md +++ b/docs/src/design/orchestrator/orchestrator-platform.md @@ -23,7 +23,7 @@ inside the orchestrator process: - **Device capabilities** (`services/orchestrator/capabilities`) — the narrow contracts the state machine's effects are executed against, e.g. `BootControl` (hold a device in reset / release it). HAL bindings live in - `services/orchestrator/hal-adapters`. + `services/orchestrator/adapters/hal`. - **Board device table** (`services/orchestrator/config`, schema; values in `target//devices.rs`) — declares the managed devices: reset signal, boot checkpoints and windows, commit policy. Validated at compile time, so diff --git a/services/orchestrator/hal-adapters/BUILD.bazel b/services/orchestrator/adapters/hal/BUILD.bazel similarity index 100% rename from services/orchestrator/hal-adapters/BUILD.bazel rename to services/orchestrator/adapters/hal/BUILD.bazel diff --git a/services/orchestrator/hal-adapters/README.md b/services/orchestrator/adapters/hal/README.md similarity index 100% rename from services/orchestrator/hal-adapters/README.md rename to services/orchestrator/adapters/hal/README.md diff --git a/services/orchestrator/hal-adapters/src/gpio_boot_monitor.rs b/services/orchestrator/adapters/hal/src/gpio_boot_monitor.rs similarity index 100% rename from services/orchestrator/hal-adapters/src/gpio_boot_monitor.rs rename to services/orchestrator/adapters/hal/src/gpio_boot_monitor.rs diff --git a/services/orchestrator/hal-adapters/src/hal_boot_control.rs b/services/orchestrator/adapters/hal/src/hal_boot_control.rs similarity index 100% rename from services/orchestrator/hal-adapters/src/hal_boot_control.rs rename to services/orchestrator/adapters/hal/src/hal_boot_control.rs diff --git a/services/orchestrator/hal-adapters/src/lib.rs b/services/orchestrator/adapters/hal/src/lib.rs similarity index 100% rename from services/orchestrator/hal-adapters/src/lib.rs rename to services/orchestrator/adapters/hal/src/lib.rs diff --git a/services/orchestrator/pldm-adapter/BUILD.bazel b/services/orchestrator/adapters/pldm/BUILD.bazel similarity index 100% rename from services/orchestrator/pldm-adapter/BUILD.bazel rename to services/orchestrator/adapters/pldm/BUILD.bazel diff --git a/services/orchestrator/pldm-adapter/src/lib.rs b/services/orchestrator/adapters/pldm/src/lib.rs similarity index 100% rename from services/orchestrator/pldm-adapter/src/lib.rs rename to services/orchestrator/adapters/pldm/src/lib.rs diff --git a/services/pldm/BUILD.bazel b/services/pldm/BUILD.bazel index b0fb31c0e..d420d0b92 100644 --- a/services/pldm/BUILD.bazel +++ b/services/pldm/BUILD.bazel @@ -55,7 +55,7 @@ rust_test( ":pldm_service", "//services/mctp/api:mctp_api", "//services/mctp/server:mctp_server_lib", - "//services/orchestrator/pldm-adapter:orchestrator_pldm_adapter", + "//services/orchestrator/adapters/pldm:orchestrator_pldm_adapter", "//services/orchestrator/sm:orchestrator_sm", "@rust_crates//:mctp", "@rust_crates//:mctp-lib", From d31a550e5742ec789b196c647fe40962019d035b Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Wed, 26 Aug 2026 21:31:29 +0200 Subject: [PATCH 3/8] Generalize the FD event seam to an FdEvent enum More PLDM lifecycle events are coming (cancel, transfer/verify/apply complete, activate). Replace UpdateEventSink's per-event method with a non_exhaustive FdEvent enum and a single FdEventSink::notify, so new events are one variant instead of a trait break. Every variant payload stays Copy and lifetime-free: across the DSP0267 FD surface the orchestrator-relevant edges carry only small integers; buffer-shaped data (image chunks, package data, version strings) lands behind FdOps and events name it instead of carrying it. UpdateRequested is the only variant for now. The adapter latch keeps its semantics and drops unmapped events. Signed-off-by: Christina Quast --- .../orchestrator/adapters/pldm/src/lib.rs | 21 ++++--- services/pldm/src/firmware_device.rs | 55 ++++++++++++------- services/pldm/src/lib.rs | 2 +- 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/services/orchestrator/adapters/pldm/src/lib.rs b/services/orchestrator/adapters/pldm/src/lib.rs index bc9bd0507..bc441fe0a 100644 --- a/services/orchestrator/adapters/pldm/src/lib.rs +++ b/services/orchestrator/adapters/pldm/src/lib.rs @@ -4,7 +4,7 @@ //! PLDM-backed adapter for the Boot Orchestrator's update-request input. //! //! [`UpdateRequestLatch`] binds the PLDM firmware-device service's -//! [`UpdateEventSink`] seam to the orchestrator's +//! [`FdEventSink`] seam to the orchestrator's //! [`Event::UpdateRequest`]: the PLDM run loop notifies the latch when the //! Update Agent's `RequestUpdate` is accepted, and the orchestrator run loop //! drains it with [`take`](UpdateRequestLatch::take). This crate depends on @@ -17,7 +17,7 @@ #![warn(missing_docs)] use openprot_orchestrator_sm::Event; -use openprot_pldm_service::firmware_device::UpdateEventSink; +use openprot_pldm_service::firmware_device::{FdEvent, FdEventSink}; /// Latches an accepted PLDM `RequestUpdate` until the orchestrator run loop /// drains it as [`Event::UpdateRequest`]. @@ -50,9 +50,14 @@ impl UpdateRequestLatch { } } -impl UpdateEventSink for UpdateRequestLatch { - fn update_requested(&mut self) { - self.pending = true; +/// Latches [`FdEvent::UpdateRequested`]; other FD lifecycle events have no +/// orchestrator mapping yet and are dropped here by design. +impl FdEventSink for UpdateRequestLatch { + fn notify(&mut self, event: FdEvent) { + match event { + FdEvent::UpdateRequested => self.pending = true, + _ => {} + } } } @@ -68,7 +73,7 @@ mod tests { #[test] fn accepted_request_yields_one_event() { let mut latch = UpdateRequestLatch::new(); - latch.update_requested(); + latch.notify(FdEvent::UpdateRequested); assert_eq!(latch.take(), Some(Event::UpdateRequest)); assert_eq!(latch.take(), None, "a drained latch must not re-fire"); } @@ -76,8 +81,8 @@ mod tests { #[test] fn undrained_notifications_coalesce() { let mut latch = UpdateRequestLatch::new(); - latch.update_requested(); - latch.update_requested(); + latch.notify(FdEvent::UpdateRequested); + latch.notify(FdEvent::UpdateRequested); assert_eq!(latch.take(), Some(Event::UpdateRequest)); assert_eq!(latch.take(), None); } diff --git a/services/pldm/src/firmware_device.rs b/services/pldm/src/firmware_device.rs index 5605c1077..2580b29ce 100644 --- a/services/pldm/src/firmware_device.rs +++ b/services/pldm/src/firmware_device.rs @@ -62,27 +62,44 @@ pub const FD_MAX_MSG: usize = 1024; /// within this window is expected and is not treated as an error. const RESPONDER_POLL_TIMEOUT_MILLIS: u32 = 1; -/// Receiver for update-lifecycle notifications out of the PLDM FD state -/// machine. +/// Update-lifecycle notification out of the PLDM FD state machine, one per +/// state-machine edge. /// -/// [`FirmwareDevice::run_terminus`] owns the PLDM state machine but has no -/// knowledge of the platform's update orchestration; this trait is the seam -/// between the two. It is deliberately PLDM-flavored (no orchestrator types) -/// so that depending on this crate never pulls in the orchestrator stack — -/// the mapping to an orchestrator event lives in an adapter crate, following -/// the same rule as the orchestrator's HAL adapters. -pub trait UpdateEventSink { +/// Every variant payload is `Copy` and lifetime-free by design. Anything +/// buffer-shaped (image chunks, package data, version strings) lands in +/// flash or an `FdOps`-owned buffer; an event carries at most the small +/// fixed-size values that name or qualify the edge. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum FdEvent { /// The Update Agent's `RequestUpdate` was accepted: the FD moved out of /// `Idle` (into `LearnComponents`) and the success response has already - /// been sent. Called exactly once per accepted `RequestUpdate`; rejected - /// ones (`ALREADY_IN_UPDATE_MODE`, bad transfer size) never reach here - /// because they leave the FD state unchanged. - fn update_requested(&mut self); + /// been sent. Emitted exactly once per accepted `RequestUpdate`; + /// rejected ones (`ALREADY_IN_UPDATE_MODE`, bad transfer size) never + /// reach here because they leave the FD state unchanged. + UpdateRequested, +} + +/// Receiver for [`FdEvent`] notifications out of the PLDM FD state machine. +/// +/// [`FirmwareDevice::run_terminus`] owns the PLDM state machine but has no +/// knowledge of the platform's update orchestration; this trait is the seam +/// between the two. It is deliberately PLDM-flavored (no orchestrator +/// types) so that depending on this crate never pulls in the orchestrator +/// stack; the mapping to an orchestrator event lives in an adapter crate, +/// following the same rule as the orchestrator's HAL adapters. +/// +/// [`FdEvent`] is `#[non_exhaustive]`: implementors match the variants they +/// care about and ignore the rest, so new FD lifecycle events do not break +/// existing sinks. +pub trait FdEventSink { + /// Receive one FD lifecycle event. + fn notify(&mut self, event: FdEvent); } /// Drop update notifications, for callers with no orchestration to notify. -impl UpdateEventSink for () { - fn update_requested(&mut self) {} +impl FdEventSink for () { + fn notify(&mut self, _event: FdEvent) {} } /// Outcome of [`FirmwareDevice::run_terminus`]. @@ -174,7 +191,7 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> /// responder path to stay live even during a stalled FD-initiated /// request should pass a bounded value instead. /// - /// `sink` receives [`UpdateEventSink::update_requested`] once per + /// `sink` receives [`FdEvent::UpdateRequested`] once per /// accepted `RequestUpdate` (the FD's only `Idle` → non-`Idle` /// transition), after the success response has been sent. Callers with /// nothing to notify pass `&mut ()`. @@ -186,7 +203,7 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, - sink: &mut impl UpdateEventSink, + sink: &mut impl FdEventSink, ) -> RunTerminusResult { match self.run_terminus_inner( remote_eid, @@ -206,7 +223,7 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> buf: &mut [u8], timeout_millis: u32, requester_timeout_millis: u32, - sink: &mut impl UpdateEventSink, + sink: &mut impl FdEventSink, ) -> Result<(), PldmServiceError> { let mut responder_listener = self .responder_transport @@ -277,7 +294,7 @@ impl<'a, O: FdOps, Cr: MctpClient, Cq: MctpClient> FirmwareDevice<'a, O, Cr, Cq> ) { Ok(()) => { if !was_update_mode && self.cmd_interface.fd_ctx.is_update_mode() { - sink.update_requested(); + sink.notify(FdEvent::UpdateRequested); } } // A short poll timeout while an initiator request is active diff --git a/services/pldm/src/lib.rs b/services/pldm/src/lib.rs index fcb999adc..7c7ddabce 100644 --- a/services/pldm/src/lib.rs +++ b/services/pldm/src/lib.rs @@ -61,7 +61,7 @@ //! // FD-initiated requests (e.g. RequestFirmwareData) once an update begins. //! // It returns only on error; a `timeout_millis`/`requester_timeout_millis` //! // of `0` blocks indefinitely while idle. The final argument is an -//! // `UpdateEventSink` notified once per accepted RequestUpdate; `&mut ()` +//! // `FdEventSink` notified once per accepted RequestUpdate; `&mut ()` //! // drops the notifications. //! if let Err(e) = fd.run_terminus(UA_EID, &mut buf, 0, 0, &mut ()) { //! // handle or log error From 53599715544d43db657e94e6ba81052549a7b689 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Wed, 26 Aug 2026 22:32:50 +0200 Subject: [PATCH 4/8] orchestrator: Replace single-arm match with matches! in FdEventSink impl Clippy's single_match lint (denied under -D warnings in presubmit) rejects a one-arm match with a wildcard. FdEvent derives no PartialEq, so use matches! rather than an equality check. Signed-off-by: Christina Quast --- services/orchestrator/adapters/pldm/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/services/orchestrator/adapters/pldm/src/lib.rs b/services/orchestrator/adapters/pldm/src/lib.rs index bc441fe0a..3b8304514 100644 --- a/services/orchestrator/adapters/pldm/src/lib.rs +++ b/services/orchestrator/adapters/pldm/src/lib.rs @@ -54,9 +54,8 @@ impl UpdateRequestLatch { /// orchestrator mapping yet and are dropped here by design. impl FdEventSink for UpdateRequestLatch { fn notify(&mut self, event: FdEvent) { - match event { - FdEvent::UpdateRequested => self.pending = true, - _ => {} + if matches!(event, FdEvent::UpdateRequested) { + self.pending = true; } } } From 5384e71e463033c80b805f24e4a76746506253ba Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Thu, 27 Aug 2026 21:25:23 +0200 Subject: [PATCH 5/8] orchestrator: Add the ReportSink seam for the Report effects The four Effect::Report* variants had no consumer seam. Add a non_exhaustive Report enum and one ReportSink::report, not a method or a trait per variant, plus a unit impl for a board with no management side to tell. report returns nothing: an error channel would put reports on the driver's fail-closed path, letting the act of reporting a contained failure escalate it. Trait only, composing a sink into the board follows. Assisted-by: Claude:claude-opus-5 Signed-off-by: Christina Quast --- services/orchestrator/driver/src/board.rs | 53 +++++++++++++++++++++++ services/orchestrator/driver/src/lib.rs | 2 +- services/orchestrator/driver/src/tests.rs | 51 ++++++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) diff --git a/services/orchestrator/driver/src/board.rs b/services/orchestrator/driver/src/board.rs index 78ca53d33..439df4f9f 100644 --- a/services/orchestrator/driver/src/board.rs +++ b/services/orchestrator/driver/src/board.rs @@ -89,6 +89,59 @@ pub enum Verdict { Rejected, } +/// One fact about the platform running degraded, carried outward to +/// management software. `#[non_exhaustive]`: a sink routes what it +/// recognises and ignores the rest, so a new report is not a trait break. +/// +/// Payloads stay `Copy` and lifetime-free, like the effects these mirror. A +/// report names a component only where the effect it mirrors does. +#[non_exhaustive] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Report { + /// Held in reset and out of the trust chain, so the platform runs + /// degraded. Reported once per component, as it is gated. + Isolated(ComponentId), + /// Recovery attempts exhausted and the platform halts. Reported before + /// the lockdown latch, while there is still a platform to report from. + RecoveryFailed(ComponentId), + /// An update request declined because the platform was busy. Nothing + /// staged, and the requester may ask again. Platform-wide, not + /// per-component: the machine supervises one update at a time and + /// `Event::UpdateRequest` names no component. + UpdateDeferred, + /// An update in flight superseded by recovery. Its staged image is + /// discarded and no verdict for that request follows. Platform-wide for + /// the same reason as [`Report::UpdateDeferred`]. + UpdateAborted, +} + +/// Where the driver hands its [`Report`]s. What a report becomes, a log +/// entry, a management transport message, a fault line, is board wiring. +/// +/// Infallible by design: a report names something that already happened, so +/// an undeliverable one costs information, not containment. An error channel +/// would put reports on the fail-closed path, letting the act of reporting a +/// contained failure escalate it. +pub trait ReportSink { + /// Receives one report. A sink that cannot deliver immediately queues on + /// its own side rather than stalling the effect batch. + fn report(&mut self, report: Report); +} + +/// Drops every report, for a board with no management side to tell. Losing +/// reports costs visibility only, so this is a wiring choice, not a stub. +impl ReportSink for () { + #[inline(always)] + fn report(&mut self, _report: Report) {} +} + +impl ReportSink for &mut S { + #[inline(always)] + fn report(&mut self, report: Report) { + (**self).report(report) + } +} + /// The set of platform capabilities one board composes into the /// `PlatformDriver`, named by a marker type. A new seam adds an associated /// type here and a field on [`Board`] — never another parameter. diff --git a/services/orchestrator/driver/src/lib.rs b/services/orchestrator/driver/src/lib.rs index d251b463d..acf2f9947 100644 --- a/services/orchestrator/driver/src/lib.rs +++ b/services/orchestrator/driver/src/lib.rs @@ -34,5 +34,5 @@ mod driver; #[cfg(test)] mod tests; -pub use board::{Board, BoardCapabilities, ImageSource, Verdict, Verifier}; +pub use board::{Board, BoardCapabilities, ImageSource, Report, ReportSink, Verdict, Verifier}; pub use driver::{BootWalkPoll, DriverError, PlatformDriver}; diff --git a/services/orchestrator/driver/src/tests.rs b/services/orchestrator/driver/src/tests.rs index f98851af7..ee3a0f627 100644 --- a/services/orchestrator/driver/src/tests.rs +++ b/services/orchestrator/driver/src/tests.rs @@ -772,3 +772,54 @@ fn boot_timeout_fails_closed_without_recovery() { assert_eq!(orch.state(), State::Locked); } + +// --------------------------------------------------------------------------- +// Report sink. +// --------------------------------------------------------------------------- + +/// Records what it is handed: the seam satisfied without a management +/// transport. +struct RecordingSink { + seen: std::vec::Vec, +} + +impl RecordingSink { + fn new() -> Self { + Self { + seen: std::vec::Vec::new(), + } + } +} + +impl ReportSink for RecordingSink { + fn report(&mut self, report: Report) { + self.seen.push(report); + } +} + +/// One of each report, so a test covers the whole enum. +fn every_report() -> [Report; 4] { + [ + Report::Isolated(C0), + Report::RecoveryFailed(C0), + Report::UpdateDeferred, + Report::UpdateAborted, + ] +} + +// Every report is deliverable through the seam alone, in the order handed +// over; the unit sink is a wiring choice and satisfies the same caller. +#[test] +fn every_report_reaches_a_sink() { + fn tell(sink: &mut S, reports: [Report; 4]) { + for report in reports { + sink.report(report); + } + } + + let mut recording = RecordingSink::new(); + tell(&mut recording, every_report()); + assert_eq!(recording.seen, every_report()); + + tell(&mut (), every_report()); +} From fd1c4a8821d6c2aa6b557d9d1b1c8f6177de6303 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Wed, 26 Aug 2026 10:58:51 +0200 Subject: [PATCH 6/8] orchestrator: Fold post-write readback into the Updatable contract Ready now means the staged payload is verified to the archetype's discipline: a direct-flash adapter reads written pages back before reporting Ready, a PLDM device runs its own verify step after the transfer. This drops the planned ReadBack capability; a separate post-Ready check would leak archetype knowledge to the caller and PLDM devices cannot serve it at all. The commit bullet now points at the BootConfirmed gated flow instead of a TrialBoot capability. UpdateError gains ReadbackMismatch. Folding it into Device would hide the one device fault a caller can act on differently: the write path reported success and the storage still disagrees, so a slot that keeps mismatching is worth retiring rather than retrying. MockFlashDevice demonstrates the discipline: writes hold written still, readback advances it, and a mismatch fails the step. MockPldmDevice takes a verify step of its own once the transfer completes, so a finished transfer is not yet Ready. Assisted-by: Claude (Fable 5) Signed-off-by: Christina Quast --- .../capabilities/src/updatable.rs | 202 ++++++++++++------ 1 file changed, 140 insertions(+), 62 deletions(-) diff --git a/services/orchestrator/capabilities/src/updatable.rs b/services/orchestrator/capabilities/src/updatable.rs index 7d89f645f..b68693e6c 100644 --- a/services/orchestrator/capabilities/src/updatable.rs +++ b/services/orchestrator/capabilities/src/updatable.rs @@ -24,12 +24,13 @@ /// What this trait deliberately does not claim: /// /// - **Verification** runs on the candidate before staging as -/// orchestrator policy; post-write read-back is the optional -/// `ReadBack` capability. +/// orchestrator policy. Post-write readback is no capability either: +/// it is the implementor's staging discipline (see `Ready` below). /// - **Commit.** Activation is always tentative: it proposes the staged /// image as the preferred boot target, never commits it. The commit -/// gate is the optional `TrialBoot` capability, which resolves what -/// `activate` proposed; there is no second slot-selection owner. +/// gate is the orchestrator's confirmed-boot flow (`BootConfirmed` +/// gating `SvnFloor::advance`, or the device committing internally); +/// there is no second slot-selection owner. /// - **Booting.** Resetting the device into the candidate is /// [`BootControl`](crate::BootControl). When activation takes effect /// (next reset, or a device-internal restart on self-activating @@ -52,9 +53,16 @@ /// [`abandon`](Self::abandon) instead of waiting out a blocked call. /// Liveness policy stays with the caller: it watches `written` and /// abandons a transfer that stalls too long, on its own clock. -/// - **`Ready` means ready.** The device holds the complete payload and -/// `activate` may be called. `activate` in any other staging state is -/// an error. +/// - **`Ready` means ready.** The device holds the complete payload, +/// verified to its archetype's discipline: a direct-flash adapter +/// reads written data back and re-verifies before reporting `Ready`, +/// a PLDM device runs its own verify step after the transfer: +/// `FdOps::verify` in pldm-lib, reported to the update agent as +/// `VerifyComplete`. A mismatch fails the step as +/// [`ReadbackMismatch`](UpdateError::ReadbackMismatch), kept apart +/// from [`Device`](UpdateError::Device) so a caller can retire a slot +/// that keeps mismatching. `activate` may be called; in any other +/// staging state it is an error. pub trait Updatable { /// Advances staging by one step, pulling from `payload`. /// @@ -120,7 +128,8 @@ pub enum StageProgress { /// Total payload bytes. total: u64, }, - /// The device holds the complete payload; `activate` may be called. + /// The device holds the complete, verified payload; `activate` may + /// be called. Ready, } @@ -139,6 +148,12 @@ pub enum UpdateError { Payload(PayloadReadError), /// The device failed or refused the step; staging anew may succeed. Device, + /// Written data did not read back as written. Distinct from + /// [`Device`](Self::Device) because the cause is narrower — the write + /// path reported success and the storage still disagrees — so a + /// caller can count it separately and retire a slot that keeps + /// mismatching instead of retrying forever. + ReadbackMismatch, /// `poll_stage` with an empty payload, a caller bug: an empty image /// must never stage to [`Ready`](StageProgress::Ready). EmptyPayload, @@ -152,6 +167,7 @@ impl core::fmt::Display for UpdateError { match self { UpdateError::Payload(_) => f.write_str("staging pull failed"), UpdateError::Device => f.write_str("device failed the update step"), + UpdateError::ReadbackMismatch => f.write_str("readback mismatch"), UpdateError::EmptyPayload => f.write_str("empty payload"), UpdateError::NothingStaged => f.write_str("nothing staged"), } @@ -162,7 +178,10 @@ impl core::error::Error for UpdateError { fn source(&self) -> Option<&(dyn core::error::Error + 'static)> { match self { UpdateError::Payload(fault) => Some(fault), - UpdateError::Device | UpdateError::EmptyPayload | UpdateError::NothingStaged => None, + UpdateError::Device + | UpdateError::ReadbackMismatch + | UpdateError::EmptyPayload + | UpdateError::NothingStaged => None, } } } @@ -482,12 +501,17 @@ mod tests { // shape: one fixed-size chunk request per step, at offsets the device // picks, including a retransmit. Out-of-range reads are a fault, so // the adapter clamps the device's last request to the payload end. + // A finished transfer does not mean `Ready`: the device runs its own + // verify first, one further step, the way a PLDM firmware device + // enters VERIFY and reports the outcome in `VerifyComplete`. const PLDM_CHUNK: usize = 3; struct MockPldmDevice { staged: Vec, offset: usize, retransmitted: bool, + // The whole image is transferred and its verify step is due. + verifying: bool, ready: bool, active: bool, } @@ -498,6 +522,7 @@ mod tests { staged: Vec::new(), offset: 0, retransmitted: false, + verifying: false, ready: false, active: false, } @@ -519,6 +544,13 @@ mod tests { if self.staged.len() != total { self.staged = vec![0; total]; } + // The device's own verify, the step that earns `Ready`. It + // transfers nothing, so no chunk is requested. + if self.verifying { + self.verifying = false; + self.ready = true; + return Ok(StageProgress::Ready); + } // The device re-requests the previous chunk once mid-transfer. let request = if self.offset == 2 * PLDM_CHUNK && !self.retransmitted { self.retransmitted = true; @@ -534,20 +566,19 @@ mod tests { self.offset += len; } if self.offset == total { - self.ready = true; - Ok(StageProgress::Ready) - } else { - Ok(StageProgress::Transferring { - written: self.offset as u64, - total: total as u64, - }) + self.verifying = true; } + Ok(StageProgress::Transferring { + written: self.offset as u64, + total: total as u64, + }) } fn abandon(&mut self) { self.staged = Vec::new(); self.offset = 0; self.retransmitted = false; + self.verifying = false; self.ready = false; } @@ -590,6 +621,16 @@ mod tests { total, }) ); + // The short last chunk completes the transfer. Still not `Ready`: + // the device's verify has not run. + assert_eq!( + dev.poll_stage(&payload), + Ok(StageProgress::Transferring { + written: total, + total + }) + ); + // The verify step earns `Ready`. assert_eq!(dev.poll_stage(&payload), Ok(StageProgress::Ready)); dev.activate().expect("activate failed"); @@ -597,18 +638,30 @@ mod tests { } // A direct-flash adapter, the erase-before-write shape: one step is - // one flash operation, either erasing the next sector or programming - // the next page. Erase steps pull nothing and hold `written` still. + // one flash operation, erasing the next sector, programming the next + // page, or reading a written page back. Erase and write steps hold + // `written` still; only a page that passed readback advances it, so + // `written` counts verified bytes. This is the readback discipline the + // `Ready` contract prescribes for direct-flash devices. const FLASH_PAGE: usize = 2; // bytes programmed per write step const FLASH_SECTOR: usize = 4; // bytes erased per erase step const FLASH_SECTORS: usize = 2; // sectors in the slot + // What a page written last step must read back as. + struct ExpectedReadback { + start: usize, + expected: Vec, + } + struct MockFlashDevice { slot: [u8; FLASH_SECTORS * FLASH_SECTOR], erased: [bool; FLASH_SECTORS], + pending: Option, count: usize, ready: bool, active: bool, + // Test knob: corrupt the next written page so its readback fails. + corrupt_next_write: bool, } impl MockFlashDevice { @@ -616,9 +669,11 @@ mod tests { MockFlashDevice { slot: [0; FLASH_SECTORS * FLASH_SECTOR], erased: [false; FLASH_SECTORS], + pending: None, count: 0, ready: false, active: false, + corrupt_next_write: false, } } } @@ -640,6 +695,24 @@ mod tests { if total > self.slot.len() { return Err(UpdateError::Device); } + // Readback: re-verify the page written last step. Only now + // does the page count as staged. + if let Some(ExpectedReadback { start, expected }) = self.pending.take() { + if self.slot[start..start + expected.len()] != expected[..] { + self.abandon(); + return Err(UpdateError::ReadbackMismatch); + } + self.count = start + expected.len(); + return if self.count == total { + self.ready = true; + Ok(StageProgress::Ready) + } else { + Ok(StageProgress::Transferring { + written: self.count as u64, + total: total as u64, + }) + }; + } let sector = self.count / FLASH_SECTOR; if !self.erased[sector] { self.slot[sector * FLASH_SECTOR..(sector + 1) * FLASH_SECTOR].fill(0xff); @@ -650,23 +723,28 @@ mod tests { }); } let end = (self.count + FLASH_PAGE).min(total); + let mut page = vec![0; end - self.count]; payload - .read_at(self.count as u64, &mut self.slot[self.count..end]) + .read_at(self.count as u64, &mut page) .map_err(UpdateError::Payload)?; - self.count = end; - if self.count == total { - self.ready = true; - Ok(StageProgress::Ready) - } else { - Ok(StageProgress::Transferring { - written: self.count as u64, - total: total as u64, - }) + self.slot[self.count..end].copy_from_slice(&page); + if self.corrupt_next_write { + self.corrupt_next_write = false; + self.slot[self.count] ^= 0xff; } + self.pending = Some(ExpectedReadback { + start: self.count, + expected: page, + }); + Ok(StageProgress::Transferring { + written: self.count as u64, + total: total as u64, + }) } fn abandon(&mut self) { self.erased = [false; FLASH_SECTORS]; + self.pending = None; self.count = 0; self.ready = false; } @@ -690,40 +768,25 @@ mod tests { let sector = FLASH_SECTOR as u64; assert_eq!(total as usize, FLASH_SECTORS * FLASH_SECTOR); - // Erase sector 0: a step with no pull, `written` holds still. - assert_eq!( - dev.poll_stage(&payload), - Ok(StageProgress::Transferring { written: 0, total }) - ); - assert_eq!( - dev.poll_stage(&payload), - Ok(StageProgress::Transferring { - written: page, - total - }) - ); - assert_eq!( - dev.poll_stage(&payload), - Ok(StageProgress::Transferring { - written: sector, - total - }) - ); - // Erase sector 1. - assert_eq!( - dev.poll_stage(&payload), - Ok(StageProgress::Transferring { - written: sector, - total - }) - ); - assert_eq!( - dev.poll_stage(&payload), - Ok(StageProgress::Transferring { - written: sector + page, - total, - }) - ); + // `written` after each step; it advances only on a passed readback. + let expected = [ + 0, // erase sector 0 + 0, // program page 0 + page, // page 0 readback passed + page, // program page 1 + sector, // page 1 readback passed + sector, // erase sector 1 + sector, // program page 2 + sector + page, // page 2 readback passed + sector + page, // program page 3 + ]; + for written in expected { + assert_eq!( + dev.poll_stage(&payload), + Ok(StageProgress::Transferring { written, total }) + ); + } + // The final readback completes staging. assert_eq!(dev.poll_stage(&payload), Ok(StageProgress::Ready)); dev.activate().expect("activate failed"); @@ -751,6 +814,21 @@ mod tests { assert_eq!(pldm.staged, b"chunked"); } + #[test] + fn a_readback_mismatch_is_a_staging_error() { + let mut dev = MockFlashDevice::idle(); + dev.corrupt_next_write = true; + let payload = SliceSource(b"8 bytes!"); + + let err = stage_all(&mut dev, &payload).expect_err("expected the readback mismatch"); + assert_eq!(err, UpdateError::ReadbackMismatch); + + // Staging anew after the fault is allowed and verifies clean. + stage_all(&mut dev, &payload).expect("re-staging failed"); + assert_eq!(&dev.slot, b"8 bytes!"); + dev.activate().expect("activate after re-staging failed"); + } + #[test] fn a_payload_beyond_the_slot_is_rejected_before_any_flash_op() { let mut dev = MockFlashDevice::idle(); From 32c46348e762a7d80bdc9eeb953478f050e1e359 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Fri, 21 Aug 2026 16:36:43 +0200 Subject: [PATCH 7/8] orchestrator: Wire CommitSvnFloor through the platform driver SvnFloor joins BoardCapabilities, one floor per component slot. The SVN to advance to travels in Verdict::Authenticated: verification is the only authenticated read of the manifest, so nothing else may tell the floor where to go. The driver caches it per component, clears it on rejection, and fails closed when asked to commit without it. SvnFloorBinding names the two wirings a board can choose: Erot, where the eRoT holds the floor, and SelfManaged for a component that tracks its own SVN (iRoT, or a PLDM device committing internally), whose commit is a no-op. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christina Quast --- services/orchestrator/driver/src/board.rs | 37 ++- services/orchestrator/driver/src/driver.rs | 63 ++++- services/orchestrator/driver/src/lib.rs | 11 +- services/orchestrator/driver/src/tests.rs | 281 +++++++++++++++++++-- 4 files changed, 356 insertions(+), 36 deletions(-) diff --git a/services/orchestrator/driver/src/board.rs b/services/orchestrator/driver/src/board.rs index 439df4f9f..f471a289a 100644 --- a/services/orchestrator/driver/src/board.rs +++ b/services/orchestrator/driver/src/board.rs @@ -7,6 +7,7 @@ use openprot_orchestrator_sm::{ComponentId, ComponentKind}; pub use orchestrator_capabilities::{BootControl, BootWatch}; +use orchestrator_capabilities::{Svn, SvnFloor}; /// Access to one component's active firmware image, however it is reached — /// interposed flash, a PLDM/MCTP transfer, a RAM copy in tests. @@ -46,8 +47,8 @@ impl ImageSource for &mut S { } } -/// Judges a component's firmware image; board wiring decides what -/// "authentic" means. +/// Judges a component's firmware image; board wiring decides what counts +/// as authenticated. pub trait Verifier { /// The error type reported by this verifier. type Error: core::error::Error; @@ -83,8 +84,15 @@ impl Verifier for &mut V { /// A [`Verifier`]'s judgment of one image. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Verdict { - /// Reported as `Event::VerificationPassed`. - Authenticated, + /// Reported as `Event::VerificationPassed`. Carries the image's + /// manifest SVN: verification is the only authenticated reading of the + /// manifest, so this is the one value the anti-rollback commit + /// ([`Effect::CommitSvnFloor`](openprot_orchestrator_sm::Effect)) may + /// trust. + Authenticated { + /// The verified image's security version number. + svn: Svn, + }, /// Reported as `Event::VerificationFailed`. Rejected, } @@ -154,9 +162,25 @@ pub trait BoardCapabilities { type BootControl: BootControl; /// Boot-checkpoint supervision for the managed components. type BootWatch: BootWatch; + /// The anti-rollback floor of the managed components. The SVN number + /// survives reset and power loss, otherwise a power cycle would + /// re-admit images below the floor. + type SvnFloor: SvnFloor; // Later seams: Recovery, Staging. } +/// Who keeps one component's anti-rollback floor. Spelled as its own type +/// so a board must state the choice; an eRoT floor and a device tracking +/// its own SVN are different wirings, not a present or absent value. +pub enum SvnFloorBinding { + /// The eRoT holds the floor and `CommitSvnFloor` advances it. + Erot(F), + /// The component tracks its own SVN (iRoT, or a PLDM device + /// committing internally). The eRoT keeps no second floor and its + /// `CommitSvnFloor` is a no-op. + SelfManaged, +} + /// Everything the board supplies, built once at bring-up and handed to /// `PlatformDriver::new`. Fields are public: executors may need two parts at once /// (disjoint borrows). @@ -168,6 +192,7 @@ pub trait BoardCapabilities { /// type Verifier = ManifestVerifier; // signature + SVN via the crypto engine /// type BootControl = ExtrstGpio; // per-component reset line /// type BootWatch = CheckpointWalk; // GPIO checkpoint walk over the boot window +/// type SvnFloor = OtpSvnFloor; // fuse-backed anti-rollback floor /// } /// let board = Board:: { /// images: [bmc_image, cpld_image], @@ -175,6 +200,7 @@ pub trait BoardCapabilities { /// boot_controls: [bmc_reset, cpld_reset], /// boot_watches: [bmc_walk, cpld_walk], /// component_kinds: [ComponentKind::Active, ComponentKind::Passive], +/// svn_floors: [SvnFloorBinding::Erot(bmc_floor), SvnFloorBinding::SelfManaged], /// }; /// ``` pub struct Board { @@ -193,5 +219,8 @@ pub struct Board { /// `ComponentReady` for `Active`, `Booted` for `Passive`. Comes from /// the same board table as the SM's chain, so both sides agree. pub component_kinds: [ComponentKind; N], + /// `svn_floors[i]` says who keeps `ComponentId(i)`'s anti-rollback + /// floor, same indexing as `images`. + pub svn_floors: [SvnFloorBinding; N], // Later seams add fields, e.g. recovery: [B::Recovery; N]. } diff --git a/services/orchestrator/driver/src/driver.rs b/services/orchestrator/driver/src/driver.rs index 5f3d9cc23..1a14d41f2 100644 --- a/services/orchestrator/driver/src/driver.rs +++ b/services/orchestrator/driver/src/driver.rs @@ -6,8 +6,8 @@ use openprot_orchestrator_sm::{ComponentId, ComponentKind, Effect, EffectError, Event, Platform}; -use crate::board::{Board, BoardCapabilities, ImageSource, Verdict, Verifier}; -use orchestrator_capabilities::{BootControl, BootWatch, WalkVerdict}; +use crate::board::{Board, BoardCapabilities, ImageSource, SvnFloorBinding, Verdict, Verifier}; +use orchestrator_capabilities::{BootControl, BootWatch, Svn, SvnFloor, WalkVerdict}; /// Why the driver could not carry out an effect. #[derive(Clone, Copy, PartialEq, Eq, Debug)] @@ -23,6 +23,11 @@ pub enum DriverError { VerifierFault, /// The component's boot control could not actuate the reset line. BootControlFault, + /// A floor commit was asked for a component with no verified image — + /// the SVN to advance to is unknown; fail closed. + NoVerifiedImage, + /// The component's SVN floor could not be advanced. + SvnFloorFault, } impl core::fmt::Display for DriverError { @@ -33,6 +38,8 @@ impl core::fmt::Display for DriverError { DriverError::NotStaged => "no image staged for this component", DriverError::VerifierFault => "verifier could not perform the check", DriverError::BootControlFault => "boot control could not actuate the reset", + DriverError::NoVerifiedImage => "no verified image to commit the floor to", + DriverError::SvnFloorFault => "svn floor could not be advanced", }) } } @@ -50,6 +57,10 @@ pub struct PlatformDriver { /// terminal verdict. Only watched walks are polled, so a finished or /// quiesced walk emits no stale event. watching: [bool; N], + /// `verified_svn[i]` is the manifest SVN of `ComponentId(i)`'s last + /// authenticated image — the only value a floor commit may trust. + /// `None` until a verification passes; cleared again on rejection. + verified_svn: [Option; N], } impl PlatformDriver { @@ -60,9 +71,19 @@ impl PlatformDriver { board, staged: None, watching: [false; N], + verified_svn: [None; N], } } + /// The board wiring, read-only, for the tests: they observe a + /// capability after it moved into the driver, instead of every mock + /// smuggling out a shared handle. Real consumers get targeted queries + /// when they exist — not this. + #[cfg(test)] + pub(crate) fn board(&self) -> &Board { + &self.board + } + /// `id`'s image source. Takes the array rather than `&mut self` so the /// caller can borrow `board.verifier` alongside the returned image. fn source(images: &mut [B::Image; N], id: ComponentId) -> Result<&mut B::Image, DriverError> { @@ -95,12 +116,37 @@ impl PlatformDriver { .verifier .verify(id, source) .map_err(|_| DriverError::VerifierFault)?; + let idx = id.get() as usize; Ok(match verdict { - Verdict::Authenticated => Event::VerificationPassed(id), - Verdict::Rejected => Event::VerificationFailed(id), + Verdict::Authenticated { svn } => { + self.verified_svn[idx] = Some(svn); + Event::VerificationPassed(id) + } + Verdict::Rejected => { + self.verified_svn[idx] = None; + Event::VerificationFailed(id) + } }) } + /// Advance `id`'s anti-rollback floor to its verified image's SVN. + /// A self-managed component keeps its own floor; the commit is a + /// no-op. A target at or below the current floor is the capability's + /// documented no-op, so a replayed commit is harmless. + pub fn commit_svn_floor(&mut self, id: ComponentId) -> Result<(), DriverError> { + let idx = id.get() as usize; + let SvnFloorBinding::Erot(floor) = self + .board + .svn_floors + .get_mut(idx) + .ok_or(DriverError::UnknownComponent)? + else { + return Ok(()); + }; + let svn = self.verified_svn[idx].ok_or(DriverError::NoVerifiedImage)?; + floor.advance(svn).map_err(|_| DriverError::SvnFloorFault) + } + /// `id`'s reset actuator. fn boot_control(&mut self, id: ComponentId) -> Result<&mut B::BootControl, DriverError> { self.board @@ -212,21 +258,20 @@ impl Platform for PlatformDriver { Effect::VerifyFirmware(id) => self.verify_firmware(id).map(Some), Effect::ReleaseReset(id) => self.release_reset(id).map(|_| None), Effect::AssertReset(id) => self.assert_reset(id).map(|_| None), + Effect::CommitSvnFloor(id) => self.commit_svn_floor(id).map(|_| None), // No board capability is composed for these seams yet, so they // fail closed here instead of behind stub methods. Each group // gains an executor when its capability joins // [`BoardCapabilities`], as BootControl did above: recovery // sourcing for RecoverComponent; update staging, authentication - // and trial activation for the update quartet; anti-rollback - // commit for CommitSvnFloor; evidence signing for - // SignAttestation; the management reporting path for the Report - // effects; the terminal latch for LatchLockdown. + // and trial activation for the update quartet; evidence signing + // for SignAttestation; the management reporting path for the + // Report effects; the terminal latch for LatchLockdown. Effect::RecoverComponent { .. } | Effect::AuthenticateUpdate | Effect::StageUpdate | Effect::ActivateUpdate | Effect::DiscardStaged - | Effect::CommitSvnFloor(_) | Effect::SignAttestation | Effect::ReportIsolated(_) | Effect::ReportRecoveryFailed(_) diff --git a/services/orchestrator/driver/src/lib.rs b/services/orchestrator/driver/src/lib.rs index acf2f9947..b2644addc 100644 --- a/services/orchestrator/driver/src/lib.rs +++ b/services/orchestrator/driver/src/lib.rs @@ -16,9 +16,10 @@ //! //! Everything device-specific arrives through the seams in [`board`]: //! image access ([`ImageSource`]), image judgment ([`Verifier`]), reset -//! actuation ([`orchestrator_capabilities::BootControl`]) and boot -//! supervision ([`orchestrator_capabilities::BootWatch`]), bundled in one -//! [`Board`] built by the board's composition crate. +//! actuation ([`orchestrator_capabilities::BootControl`]), boot +//! supervision ([`orchestrator_capabilities::BootWatch`]) and the +//! anti-rollback floor ([`orchestrator_capabilities::SvnFloor`]), bundled +//! in one [`Board`] built by the board's composition crate. //! //! Boot-walk verdicts are the one asynchronous read: the run loop calls //! [`PlatformDriver::poll_boot_walks`] and dispatches the returned events @@ -34,5 +35,7 @@ mod driver; #[cfg(test)] mod tests; -pub use board::{Board, BoardCapabilities, ImageSource, Report, ReportSink, Verdict, Verifier}; +pub use board::{ + Board, BoardCapabilities, ImageSource, Report, ReportSink, SvnFloorBinding, Verdict, Verifier, +}; pub use driver::{BootWalkPoll, DriverError, PlatformDriver}; diff --git a/services/orchestrator/driver/src/tests.rs b/services/orchestrator/driver/src/tests.rs index ee3a0f627..c18b82f41 100644 --- a/services/orchestrator/driver/src/tests.rs +++ b/services/orchestrator/driver/src/tests.rs @@ -5,9 +5,10 @@ extern crate std; use crate::*; use openprot_orchestrator_sm::{ - ComponentAttrs, ComponentId, ComponentKind, Event, Orchestrator, PowerOnResult, State, + ComponentAttrs, ComponentId, ComponentKind, Effect, Event, Orchestrator, Platform, + PowerOnResult, State, }; -use orchestrator_capabilities::{BootWatch, FailureCause, WalkVerdict}; +use orchestrator_capabilities::{BootWatch, FailureCause, Svn, SvnFloor, WalkVerdict}; const C0: ComponentId = ComponentId::new(0); @@ -36,9 +37,14 @@ impl core::fmt::Display for MemFault { impl core::error::Error for MemFault {} -/// RAM-backed image source — the seam satisfied without a HAL. +/// RAM-backed image source — the seam satisfied without a HAL. A re-flash +/// can be queued: it lands on the first re-open (a re-stage), the way real +/// flash changes underneath a source between stagings — never on the +/// initial staging. struct MemImage { data: std::vec::Vec, + reflash: Option>, + opened: bool, fail_open: bool, fail_read: bool, } @@ -47,10 +53,18 @@ impl MemImage { fn holding(data: std::vec::Vec) -> Self { Self { data, + reflash: None, + opened: false, fail_open: false, fail_read: false, } } + + /// Queue a re-flash: the first re-open stages `data` instead. + fn reflash_on_reopen(mut self, data: std::vec::Vec) -> Self { + self.reflash = Some(data); + self + } } impl ImageSource for MemImage { @@ -60,6 +74,10 @@ impl ImageSource for MemImage { if self.fail_open { return Err(MemFault); } + if let Some(data) = self.reflash.take_if(|_| self.opened) { + self.data = data; + } + self.opened = true; Ok(()) } @@ -91,6 +109,10 @@ impl core::error::Error for VerifierError {} /// chunks. struct XorVerifier { fault: bool, + /// The manifest SVN this verifier reports for an authenticated image. + /// A real verifier reads it from the signed manifest of the image it + /// just checked; the test image format has no manifest, so tests pin it. + svn: u32, } impl Verifier for XorVerifier { @@ -122,7 +144,7 @@ impl Verifier for XorVerifier { } let ok = len > IMAGE_MAGIC.len() && magic == IMAGE_MAGIC && xor == 0; Ok(if ok { - Verdict::Authenticated + Verdict::Authenticated { svn: Svn(self.svn) } } else { Verdict::Rejected }) @@ -224,6 +246,52 @@ impl BootWatch for MockWalk { } } +#[derive(Debug, PartialEq, Eq)] +struct FloorFaultInjected; + +impl core::fmt::Display for FloorFaultInjected { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str("floor fault injected") + } +} + +impl core::error::Error for FloorFaultInjected {} + +/// An SVN floor without storage; tests read it back through the +/// capability's own `floor()` via `PlatformDriver::board`. +struct MockFloor { + floor: u32, + fail: bool, +} + +impl MockFloor { + fn new() -> Self { + Self { + floor: 0, + fail: false, + } + } +} + +impl orchestrator_capabilities::SvnFloor for MockFloor { + type Error = FloorFaultInjected; + + fn floor(&self) -> Result { + if self.fail { + return Err(FloorFaultInjected); + } + Ok(Svn(self.floor)) + } + + fn advance(&mut self, to: Svn) -> Result<(), FloorFaultInjected> { + if self.fail { + return Err(FloorFaultInjected); + } + self.floor = self.floor.max(to.0); + Ok(()) + } +} + /// The test board's type choices. struct MockBoard; @@ -232,15 +300,20 @@ impl BoardCapabilities for MockBoard { type Verifier = XorVerifier; type BootControl = MockReset; type BootWatch = MockWalk; + type SvnFloor = MockFloor; } fn driver(images: [MemImage; 1]) -> PlatformDriver { PlatformDriver::new(Board { images, - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [MockReset::new()], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }) } @@ -320,10 +393,14 @@ fn verifier_fault_fails_closed() { let mut orch = orchestrator(); let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: true }, + verifier: XorVerifier { + fault: true, + svn: 5, + }, boot_controls: [MockReset::new()], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); orch.dispatch(&mut driver, Event::PowerGood(PowerOnResult::Provisioned)); @@ -340,10 +417,17 @@ fn verify_for_a_different_component_is_refused() { MemImage::holding(valid_image()), MemImage::holding(valid_image()), ], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [MockReset::new(), MockReset::new()], boot_watches: [MockWalk::idle(), MockWalk::idle()], component_kinds: [ComponentKind::Passive, ComponentKind::Passive], + svn_floors: [ + SvnFloorBinding::Erot(MockFloor::new()), + SvnFloorBinding::Erot(MockFloor::new()), + ], }); driver.stage_firmware(C0).unwrap(); @@ -374,21 +458,23 @@ fn verify_of_unknown_component_is_refused() { #[test] fn reset_release_and_assert_reach_the_boot_control() { - let control = MockReset::new(); - let held = control.held.clone(); let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: false }, - boot_controls: [control], + verifier: XorVerifier { + fault: false, + svn: 5, + }, + boot_controls: [MockReset::new()], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); driver.release_reset(C0).unwrap(); - assert!(!held.get()); + assert!(!driver.board().boot_controls[0].held.get()); driver.assert_reset(C0).unwrap(); - assert!(held.get()); + assert!(driver.board().boot_controls[0].held.get()); } #[test] @@ -411,10 +497,14 @@ fn reset_line_fault_is_reported() { control.fail = true; let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [control], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); assert_eq!(driver.release_reset(C0), Err(DriverError::BootControlFault)); @@ -477,6 +567,7 @@ impl BoardCapabilities for WatchBoard { type Verifier = LineWatchingVerifier; type BootControl = MockReset; type BootWatch = MockWalk; + type SvnFloor = MockFloor; } // The at-rest guarantee end to end: the component is still held while its @@ -489,13 +580,17 @@ fn release_follows_verification() { let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], verifier: LineWatchingVerifier { - inner: XorVerifier { fault: false }, + inner: XorVerifier { + fault: false, + svn: 5, + }, line: held.clone(), held_during_verify: held_during_verify.clone(), }, boot_controls: [control], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); let mut orch = orchestrator(); @@ -518,10 +613,14 @@ fn failed_release_fails_closed() { let held = control.held.clone(); let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [control], boot_watches: [MockWalk::idle()], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); let mut orch = orchestrator(); @@ -546,10 +645,17 @@ fn walk_driver( MemImage::holding(valid_image()), MemImage::holding(valid_image()), ], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [MockReset::new(), MockReset::new()], boot_watches: walks, component_kinds, + svn_floors: [ + SvnFloorBinding::Erot(MockFloor::new()), + SvnFloorBinding::Erot(MockFloor::new()), + ], }) } @@ -729,10 +835,14 @@ fn booted_walk_settles_in_ready() { let mut orch = orchestrator(); let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [MockReset::new()], boot_watches: [MockWalk::scripted(std::vec![WalkVerdict::Complete])], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); orch.dispatch(&mut driver, Event::PowerGood(PowerOnResult::Provisioned)); @@ -754,13 +864,17 @@ fn boot_timeout_fails_closed_without_recovery() { let mut orch = orchestrator(); let mut driver = PlatformDriver::::new(Board { images: [MemImage::holding(valid_image())], - verifier: XorVerifier { fault: false }, + verifier: XorVerifier { + fault: false, + svn: 5, + }, boot_controls: [MockReset::new()], boot_watches: [MockWalk::scripted(std::vec![WalkVerdict::Failed { checkpoint: "heartbeat", cause: FailureCause::TimedOut, }])], component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], }); orch.dispatch(&mut driver, Event::PowerGood(PowerOnResult::Provisioned)); @@ -823,3 +937,132 @@ fn every_report_reaches_a_sink() { tell(&mut (), every_report()); } + +// --------------------------------------------------------------------------- +// Anti-rollback floor commits. +// --------------------------------------------------------------------------- + +// The floor may only move to the SVN the verifier vouched for, and only +// after a verification has passed — the two halves of the commit contract. +#[test] +fn commit_advances_the_floor_to_the_verified_svn() { + let mut driver = PlatformDriver::::new(Board { + images: [MemImage::holding(valid_image())], + verifier: XorVerifier { + fault: false, + svn: 5, + }, + boot_controls: [MockReset::new()], + boot_watches: [MockWalk::idle()], + component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], + }); + + driver + .execute(Effect::ReadFirmware(C0)) + .expect("stage failed"); + driver + .execute(Effect::VerifyFirmware(C0)) + .expect("verify failed"); + + assert_eq!(driver.execute(Effect::CommitSvnFloor(C0)), Ok(None)); + // Read back through the capability's own seam. + let SvnFloorBinding::Erot(floor) = &driver.board().svn_floors[0] else { + panic!("C0 is wired with an eRoT floor"); + }; + assert_eq!( + floor.floor(), + Ok(Svn(5)), + "floor advanced to the verifier's SVN" + ); +} + +// A component wired without an eRoT floor tracks its own SVN. The commit +// succeeds even with no verified image cached: there is no floor to +// mis-advance. +#[test] +fn commit_without_an_erot_floor_is_a_no_op() { + let mut driver = PlatformDriver::::new(Board { + images: [MemImage::holding(valid_image())], + verifier: XorVerifier { + fault: false, + svn: 5, + }, + boot_controls: [MockReset::new()], + boot_watches: [MockWalk::idle()], + component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::SelfManaged], + }); + + assert_eq!(driver.execute(Effect::CommitSvnFloor(C0)), Ok(None)); +} + +#[test] +fn commit_without_a_verified_image_fails_closed() { + let mut driver = driver([MemImage::holding(valid_image())]); + + assert_eq!( + driver.commit_svn_floor(C0), + Err(DriverError::NoVerifiedImage) + ); +} + +// A rejection must clear the cached SVN, or the floor could commit against +// an image that is no longer the authenticated one. +#[test] +fn rejected_image_clears_the_verified_svn() { + let mut corrupt = valid_image(); + corrupt[7] ^= 0x01; + let mut driver = PlatformDriver::::new(Board { + images: [MemImage::holding(valid_image()).reflash_on_reopen(corrupt)], + verifier: XorVerifier { + fault: false, + svn: 5, + }, + boot_controls: [MockReset::new()], + boot_watches: [MockWalk::idle()], + component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(MockFloor::new())], + }); + + driver.stage_firmware(C0).expect("stage failed"); + assert_eq!( + driver.verify_firmware(C0), + Ok(Event::VerificationPassed(C0)) + ); + + // The queued re-flash lands on the re-stage; the rejection must take + // the cached SVN with it. + driver.stage_firmware(C0).expect("re-stage failed"); + assert_eq!( + driver.verify_firmware(C0), + Ok(Event::VerificationFailed(C0)) + ); + + assert_eq!( + driver.commit_svn_floor(C0), + Err(DriverError::NoVerifiedImage) + ); +} + +#[test] +fn floor_fault_is_reported() { + let mut mock = MockFloor::new(); + mock.fail = true; + let mut driver = PlatformDriver::::new(Board { + images: [MemImage::holding(valid_image())], + verifier: XorVerifier { + fault: false, + svn: 5, + }, + boot_controls: [MockReset::new()], + boot_watches: [MockWalk::idle()], + component_kinds: [ComponentKind::Passive], + svn_floors: [SvnFloorBinding::Erot(mock)], + }); + + driver.stage_firmware(C0).expect("stage failed"); + driver.verify_firmware(C0).expect("verify failed"); + + assert_eq!(driver.commit_svn_floor(C0), Err(DriverError::SvnFloorFault)); +} From f9b52223af0ccfa2eb2e8d3dd87022ad71d17eb8 Mon Sep 17 00:00:00 2001 From: Christina Quast Date: Tue, 8 Sep 2026 13:15:55 +0200 Subject: [PATCH 8/8] orchestrator: add IncrementalVerifier capability trait Polled incremental verification seam for the update path: judge a candidate image one bounded step at a time so the single-threaded runtime stays live while hashing megabytes of payload. The trait lives in orchestrator-capabilities (dependency-free leaf). start() opens a session, poll(payload) does one implementor-chosen chunk of work and returns Processing/Authenticated/Rejected. Faults (unreadable payload, crypto error, misuse) are Err, never a verdict, so a check that could not run cannot forge a judgment. After an Err the session is abandoned; only start() restores defined state. Explicit start() (unlike Updatable's implicit idle-to-active) catches an accidental extra poll after a verdict instead of silently re-hashing from zero. Assisted-by: Claude --- .../orchestrator/capabilities/BUILD.bazel | 1 + .../capabilities/src/incremental_verifier.rs | 287 ++++++++++++++++++ services/orchestrator/capabilities/src/lib.rs | 6 + 3 files changed, 294 insertions(+) create mode 100644 services/orchestrator/capabilities/src/incremental_verifier.rs diff --git a/services/orchestrator/capabilities/BUILD.bazel b/services/orchestrator/capabilities/BUILD.bazel index 29f69d1f0..4c413c822 100644 --- a/services/orchestrator/capabilities/BUILD.bazel +++ b/services/orchestrator/capabilities/BUILD.bazel @@ -9,6 +9,7 @@ rust_library( "src/boot_control.rs", "src/boot_watch.rs", "src/evidence.rs", + "src/incremental_verifier.rs", "src/lib.rs", "src/lockdown_latch.rs", "src/svn_floor.rs", diff --git a/services/orchestrator/capabilities/src/incremental_verifier.rs b/services/orchestrator/capabilities/src/incremental_verifier.rs new file mode 100644 index 000000000..842859368 --- /dev/null +++ b/services/orchestrator/capabilities/src/incremental_verifier.rs @@ -0,0 +1,287 @@ +// Licensed under the Apache-2.0 license +// SPDX-License-Identifier: Apache-2.0 + +//! The [`IncrementalVerifier`] update-verification capability contract. + +use crate::PayloadSource; + +/// Incremental firmware verification: judge a candidate image one bounded +/// step at a time, so a single-threaded runtime stays live while hashing +/// megabytes of payload. +/// +/// The caller (the platform driver's update pump) calls [`start`] once to +/// begin a session, then calls [`poll`] repeatedly on its own clock. Each +/// poll does at most one read from the payload and one hash update, then +/// returns. The chunk size is implementor-chosen, sized so each poll fits +/// the caller's per-poll time budget. The caller watches `hashed` +/// progress and abandons a session that stalls, on its own budget; the +/// verifier never judges liveness. +/// +/// A session produces exactly one terminal verdict: [`Authenticated`] or +/// [`Rejected`]. A fault (unreadable payload, crypto engine error) is +/// `Err`, not a verdict, because a check that could not run must not forge +/// a judgment. After an `Err`, the session is abandoned; only [`start`] +/// returns the verifier to a defined state. +/// +/// [`start`] discards any in-progress session, so the caller can abandon +/// and restart without a separate reset method. Unlike `Updatable`, which +/// starts implicitly from idle, the explicit `start` ensures that one +/// extra poll after a verdict is caught rather than silently re-hashing +/// from zero. The boot-time synchronous `Verifier` (in the driver crate) +/// is unaffected: it stays one-shot for the chain walk, where the image is +/// small and local. +/// +/// [`start`]: IncrementalVerifier::start +/// [`poll`]: IncrementalVerifier::poll +/// [`Authenticated`]: VerifyStep::Authenticated +/// [`Rejected`]: VerifyStep::Rejected +pub trait IncrementalVerifier { + /// The error reported when the check itself cannot run: crypto fault, + /// unreadable payload, or misuse (poll outside a session). A bad image + /// is [`Rejected`](VerifyStep::Rejected), not an error. + type Error: core::error::Error; + + /// Discards any in-progress session and prepares to verify from the + /// start of the image. The next [`poll`](Self::poll) begins hashing + /// at offset zero. + fn start(&mut self); + + /// Processes one bounded step: at most one read from `payload` and one + /// hash update, then returns. Never waits on device progress, never + /// sleeps. Returns the session's current state: still processing + /// (with byte-level progress), or a terminal verdict. + /// + /// Calling `poll` after a terminal verdict (or before [`start`](Self::start)) + /// is an error. + fn poll(&mut self, payload: &dyn PayloadSource) -> Result; +} + +/// One step of an incremental verification session. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum VerifyStep { + /// One chunk hashed. `hashed` bytes processed so far out of `total`. + /// The caller watches progress and abandons a session whose `hashed` + /// stops advancing, on its own stall budget. + Processing { hashed: u64, total: u64 }, + /// The complete image authenticated (signature and policy checks passed). + Authenticated, + /// The complete image was checked and found invalid. + Rejected, +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::{PayloadReadError, PayloadSource}; + + // A PayloadSource over a plain byte slice. + struct SlicePayload(&'static [u8]); + + impl PayloadSource for SlicePayload { + fn len(&self) -> u64 { + self.0.len() as u64 + } + + fn read_at(&self, offset: u64, buf: &mut [u8]) -> Result<(), PayloadReadError> { + let start = usize::try_from(offset).map_err(|_| PayloadReadError::OutOfRange)?; + let end = start + .checked_add(buf.len()) + .ok_or(PayloadReadError::OutOfRange)?; + buf.copy_from_slice(self.0.get(start..end).ok_or(PayloadReadError::OutOfRange)?); + Ok(()) + } + } + + // A verifier that hashes 4 bytes per poll and accepts any image whose + // first byte is nonzero. + struct ChunkedVerifier { + offset: u64, + total: u64, + active: bool, + } + + #[derive(Debug, PartialEq)] + struct VerifierFault; + + impl core::fmt::Display for VerifierFault { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str("verifier fault") + } + } + + impl core::error::Error for VerifierFault {} + + impl ChunkedVerifier { + fn new() -> Self { + Self { + offset: 0, + total: 0, + active: false, + } + } + } + + impl IncrementalVerifier for ChunkedVerifier { + type Error = VerifierFault; + + fn start(&mut self) { + self.offset = 0; + self.total = 0; + self.active = true; + } + + fn poll(&mut self, payload: &dyn PayloadSource) -> Result { + if !self.active { + return Err(VerifierFault); + } + if self.total == 0 { + self.total = payload.len(); + } + if self.offset >= self.total { + self.active = false; + let mut first = [0u8; 1]; + payload.read_at(0, &mut first).map_err(|_| VerifierFault)?; + return Ok(if first[0] != 0 { + VerifyStep::Authenticated + } else { + VerifyStep::Rejected + }); + } + let chunk = core::cmp::min(4, (self.total - self.offset) as usize); + let mut buf = [0u8; 4]; + payload + .read_at(self.offset, &mut buf[..chunk]) + .map_err(|_| VerifierFault)?; + self.offset += chunk as u64; + Ok(VerifyStep::Processing { + hashed: self.offset, + total: self.total, + }) + } + } + + #[test] + fn multi_poll_until_authenticated() { + let payload = SlicePayload(&[0xAA; 10]); + let mut v = ChunkedVerifier::new(); + v.start(); + + // 4 bytes, 4 bytes, 2 bytes = 3 Processing steps, then verdict. + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 4, + total: 10 + }) + ); + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 8, + total: 10 + }) + ); + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 10, + total: 10 + }) + ); + assert_eq!(v.poll(&payload), Ok(VerifyStep::Authenticated)); + } + + #[test] + fn rejected_image() { + let payload = SlicePayload(&[0x00; 8]); + let mut v = ChunkedVerifier::new(); + v.start(); + + // Drain processing steps. + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 4, + total: 8 + }) + ); + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 8, + total: 8 + }) + ); + assert_eq!(v.poll(&payload), Ok(VerifyStep::Rejected)); + } + + #[test] + fn start_discards_in_progress_session() { + let payload = SlicePayload(&[0xFF; 12]); + let mut v = ChunkedVerifier::new(); + v.start(); + + // Partial progress. + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 4, + total: 12 + }) + ); + + // Restart: offset resets, next poll begins from zero. + v.start(); + assert_eq!( + v.poll(&payload), + Ok(VerifyStep::Processing { + hashed: 4, + total: 12 + }) + ); + } + + #[test] + fn poll_before_start_is_an_error() { + let payload = SlicePayload(&[0xFF; 4]); + let mut v = ChunkedVerifier::new(); + assert!(v.poll(&payload).is_err()); + } + + // A PayloadSource whose read_at always fails. + struct Lying; + + impl PayloadSource for Lying { + fn len(&self) -> u64 { + 64 + } + + fn read_at(&self, _offset: u64, _buf: &mut [u8]) -> Result<(), PayloadReadError> { + Err(PayloadReadError::Storage) + } + } + + #[test] + fn read_fault_is_err_not_verdict() { + let mut v = ChunkedVerifier::new(); + v.start(); + + // The verifier tries to read, fails, and returns Err (not Rejected). + let result = v.poll(&Lying); + assert!(result.is_err(), "payload fault must be Err, not a verdict"); + } + + #[test] + fn poll_after_verdict_is_an_error() { + let payload = SlicePayload(&[0xFF; 4]); + let mut v = ChunkedVerifier::new(); + v.start(); + + // Drain to verdict. + let _ = v.poll(&payload); + let _ = v.poll(&payload); + + // Post-verdict poll is a fault. + assert!(v.poll(&payload).is_err()); + } +} diff --git a/services/orchestrator/capabilities/src/lib.rs b/services/orchestrator/capabilities/src/lib.rs index a7a479f1e..f738968d9 100644 --- a/services/orchestrator/capabilities/src/lib.rs +++ b/services/orchestrator/capabilities/src/lib.rs @@ -22,6 +22,10 @@ //! the chunked read seam staging pulls from — transports and slot //! bookkeeping stay behind the adapter. //! +//! `IncrementalVerifier` is the polled verification seam: judge a candidate +//! image one bounded step at a time, so the update pump never blocks on +//! hashing megabytes of payload. Same poll-not-block contract as `Updatable`. +//! //! `BootWatch` is the seam the orchestrator polls: one device's boot walk, //! erased of every device-specific type, answering with a `WalkVerdict`. //! @@ -41,6 +45,7 @@ mod boot_control; mod boot_watch; mod evidence; +mod incremental_verifier; mod lockdown_latch; mod svn_floor; mod updatable; @@ -48,6 +53,7 @@ mod updatable; pub use boot_control::BootControl; pub use boot_watch::{BootWatch, FailureCause, WalkVerdict}; pub use evidence::{BootStatus, EvidenceReader}; +pub use incremental_verifier::{IncrementalVerifier, VerifyStep}; pub use lockdown_latch::LockdownLatch; pub use svn_floor::{Svn, SvnFloor}; pub use updatable::{PayloadReadError, PayloadSource, StageProgress, Updatable, UpdateError};