Attempt a basic PC range check as part of `hubris.validate` by labbott · Pull Request #711 · oxidecomputer/humility

labbott · 2026-07-07T14:50:30Z

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.

`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.

labbott · 2026-07-07T14:50:45Z

labbott@labbott-desktop2:~/humility$ ./target/debug/humility -a ~/hubris/target/oxide-rot-1-selfsigned/dist/a/build-oxide-rot-1-selfsigned-image-a.zip tasks
humility: attached via 1fc9:143:Some("U2VLC0GBANWF1") MCU-LINK (r0FB) CMSIS-DAP V3.172
system time = 200385
ID TASK                       GEN PRI STATE    
 0 jefe                         0   0 recv, notif: fault timer(T+15)
 1 hiffy                        0   6 notif: timer(T+18)
 2 idle                         0   7 RUNNING
 3 update_server                0   3 recv
 4 syscon_driver                0   2 recv
 5 gpio_driver                  0   3 recv
 6 sprot                        0   6 notif: spi-irq(irq59)
 7 swd                          0   4 recv, notif: timer sp_reset-irq(irq4) jtag_detect-irq(irq5)
 8 dumper                       0   5 recv
 9 attest                       0   3 recv
labbott@labbott-desktop2:~/humility$ ./target/debug/humility -a ~/hubris/target/oxide-rot-1-selfsigned/dist/b/build-oxide-rot-1-selfsigned-image-b.zip tasks
humility: attached via 1fc9:143:Some("U2VLC0GBANWF1") MCU-LINK (r0FB) CMSIS-DAP V3.172
humility tasks failed: PC at 0x1ef34 is not part of any module. This is likely an incorrect archive.

hawkw · 2026-07-07T17:09:35Z

+            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.");
+                }
+            }


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.");

}

hawkw · 2026-07-07T17:10:29Z

+            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 \


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 · 2026-07-07T19:06:50Z

+            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 \


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.

yeah you're right that's confusing behavior. I think that would point to needing to fix validate to return a more specific error

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.

evan-oxide · 2026-07-08T17:38:04Z

+        if self.task_dump().is_some() {
            return Ok(());
+        } else {
+            core.halt()?;


Should we do core.run()? again after checking the PC?

Absolutely yes this got lost in some refactoring 😬

labbott

Thanks for doing this

labbott · 2026-07-09T13:41:42Z

+    pub fn is_pc_within_archive(
+        &self,
+        core: &mut dyn crate::core::Core
+    ) -> Result<(bool, u32)> {


I don't quite follow this return type. Can you explain it with a comment?

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.

No need to change it now but it's also worth considering something like

enum PcResult { InArchive, NotInArchive { pc: u32 }, }

I do like that, changed.

labbott · 2026-07-09T13:47:14Z

+        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,
+                )),
+            ));
+        }


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 anyhow and have all the information in the ImageStateMismatch enum.

Done. That's a nice side effect of not calling validate anymore.

lzrd · 2026-07-09T17:10:18Z

Test this corner case: corrupt/erase both Hubris images, reset the RoT. Use humility to dump the registers. It should be in a bootleby loop. (check bootleby source).
The other harder to hit case is in pre-kernel where ROM routines are being used to check image signatures.
There should be an override flag "--i-know-what-i-am-doing" or some such.

evan-oxide · 2026-07-13T16:42:09Z


-        if criteria == HubrisValidate::ArchiveMatch {
+        if self.task_dump().is_some() {
            return Ok(());


@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::Booted in that case, it returns early here and skips the boot check.

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 :)

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 tasks and humility ringbuf and 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.

Thanks, I added a comment about it.

evan-oxide · 2026-07-13T20:41:51Z

Test this corner case: corrupt/erase both Hubris images, reset the RoT. Use humility to dump the registers. It should be in a bootleby loop. (check bootleby source). The other harder to hit case is in pre-kernel where ROM routines are being used to check image signatures. There should be an override flag "--i-know-what-i-am-doing" or some such.

I corrupted both the A and B images so that it would stay in the bootloader. humility registers succeeded since it doesn't check for an archive mismatch anyways. humility tasks failed as expected.

It's looking like we won't need a --skip-pc-check flag, but let's see what folks think of the changes to the test cases.

evan-oxide · 2026-07-13T20:48:51Z

I split up the test-case fixes into 3 commits. The first is straightforward changes like this file, where just the error message changed. But the other two are tests where the command used to succeed (or appear to succeed?) but now fails the PC check. Does anyone disagree with any of these changes and think that we should allow someone to, for example, run humility readvar while in the bootloader? If so, that might be a reason to add a --skip-pc-check flag.

I'm inclined to say that the change is fine and that it's okay to give failures if we're in the bootloader. For the SP in particular we should never expect to end up in the bootloader so any attempt to read things from hubris RAM is likely to be questionable.

The RoT may be a more interesting case because we do expect to briefly go through a non-hubris bootloader but that code is designed to be so minimal I think rejecting it is okay. Really I think if future us actually finds a situation where something like --skip-pc-check is valuable/solves a real problem I think we can add it then.

labbott

LGTM. This is actually originally my PR so I can't actually approve it!

labbott · 2026-07-15T13:12:32Z

+    if let Ok(PcResult::NotInArchive { 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?"
+        );
+    }


we drop the Err case here. That should only happen if we can't read the PC and we're probably going to have other errors that would get caught below so I think it's fine.

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?

labbott requested review from evan-oxide, hawkw and jamesmunns July 7, 2026 14:50

hawkw approved these changes Jul 7, 2026

View reviewed changes

evan-oxide mentioned this pull request Jul 7, 2026

Use BOOTED_IMAGE to warn when using wrong A/B image #712

Closed

evan-oxide reviewed Jul 7, 2026

View reviewed changes

evan-oxide reviewed Jul 8, 2026

View reviewed changes

evan-oxide added 4 commits July 8, 2026 15:26

restart core after checking PC

015a3a3

add read_image_id_from_flash()

9d708f0

don't call validate() before flashing, just check ID

4f99d53

warn before flashing if the PC suggests this is the wrong A/B image

02df730

labbott commented Jul 9, 2026

View reviewed changes

evan-oxide added 2 commits July 9, 2026 10:31

document is_pc_within_archive

0dd349c

use separate error variants for actual mismatch vs failure to check

2635ae7

mention bootloader and ROM in error message

1bab5bc

evan-oxide reviewed Jul 13, 2026

View reviewed changes

evan-oxide added 5 commits July 13, 2026 12:49

remove redundant PC check

c01d91c

add comment about single-task dumps

cfa9fe2

overwrite tests that just have a different error message now

b683227

overwrite flash-ram-mismatch tests that started erroring

787b3a1

overwrite in_bootloader tests that started erroring

3d1e66b

evan-oxide reviewed Jul 13, 2026

View reviewed changes

evan-oxide added 2 commits July 13, 2026 17:01

don't leave core halted if PC check fails

220886a

refactor is_pc_within_archive using PcResult

e6726f4

evan-oxide force-pushed the check_rot_a_b_image branch from 53952e2 to e6726f4 Compare July 13, 2026 21:07

labbott commented Jul 15, 2026

View reviewed changes

	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 \

Uh oh!

Conversation

labbott commented Jul 7, 2026

Uh oh!

labbott commented Jul 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzrd commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evan-oxide commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labbott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lzrd commented Jul 9, 2026 •

edited

Loading