diff --git a/cmd/probe/src/lib.rs b/cmd/probe/src/lib.rs index bfe42da2c..72ce0dcf2 100644 --- a/cmd/probe/src/lib.rs +++ b/cmd/probe/src/lib.rs @@ -319,7 +319,7 @@ fn probecmd(subargs: ProbeArgs, context: &mut ExecutionContext) -> Result<()> { } let regions = if let Some(hubris) = &hubris { - match hubris.validate(core, HubrisValidate::ArchiveMatch) { + match hubris.validate(core, HubrisValidate::ArchiveMatch, log) { Ok(_) => hubris.regions(core).unwrap(), Err(err) => { // diff --git a/cmd/readmem/src/lib.rs b/cmd/readmem/src/lib.rs index db60b88cb..eee627697 100644 --- a/cmd/readmem/src/lib.rs +++ b/cmd/readmem/src/lib.rs @@ -188,7 +188,7 @@ fn readmem(subargs: ReadmemArgs, context: &mut ExecutionContext) -> Result<()> { if subargs.symbol { if let Some(hubris) = &hubris { - hubris.validate(core, HubrisValidate::ArchiveMatch)?; + hubris.validate(core, HubrisValidate::ArchiveMatch, log)?; } else { bail!("cannot specify `--symbol` without Hubris archive"); } @@ -198,7 +198,7 @@ fn readmem(subargs: ReadmemArgs, context: &mut ExecutionContext) -> Result<()> { Ok(addr) => addr, _ => { if let Some(hubris) = &hubris { - hubris.validate(core, HubrisValidate::ArchiveMatch)?; + hubris.validate(core, HubrisValidate::ArchiveMatch, log)?; hubris.lookup_peripheral(&subargs.address)? } else { bail!("cannot look up peripheral without archive"); diff --git a/cmd/test/src/lib.rs b/cmd/test/src/lib.rs index 9ff4001b3..eed63460f 100644 --- a/cmd/test/src/lib.rs +++ b/cmd/test/src/lib.rs @@ -165,7 +165,7 @@ fn test(subargs: TestArgs, context: &mut ExecutionContext) -> Result<()> { let log = context.log(); let core = &mut *context.cli.attach_live_booted(hubris)?; - hubris.validate(core, HubrisValidate::Booted)?; + hubris.validate(core, HubrisValidate::Booted, log)?; // This type is &[(&str, &(dyn Fn() + Send + Sync))] let test_slice = hubris diff --git a/humility-cli/src/lib.rs b/humility-cli/src/lib.rs index 4e22e8ca2..773271c01 100644 --- a/humility-cli/src/lib.rs +++ b/humility-cli/src/lib.rs @@ -193,7 +193,7 @@ impl Cli { let Some(hubris) = hubris else { bail!("cannot validate without Hubris archive"); }; - hubris.validate(&mut *core, validate)?; + hubris.validate(&mut *core, validate, self.log())?; } Ok(core) } @@ -269,7 +269,7 @@ impl Cli { let Some(hubris) = hubris else { bail!("cannot validate without Hubris archive"); }; - hubris.validate(&mut *core, validate)?; + hubris.validate(&mut *core, validate, self.log())?; } Ok(core) } diff --git a/humility-core/src/hubris.rs b/humility-core/src/hubris.rs index e894965fc..7ef8156f1 100644 --- a/humility-core/src/hubris.rs +++ b/humility-core/src/hubris.rs @@ -2071,36 +2071,39 @@ impl HubrisArchive { &self, core: &mut dyn crate::core::Core, criteria: HubrisValidate, + log: &Logger, ) -> Result<()> { - let ntasks = self.ntasks(); if core.is_net() || core.is_archive() { return Ok(()); } - // To validate that what we're running on the target matches what - // we have in the archive, we are going to check the image ID, an - // identifer created for this purpose. - let addr = self.imageid.0; - let nbytes = self.imageid.1.len(); - assert!(nbytes > 0); - - let mut id = vec![0; nbytes]; - core.read_8(addr, &mut id[0..nbytes]).with_context(|| { - format!("failed to read image ID at 0x{:x}; board mismatch?", addr) - })?; - - let deltas = id - .iter() - .zip(self.imageid.1.iter()) - .filter(|&(lhs, rhs)| lhs != rhs) - .count(); + // To validate that what we're running on the target matches what we + // have in the archive, we are going to check the image ID, an identifer + // created for this purpose. First try to read the ID of the image that + // actually booted, as recorded by the kernel. If that fails then fall + // back to reading the ID from FLASH, which can be misleading on targets + // with A/B images. For example, if the archive is an A image, then + // `read_image_id_from_flash` will return the ID of whichever A image + // was last flashed regardless of whether the target is currently + // running an A or B image. + let id = match self.booted_image(core) { + Ok((id, _)) => id, + Err(err) => { + info!( + log, + "can't detect booted image, reading image ID from FLASH ({})", + err + ); + self.read_image_id_from_flash(core)? + } + }; - if deltas > 0 || id.len() != self.imageid.1.len() { + if id != self.imageid.1 { bail!( "image ID in archive ({:x?}) does not equal \ - ID at 0x{:x} ({:x?})", - self.imageid.1, self.imageid.0, id, - ); + image ID of target ({:x?})", + self.imageid.1, id, + ); } if criteria == HubrisValidate::ArchiveMatch { @@ -2111,6 +2114,7 @@ impl HubrisArchive { return Ok(()); } + let ntasks = self.ntasks(); let (_, n) = self.task_table(core)?; if n == ntasks as u32 { @@ -2170,6 +2174,67 @@ impl HubrisArchive { ); } + /// Reads the kernel's `BOOTED_IMAGE` array to determine which image is + /// actually running on the target. + /// + /// Returns the image ID and the image name. + pub fn booted_image( + &self, + core: &mut dyn crate::core::Core, + ) -> Result<(Vec, String)> { + const MAGIC: &[u8] = b"HUBRISID"; + const ID_LEN: usize = 8; + + // If BOOTED_IMAGE isn't in the symbol table, it's probably because the + // archive is too old. + let &(addr, size) = + self.esyms_byname.get("BOOTED_IMAGE").ok_or_else(|| { + anyhow!("BOOTED_IMAGE not in archive's symbol table") + })?; + + let size = size as usize; + if size < ID_LEN + MAGIC.len() { + // Even if the name was empty, the fixed length parts wouldn't fit. + // Don't try to interpret it. + return Err(anyhow!("BOOTED_IMAGE array is unexpectedly short")); + } + + let mut block = vec![0u8; size]; + core.read_8(addr, &mut block).with_context(|| { + format!("failed to read BOOTED_IMAGE at 0x{:x}", addr) + })?; + + let id = &block[..ID_LEN]; + // We don't know the length of the name, but we know the lengths of the + // stuff on either side. + let name = &block[ID_LEN..size - MAGIC.len()]; + let magic = &block[size - MAGIC.len()..]; + if magic != MAGIC { + return Err( + anyhow!("BOOTED_IMAGE array does not contain magic marker"), + ); + } + let name_string = + String::from_utf8_lossy(name).trim_end_matches('\0').to_string(); + + Ok((id.to_vec(), name_string)) + } + + pub fn read_image_id_from_flash( + &self, + core: &mut dyn crate::core::Core, + ) -> Result> { + let addr = self.imageid.0; + let nbytes = self.imageid.1.len(); + assert!(nbytes > 0); + + let mut id = vec![0; nbytes]; + core.read_8(addr, &mut id[0..nbytes]).with_context(|| { + format!("failed to read image ID at 0x{:x}; board mismatch?", addr) + })?; + Ok(id) + } + pub fn verify( &self, core: &mut dyn crate::core::Core, diff --git a/humility-flash/src/lib.rs b/humility-flash/src/lib.rs index 8199b434c..7b6b55376 100644 --- a/humility-flash/src/lib.rs +++ b/humility-flash/src/lib.rs @@ -5,10 +5,12 @@ //! Functions to check flash state and reprogram a processor #![warn(missing_docs)] +use anyhow::anyhow; + use humility::{ core::Core, - hubris::{HubrisArchive, HubrisValidate}, - log::{Logger, info}, + hubris::HubrisArchive, + log::{Logger, info, warn}, }; use humility_auxflash::{AuxFlashHandler, AuxFlashWriter}; use humility_probes_core::ProbeCore; @@ -92,11 +94,45 @@ pub fn get_image_state( ) -> Result { core.halt().map_err(ImageStateError::HaltFailed)?; - // First pass: check only the image ID - if let Err(e) = hubris.validate(core, HubrisValidate::ArchiveMatch) { - return Ok(ImageStateResult::DoesNotMatch( - ImageStateMismatch::FlashArchiveMismatch(e), - )); + // Warn if we're sure that the user is flashing an archive for a different + // image slot than the one currently running (e.g. flashing an A image onto + // an RoT that's running a B image). This isn't necessarily a problem but it + // means that the newly flashed image won't actually run after the target + // resets. If the user does it by accident, they could get very confused. + if let Some(archive_image_name) = hubris.manifest.image.as_deref() + && let Ok((_, name)) = hubris.booted_image(core) + && name != archive_image_name + { + warn!( + log, + "the archive contains image '{archive_image_name}' but the target \ + is running image '{name}'; image '{archive_image_name}' won't run \ + unless the boot preference is changed" + ); + } + + // First pass: check only the image ID. + // + // We specifically care about the ID of the image stored in the FLASH slot + // that we're planning to overwrite, not the ID of the image that's + // currently running. + match hubris.read_image_id_from_flash(core) { + Err(e) => { + return Ok(ImageStateResult::DoesNotMatch( + ImageStateMismatch::FlashArchiveMismatch(e), + )); + } + Ok(id) if id != hubris.image_id() => { + return Ok(ImageStateResult::DoesNotMatch( + ImageStateMismatch::FlashArchiveMismatch(anyhow!( + "image ID in archive ({:x?}) does not equal \ + image ID of target ({:x?})", + hubris.image_id(), + id, + )), + )); + } + _ => (), } // More rigorous checks if requested diff --git a/humility-probes-core/src/lib.rs b/humility-probes-core/src/lib.rs index 0c2917807..d06460693 100644 --- a/humility-probes-core/src/lib.rs +++ b/humility-probes-core/src/lib.rs @@ -300,6 +300,7 @@ impl HubrisAttach for humility::hubris::HubrisArchive { self.validate( &mut core, humility::hubris::HubrisValidate::ArchiveMatch, + log, )?; Ok(core)