diff --git a/Cargo.lock b/Cargo.lock index 3fb0fa3d22..8f3201048e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3863,9 +3863,9 @@ dependencies = [ [[package]] name = "minicbor" -version = "0.26.4" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acb9d59e79ad66121ab441a0d1950890906a41e01ae14145ecf8401aa8894f50" +checksum = "4f182275033b808ede9427884caa8e05fa7db930801759524ca7925bd8aa7a82" [[package]] name = "minicbor-derive" @@ -3878,6 +3878,14 @@ dependencies = [ "syn 2.0.98", ] +[[package]] +name = "minicbor-lease" +version = "0.1.0" +dependencies = [ + "idol-runtime", + "minicbor 2.1.1", +] + [[package]] name = "minicbor-serde" version = "0.3.2" @@ -5038,8 +5046,7 @@ dependencies = [ [[package]] name = "ssmarshal" version = "1.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f3e6ad23b128192ed337dfa4f1b8099ced0c2bf30d61e551b65fda5916dbb850" +source = "git+https://gitlab.com/de-vri-es/ssmarshal?rev=10e90cbe389c8f07c52815857551a051946881ab#10e90cbe389c8f07c52815857551a051946881ab" dependencies = [ "encode_unicode", "serde", @@ -5391,7 +5398,7 @@ dependencies = [ "counters", "idol", "idol-runtime", - "minicbor 0.26.4", + "minicbor 2.1.1", "num-traits", "ringbuf", "task-packrat-api", @@ -5730,7 +5737,8 @@ dependencies = [ "hubris-task-names", "idol", "idol-runtime", - "minicbor 0.26.4", + "minicbor 2.1.1", + "minicbor-lease", "mutable-statics", "num-traits", "quote", diff --git a/Cargo.toml b/Cargo.toml index de44778dd5..06bc6f7bff 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -92,7 +92,7 @@ leb128 = { version = "0.2.5", default-features = false } lpc55-pac = { version = "0.4", default-features = false } memchr = { version = "2.4", default-features = false } memoffset = { version = "0.6.5", default-features = false } -minicbor = { version = "0.26.4", default-features = false } +minicbor = { version = "2.1.1", default-features = false } multimap = { version = "0.8.3", default-features = false } nb = { version = "1", default-features = false } num = { version = "0.4", default-features = false } @@ -163,6 +163,10 @@ tlvc-text = { git = "https://github.com/oxidecomputer/tlvc", default-features = transceiver-messages = { git = "https://github.com/oxidecomputer/transceiver-control/", default-features = false } vsc7448-pac = { git = "https://github.com/oxidecomputer/vsc7448", default-features = false } +[patch.crates-io] +# See https://gitlab.com/robigalia/ssmarshal/-/merge_requests/2 +ssmarshal = { git = "https://gitlab.com/de-vri-es/ssmarshal", rev = "10e90cbe389c8f07c52815857551a051946881ab" } + [workspace.lints.rust] elided_lifetimes_in_paths = "warn" diff --git a/lib/minicbor-lease/Cargo.toml b/lib/minicbor-lease/Cargo.toml new file mode 100644 index 0000000000..8fe3f75535 --- /dev/null +++ b/lib/minicbor-lease/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "minicbor-lease" +version = "0.1.0" +edition = "2024" + +[dependencies] +minicbor = { workspace = true } +idol-runtime = { workspace = true } + +[lib] +test = false +doctest = false +bench = false + +[lints] +workspace = true diff --git a/lib/minicbor-lease/src/lib.rs b/lib/minicbor-lease/src/lib.rs new file mode 100644 index 0000000000..636a472642 --- /dev/null +++ b/lib/minicbor-lease/src/lib.rs @@ -0,0 +1,109 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! An adapter implementing [`minicbor::encode::write::Write`] for +//! [`idol_runtime::Leased`] byte buffers. + +#![no_std] + +/// An adapter implementing [`minicbor::encode::write::Write`] for +/// [`idol_runtime::Leased`] byte buffers. +pub struct LeasedWriter<'lease, A> +where + A: idol_runtime::AttributeWrite, +{ + lease: &'lease mut idol_runtime::Leased, + pos: usize, +} + +/// Errors returned by the [`minicbor::encode::write::Write`] implementation for +/// [`LeasedWriter`]. +#[derive(Copy, Clone, PartialEq, Debug)] +pub enum Error { + /// The other side of the lease has gone away. + WentAway, + /// Data could not be written as there was no room left in the lease. + EndOfLease, +} + +impl core::fmt::Display for Error { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + f.write_str(match self { + Self::WentAway => "lease went away", + Self::EndOfLease => "end of lease", + }) + } +} + +impl minicbor::encode::write::Write for LeasedWriter<'_, A> +where + A: idol_runtime::AttributeWrite, +{ + type Error = Error; + + fn write_all(&mut self, buf: &[u8]) -> Result<(), Self::Error> { + let Some(end) = self.pos.checked_add(buf.len()) else { + return Err(Error::EndOfLease); + }; + if end >= self.lease.len() { + return Err(Error::EndOfLease); + } + self.lease + .write_range(self.pos..end, buf) + .map_err(|_| Error::WentAway)?; + + self.pos += buf.len(); + + Ok(()) + } +} + +impl<'lease, A> LeasedWriter<'lease, A> +where + A: idol_runtime::AttributeWrite, +{ + /// Returns a new `LeasedWriter` starting at byte 0 of the lease. + pub fn new(lease: &'lease mut idol_runtime::Leased) -> Self { + Self { lease, pos: 0 } + } + + /// Returns a new `LeasedWriter` starting at the specified byte position in + /// the lease. + /// + /// This is intended for cases where some data has already been written to + /// the lease. + pub fn starting_at( + position: usize, + lease: &'lease mut idol_runtime::Leased, + ) -> Self { + Self { + lease, + pos: position, + } + } + + /// Returns the current byte position within the lease. + pub fn position(&self) -> usize { + self.pos + } + + /// Borrows the underlying lease from the writer. + pub fn lease(&self) -> &idol_runtime::Leased { + self.lease + } + + /// Returns the underlying lease, consuming the writer. + pub fn into_inner(self) -> &'lease mut idol_runtime::Leased { + self.lease + } +} + +impl From for idol_runtime::ClientError { + fn from(error: Error) -> Self { + match error { + Error::EndOfLease => idol_runtime::ClientError::BadLease, + Error::WentAway => idol_runtime::ClientError::WentAway, + } + } +} diff --git a/task/packrat/Cargo.toml b/task/packrat/Cargo.toml index 41556efce9..9420ce6040 100644 --- a/task/packrat/Cargo.toml +++ b/task/packrat/Cargo.toml @@ -23,6 +23,7 @@ task-packrat-api = { path = "../packrat-api" } userlib = { path = "../../sys/userlib", features = ["panic-messages"] } snitch-core = { version = "0.1.0", path = "../../lib/snitch-core", optional = true, features = ["counters"] } minicbor = { workspace = true, optional = true } +minicbor-lease = { path = "../../lib/minicbor-lease", optional = true } [build-dependencies] anyhow.workspace = true @@ -37,7 +38,7 @@ gimlet = ["drv-cpu-seq-api"] grapefruit = [] boot-kmdb = [] no-ipc-counters = ["idol/no-counters"] -ereport = ["dep:drv-rng-api", "dep:snitch-core", "dep:minicbor", "dep:quote"] +ereport = ["dep:drv-rng-api", "dep:snitch-core", "dep:minicbor", "dep:minicbor-lease", "dep:quote"] # This section is here to discourage RLS/rust-analyzer from doing test builds, # since test builds don't work for cross compilation. diff --git a/task/packrat/src/ereport.rs b/task/packrat/src/ereport.rs index 48b7431acb..2e96c85ad9 100644 --- a/task/packrat/src/ereport.rs +++ b/task/packrat/src/ereport.rs @@ -17,6 +17,7 @@ use super::ereport_messages; use core::convert::Infallible; use idol_runtime::{ClientError, Leased, LenLimit, RequestError}; use minicbor::CborLen; +use minicbor_lease::LeasedWriter; use ringbuf::{counted_ringbuf, ringbuf_entry}; use task_packrat_api::{EreportReadError, VpdIdentity}; use userlib::{kipc, sys_get_timer, RecvMessage, TaskId}; @@ -78,7 +79,6 @@ enum Trace { #[derive(Copy, Clone, PartialEq, Eq, counters::Count)] enum MetadataError { - TooLong, PartNumberNotUtf8, SerialNumberNotUtf8, } @@ -86,7 +86,6 @@ enum MetadataError { #[derive(Copy, Clone, PartialEq, Eq, counters::Count)] enum EreportError { TaskIdOutOfRange, - TooLong, } counted_ringbuf!(Trace, 16, Trace::None); @@ -143,41 +142,46 @@ impl EreportStore { begin_ena: ereport_messages::Ena, limit: u8, committed_ena: ereport_messages::Ena, - data: Leased, + mut data: Leased, vpd: Option<&VpdIdentity>, ) -> Result> { ringbuf_entry!(Trace::ReadRequest { restart_id: restart_id.into() }); - // XXX(eliza): in theory it might be nicer to use - // `minicbor::data::Token::{BeginArray, BeginMap, Break}` for these, but - // it's way more annoying in practice, because you need to have an - // `Encoder` and can't just put it in the buffer. - /// Byte indicating the beginning of an indeterminate-length CBOR - /// array. - const CBOR_BEGIN_ARRAY: u8 = 0x9f; - /// Byte indicating the beginning of an indeterminate-length CBOR map. - const CBOR_BEGIN_MAP: u8 = 0xbf; - /// Byte indicating the end of an indeterminate-length CBOR array or - /// map. - const CBOR_BREAK: u8 = 0xff; - let current_restart_id = self.restart_id.ok_or(EreportReadError::RestartIdNotSet)?; // Skip over a header-sized initial chunk. let first_data_byte = size_of::(); - - let mut position = first_data_byte; let mut first_written_ena = None; + let mut encoder = minicbor::Encoder::new(LeasedWriter::starting_at( + first_data_byte, + &mut data, + )); + + fn handle_encode_err( + err: minicbor::encode::Error, + ) -> RequestError { + // These should always be write errors; everything we write should + // always encode successfully. + match err.into_write() { + Some(e) => ClientError::from(e).fail(), + // This really shouldn't ever happen: an error that didn't come + // from the underlying lease writer means that we couldn't + // encode the ereport as CBOR. Since the structure of these list + // entries is always the same, they really had better always be + // well-formed CBOR. But, since Packrat is never supposed to + // panic, let's just kill the client instead of us. + None => ClientError::BadMessageContents.fail(), + } + } + // Start the metadata map. // - // MGS expects us to always include this, and to just have it be + // MGS expects us to always include this, and to just have it be // empty if we didn't send any metadata. - data.write_at(position, CBOR_BEGIN_MAP) - .map_err(|_| ClientError::WentAway.fail())?; - position += 1; + encoder.begin_map().map_err(handle_encode_err)?; // If the requested restart ID matches the current restart ID, then read // from the requested ENA. If not, start at ENA 0. @@ -204,36 +208,23 @@ impl EreportStore { // ensures that MGS will continue asking for metadata on subsequent // requests. if let Some(vpd) = vpd { - // Encode the metadata map into our buffer. - match self.encode_metadata(vpd) { - Ok(encoded) => { - data.write_range( - position..position + encoded.len(), - encoded, - ) - .map_err(|_| ClientError::WentAway.fail())?; - - position += encoded.len(); - - ringbuf_entry!(Trace::MetadataEncoded { - len: encoded.len() as u32 - }); - } - Err(err) => { - // Encoded VPD metadata was too long, or couldn't be - // represented as a CBOR string. - ringbuf_entry!(Trace::MetadataError(err)); - } - } + // Encode the metadata map. + self.encode_metadata(&mut encoder, vpd) + .map_err(handle_encode_err)?; + ringbuf_entry!(Trace::MetadataEncoded { + len: encoder + .writer() + .position() + .saturating_sub(first_data_byte) + as u32, + }); } // Begin at ENA 0 0 }; // End metadata map. - data.write_at(position, CBOR_BREAK) - .map_err(|_| ClientError::WentAway.fail())?; - position += 1; + encoder.end().map_err(handle_encode_err)?; let mut reports = 0; // Beginning with the first @@ -242,14 +233,6 @@ impl EreportStore { break; } - if first_written_ena.is_none() { - first_written_ena = Some(r.ena); - // Start the ereport array - data.write_at(position, CBOR_BEGIN_ARRAY) - .map_err(|_| ClientError::WentAway.fail())?; - position += 1; - } - let tid = TaskId(r.tid); let task_name = hubris_task_names::TASK_NAMES .get(tid.index()) @@ -272,41 +255,38 @@ impl EreportStore { r.timestamp, ByteGather(r.slices.0, r.slices.1), ); - let mut c = - minicbor::encode::write::Cursor::new(&mut self.recv[..]); - match minicbor::encode(&entry, &mut c) { - Ok(()) => { - let size = c.position(); - // If there's no room left for this one in the lease, we're - // done here. Note that the use of `>=` rather than `>` is - // intentional, as we want to ensure that there's room for - // the final `CBOR_BREAK` byte that ends the CBOR array. - if position + size >= data.len() { - break; - } - data.write_range( - position..position + size, - &self.recv[..size], - ) - .map_err(|_| ClientError::WentAway.fail())?; - position += size; - reports += 1; - } - Err(_end) => { - // This is an odd one; we've admitted a record into our - // queue that won't fit in our buffer. This can happen - // because of the encoding overhead, in theory, but should - // be prevented. - ringbuf_entry!(Trace::EreportError(EreportError::TooLong)); - } + + // If there's no room left for this one in the lease, we're + // done here. Note that the use of `>=` rather than `>` is + // intentional, as we want to ensure that there's room for + // the final `CBOR_BREAK` byte that ends the CBOR array. + // + // N.B.: saturating_add is used to avoid panic sites --- if this + // adds up to `u32::MAX` it *definitely* won't fit :) + let len_needed = encoder + .writer() + .position() + // Of course, we need enough space for the ereport itself... + .saturating_add(minicbor::len(&entry)) + // In addition, if we haven't yet started the CBOR array of + // ereports, we need a byte for that too. + .saturating_add(first_written_ena.is_none() as usize); + if len_needed >= encoder.writer().lease().len() { + break; + } + + if first_written_ena.is_none() { + first_written_ena = Some(r.ena); + // Start the ereport array + encoder.begin_array().map_err(handle_encode_err)?; } + encoder.encode(&entry).map_err(handle_encode_err)?; + reports += 1; } if let Some(start_ena) = first_written_ena { // End CBOR array, if we wrote anything. - data.write_at(position, CBOR_BREAK) - .map_err(|_| ClientError::WentAway.fail())?; - position += 1; + encoder.end().map_err(handle_encode_err)?; ringbuf_entry!(Trace::Reported { start_ena, @@ -315,6 +295,8 @@ impl EreportStore { }); } + // Release the mutable borrow on the lease so we can write the header. + let end = encoder.into_writer().position(); let first_ena = first_written_ena.unwrap_or(0); let header = ereport_messages::ResponseHeader::V0( ereport_messages::ResponseHeaderV0 { @@ -325,18 +307,14 @@ impl EreportStore { ); data.write_range(0..size_of_val(&header), header.as_bytes()) .map_err(|_| ClientError::WentAway.fail())?; - Ok(position) + Ok(end) } fn encode_metadata( - &mut self, + &self, + encoder: &mut minicbor::Encoder>, vpd: &VpdIdentity, - ) -> Result<&[u8], MetadataError> { - let c = minicbor::encode::write::Cursor::new(&mut self.recv[..]); - let mut encoder = minicbor::Encoder::new(c); - // TODO(eliza): presently, this code bails out if the metadata map gets - // longer than our buffer. It would be nice to have a way to keep the - // encoded metadata up to the last complete key-value pair... + ) -> Result<(), minicbor::encode::Error> { encoder .str("hubris_archive_id")? .bytes(&self.image_id[..])?; @@ -357,8 +335,7 @@ impl EreportStore { )), } encoder.str("baseboard_rev")?.u32(vpd.revision)?; - let size = encoder.into_writer().position(); - Ok(&self.recv[..size]) + Ok(()) } } @@ -397,16 +374,6 @@ impl CborLen for ByteGather<'_, '_> { } } -impl From> - for MetadataError -{ - fn from( - _: minicbor::encode::Error, - ) -> MetadataError { - MetadataError::TooLong - } -} - mod config { include!(concat!(env!("OUT_DIR"), "/ereport_config.rs")); }