Attempt a basic PC range check as part of hubris.validate#711
Open
labbott wants to merge 1 commit into
Open
Conversation
`validate` on a hubris archive will verify the `image_id` present in an archive matches what's in flash. This has an unfortunate failure mode for the RoT where we partition the flash manually. It's possible to have the image ID check succeed on the archive but actually be running the opposite image. This causes all kinds of confusion. Add a basic spot check that the PC is in at least one module range. This could cause potential failures if the PC is actually running in a range that does not correspond to a module. Based on other work, the chances of this happening are very limited and should be transient.
Contributor
Author
|
hawkw
approved these changes
Jul 7, 2026
Comment on lines
+2110
to
+2115
| if let Ok(pc) = core.read_reg(ARMRegister::PC) { | ||
| if self.instr_mod(pc).is_none() { | ||
| bail!("PC at 0x{pc:x} is not part of any module. This is \ | ||
| likely an incorrect archive."); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
i believe clippy would like you to have written something more like:
Suggested change
| if let Ok(pc) = core.read_reg(ARMRegister::PC) { | |
| if self.instr_mod(pc).is_none() { | |
| bail!("PC at 0x{pc:x} is not part of any module. This is \ | |
| likely an incorrect archive."); | |
| } | |
| } | |
| if let Ok(pc) = core.read_reg(ARMRegister::PC) | |
| && self.instr_mod(pc).is_none() { | |
| bail!("PC at 0x{pc:x} is not part of any module. \ | |
| This is likely an incorrect archive."); | |
| } |
| core.halt()?; | ||
| if let Ok(pc) = core.read_reg(ARMRegister::PC) { | ||
| if self.instr_mod(pc).is_none() { | ||
| bail!("PC at 0x{pc:x} is not part of any module. This is \ |
Member
There was a problem hiding this comment.
unimportant turbo nitpick: if we write
Suggested change
| bail!("PC at 0x{pc:x} is not part of any module. This is \ | |
| bail!("PC at {pc:#x} is not part of any module. This is \ |
that tells the formatter to emit the 0x prefix itself. which...changes absolutely nothing here so it doesn't matter...
evan-oxide
reviewed
Jul 7, 2026
| core.halt()?; | ||
| if let Ok(pc) = core.read_reg(ARMRegister::PC) { | ||
| if self.instr_mod(pc).is_none() { | ||
| bail!("PC at 0x{pc:x} is not part of any module. This is \ |
Contributor
There was a problem hiding this comment.
I think the way this interacts with flashing is not ideal. If you try to re-flash an A image that's already been flashed to the A slot while the RoT is running a B image:
- the old behavior was to fail with "archive appears to be already flashed on attached device; use -F ("--force") to force re-flash"
- this PR's behavior is to re-flash the A image
- the ideal behavior is probably to warn the user that they probably meant to flash the other image, then fail with the "archive appears to be already flashed" error.
Contributor
Author
There was a problem hiding this comment.
yeah you're right that's confusing behavior. I think that would point to needing to fix validate to return a more specific error
Contributor
There was a problem hiding this comment.
Or don't use validate for the pre-flash check. In my PR, I considered a bunch of different return types then gave up trying to make the same validate function work for flashing and for both of the HubrisValidate options.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
validateon a hubris archive will verify theimage_idpresent in an archive matches what's in flash. This has an unfortunate failure mode for the RoT where we partition the flash manually. It's possible to have the image ID check succeed on the archive but actually be running the opposite image. This causes all kinds of confusion. Add a basic spot check that the PC is in at least one module range. This could cause potential failures if the PC is actually running in a range that does not correspond to a module. Based on other work, the chances of this happening are very limited and should be transient.