-
Notifications
You must be signed in to change notification settings - Fork 64
Attempt a basic PC range check as part of hubris.validate
#711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 5 commits
a716c34
015a3a3
9d708f0
4f99d53
02df730
0dd349c
2635ae7
1bab5bc
c01d91c
cfa9fe2
b683227
787b3a1
3d1e66b
220886a
e6726f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2080,34 +2080,27 @@ impl HubrisArchive { | |
| // 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(); | ||
|
|
||
| if deltas > 0 || id.len() != self.imageid.1.len() { | ||
| let id = self.read_image_id_from_flash(core)?; | ||
| 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, | ||
| ); | ||
| } | ||
|
|
||
| if criteria == HubrisValidate::ArchiveMatch { | ||
| if self.task_dump().is_some() { | ||
| return Ok(()); | ||
| } else { | ||
| core.halt()?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely yes this got lost in some refactoring 😬 |
||
| if let Ok((false, pc)) = self.is_pc_within_archive(core) { | ||
| bail!("image ID matches but PC at 0x{pc:x} is not part of any \ | ||
| module. This is likely an incorrect A/B archive."); | ||
| } | ||
| core.run()?; | ||
| } | ||
|
|
||
| if self.task_dump().is_some() { | ||
| if criteria == HubrisValidate::ArchiveMatch { | ||
| return Ok(()); | ||
| } | ||
|
|
||
|
|
@@ -2170,6 +2163,34 @@ impl HubrisArchive { | |
| ); | ||
| } | ||
|
|
||
| pub fn read_image_id_from_flash( | ||
| &self, | ||
| core: &mut dyn crate::core::Core, | ||
| ) -> Result<Vec<u8>> { | ||
| 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) | ||
| } | ||
|
|
||
| /// The core must be halted before this is called | ||
| pub fn is_pc_within_archive( | ||
| &self, | ||
| core: &mut dyn crate::core::Core | ||
| ) -> Result<(bool, u32)> { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite follow this return type. Can you explain it with a comment?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Yeah it's kinda weird. I wanted a helper function for doing the check, but also didn't want to have to remove the PC value from your error message. It will be useful for debugging when this error gets triggered by some other strange situation someday.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to change it now but it's also worth considering something like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like that, changed. |
||
| let pc = core.read_reg(ARMRegister::PC)?; | ||
| if self.instr_mod(pc).is_none() { | ||
| Ok((false, pc)) | ||
| } else { | ||
| Ok((true, pc)) | ||
| } | ||
| } | ||
|
|
||
| pub fn verify( | ||
| &self, | ||
| core: &mut dyn crate::core::Core, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,11 @@ | |
| //! 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 +93,30 @@ pub fn get_image_state( | |
| ) -> Result<ImageStateResult, ImageStateError> { | ||
| core.halt().map_err(ImageStateError::HaltFailed)?; | ||
|
|
||
| if let Ok((false, pc)) = hubris.is_pc_within_archive(core) { | ||
| warn!(log, "PC at 0x{pc:x} is not part of any module. Maybe you're \ | ||
| flashing an A image while a B image is running, or vice \ | ||
| versa?"); | ||
| } | ||
|
Comment on lines
+107
to
+115
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we drop the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah my thought was for it to just log nothing unless it's sure the PC location is bad. Would you prefer to have a "failed to check PC location" warning too? |
||
|
|
||
| // First pass: check only the image ID | ||
| if let Err(e) = hubris.validate(core, HubrisValidate::ArchiveMatch) { | ||
| return Ok(ImageStateResult::DoesNotMatch( | ||
| ImageStateMismatch::FlashArchiveMismatch(e), | ||
| )); | ||
| 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, | ||
| )), | ||
| )); | ||
| } | ||
|
Comment on lines
+119
to
+131
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these should be two separate error types: one to represent an error reading the image ID and a separate one to indicate an actual mismatch. This also means we can avoid having to create an extra error message with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. That's a nice side effect of not calling |
||
| _ => (), | ||
| } | ||
|
|
||
| // More rigorous checks if requested | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@labbott What does it mean for an archive to be "a dump from a single task"? I'm wondering if it's intentional/correct that if you pass in
HubrisValidate::Bootedin that case, it returns early here and skips the boot check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to take a dump from only a single task's memory range (e.g. a "single task dump"), but not an "entire system" dump. Laura can confirm though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
James is correct. We have the notion of single task dumps vs a wholes system dump. You can run many of the same commands like e.g.
humility tasksandhumility ringbufand things should work as expected. I don't think we capture the boot information in a single task dump which is why it gets skipped.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added a comment about it.