Implement a software rollback protection policy in RoT update_server. #1809

lzrd · 2024-06-10T17:04:43Z

Issue #1570 software anti-rollback

Implement a software rollback protection policy in RoT update_server using:

the epoch value in the optional ImageHeader for early rejection (1st block presented before any flash erase/write) or
the EPOC tag in optional caboose for rejecting activation of an image.

"optional" because there are existent Bootleby images without ImageHeader or Caboose and Hubris images images in the future that may eliminate ImageHeader. In cases where an epoch value is not present, the image epoch is treated as equal to zero.

Related PRs:
https://github.com/oxidecomputer/sprot-release/pull/11
oxidecomputer/hubtools#32
https://github.com/oxidecomputer/permission-slip/pull/205
oxidecomputer/management-gateway-service#240

drv/lpc55-update-server/src/images.rs

…d rollback protection.

Remove last of the saturated math calls. Rename FlashRange::{store,exec} to {stored,at_runtime} for clarity. Remove unnecessary image validation logic in fn padded_image_len.

Don't require updating to hubtools 0.4.7. There doesn't need to be an 'EPOC' in the caboose as it is always "0" at this step in the release engineering flow. Fix complementary swapped args and corresponding swapped comparison to read correctly. Remove vestigial epoch header check. With the decision to not use ImageHeader.epoch, a TBD prep_image_update variant will contain the epoch information.

mkeeter · 2024-11-14T18:40:02Z

drv/stm32h7-update-server/src/main.rs

@@ -416,13 +416,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> {
        Ok(BLOCK_SIZE_BYTES)
    }

+    /// Deprecated. The version and epoch values are in the Caboose.


Can we remove this entirely? I don't see anyone calling it anymore.

mkeeter · 2024-11-14T18:40:28Z

drv/sprot-api/README.md

@@ -461,7 +461,7 @@ SpRot.status() => Status {
 }
 ```

-Update API, retrieve current version.
+Update API, retrieve current version (deprecated)


I think we may be able to remove SpRot.current_version entirely; I don't see anyone calling it anymore.

mkeeter · 2024-11-14T18:41:51Z

app/gimletlet/app-mgmt.toml

@@ -49,7 +49,7 @@ task-slots = ["sys", "user_leds"]

 [tasks.net]
 name = "task-net"
-stacksize = 3000
+stacksize = 8000


I'm assuming this is an unrelated change which you discovered when building every app-*.toml?

mkeeter · 2024-11-14T18:42:25Z

build/xtask/src/dist.rs

@@ -1567,7 +1567,7 @@ fn build_kernel(
 /// Returns true if the header was found and updated,
 /// false otherwise.
 fn update_image_header(
-    cfg: &PackageConfig,
+    _cfg: &PackageConfig,


Can we just remove this argument, since it's now unused?

mkeeter · 2024-11-14T18:43:31Z

drv/lpc55-update-api/src/lib.rs

+        if *self == SlotId::A {
+            SlotId::B
+        } else {
+            SlotId::A
+        }


Suggested change

if *self == SlotId::A {

SlotId::B

} else {

SlotId::A

}

match self {

SlotId::A => SlotId::B,

SlotId::B => SlotId::A,

}

slightly shorter, and will break loudly if someone adds a third slot

mkeeter · 2024-11-14T18:58:50Z

drv/lpc55-update-server/src/images.rs

+// An image may have an ImageHeader located after the
+// LPC55's mixed header/vector table.
+pub const IMAGE_HEADER_OFFSET: u32 = 0x130;
+pub const CABOOSE_TAG_EPOC: [u8; 4] = [b'E', b'P', b'O', b'C'];


Nit: this could be written as

Suggested change

pub const CABOOSE_TAG_EPOC: [u8; 4] = [b'E', b'P', b'O', b'C'];

pub const CABOOSE_TAG_EPOC: [u8; 4] = *b"EPOC";

mkeeter · 2024-11-14T19:00:29Z

drv/lpc55-update-server/src/images.rs

+
+/// Does (component, slot) refer to the currently running Hubris image?
+pub fn is_current_hubris_image(component: RotComponent, slot: SlotId) -> bool {
+    // Safety: extern statics aren't controlled by Rust so poking them can


Why is there a Safety comment here? There's no unsafe.

mkeeter · 2024-11-14T19:02:59Z

drv/lpc55-update-server/src/images.rs

+    nxp_image_executation_address: u32, // 0x32
+                                        // Additional trailing vectors are not
+                                        // interesting here.
+                                        // _vector_table_2[u32; 2]


Remove commented out _vector_table_2?

mkeeter · 2024-11-14T19:21:50Z

drv/lpc55-update-server/src/main.rs

-                return Ok(len as usize);
+    fn cached_stage0_image_len(&self) -> Result<usize, UpdateError> {
+        if let Ok(vectors) = ImageVectorsLpc55::try_from(
+            self.fw_cache[0..BLOCK_SIZE_BYTES].as_bytes(),


Slicing a BLOCK_SIZE_BYTES chunk then passing it into read_from_prefix doesn't make sense to me.

We should

Add a const assertion that fw_cache is long enough to fit core::mem::size_of::<ImageVectorsLpc55>

do something like

let vectors = ImageVectorsLpc55::read_from( &self.fw_cache[..size_of::<ImageVectorsLpc55>()] ).unwrap_lite()

Remove ImageVectorsLpc55's TryFrom implementation, since this is the only place it's used

mkeeter · 2024-11-14T19:24:52Z

drv/lpc55-update-server/src/images.rs

+) -> Result<Option<Epoch>, UpdateError> {
+    if let Ok(span) = caboose_slice(image) {
+        let mut block = [0u8; BLOCK_SIZE_BYTES];
+        let caboose = block[0..span.len()].as_bytes_mut();


⚠️ Can this line panic if the caboose is larger than BLOCK_SIZE_BYTES? It seems like we should instead have a custom type implementing TlvcRead that uses image to read caboose bytes.

mkeeter · 2024-11-14T19:26:26Z

drv/lpc55-update-server/src/images.rs

+    }
+
+    pub fn new_hybrid<'a>(
+        flash: &'a drv_lpc55_flash::Flash<'a>,


Nit: if the order in memory is [buffer, flash], then that should also be the argument order her (and constructor order below)

mkeeter · 2024-11-14T19:36:35Z

drv/lpc55-update-server/src/images.rs

+        return Err(UpdateError::InvalidHeaderBlock);
+    }
+
+    // We don't rely on the ImageHeader, but if it is there, it needs to be valid.


This PR doesn't remove the ImageHeader, right?

I'm wondering why the logic change here; previously, we required the ImageHeader if the component is RotComponent::Hubris (but allowed for bootloaders that have been released without an ImageHeader).

mkeeter · 2024-11-14T19:41:56Z

drv/lpc55-update-server/src/images.rs

+                    let ram_end_offset = ram.len().min(end_offset);
+                    // Transfer starts within the RAM part of this image.
+                    let data = ram
+                        .get((start_offset)..ram_end_offset)


Suggested change

.get((start_offset)..ram_end_offset)

.get(start_offset..ram_end_offset)

mkeeter · 2024-11-14T19:45:30Z

drv/lpc55-update-server/src/images.rs

+                let buffer =
+                    &mut [0u8; core::mem::size_of::<ImageVectorsLpc55>()];
+                self.read_bytes(0u32, buffer.as_bytes_mut())?;
+                ImageVectorsLpc55::read_from_prefix(&buffer[..])
+                    .ok_or(UpdateError::OutOfBounds)


Suggested change

let buffer =

&mut [0u8; core::mem::size_of::<ImageVectorsLpc55>()];

self.read_bytes(0u32, buffer.as_bytes_mut())?;

ImageVectorsLpc55::read_from_prefix(&buffer[..])

.ok_or(UpdateError::OutOfBounds)

let mut v = ImageVectorsLpc55::new_zeroed();

self.read_bytes(0u32, v.as_bytes_mut())?;

v

mkeeter · 2024-11-14T20:04:40Z

drv/lpc55-update-server/src/images.rs

+//   - values greater than u32::MAX
+//
+//   Hand coding reduces size by about 950 bytes.
+impl From<&[u8]> for Epoch {


I would vote to return an Option<Epoch> instead, or Result<Epoch, ()> if you want to use TryFrom. Snapping invalid epoch tags to 0 seems like a recipe for confusion, because it will allow updates to an invalid epoch if the initial epoch is 0.

mkeeter · 2024-11-14T20:07:27Z

drv/lpc55-update-server/src/images.rs

+/// This test is also used when the header block first arrives
+/// so that images can be rejected early, i.e. before any flash has
+/// been altered.
+pub fn check_rollback_policy(


It seems like this function is only ever useful when complete is true; otherwise, we can't read the epoch tag from the new image's caboose (so we have to keep loading more of the image). Could we remove the complete flag and only call the function on complete images?

mkeeter · 2024-11-14T20:08:01Z

drv/lpc55-update-server/src/images.rs

+        (Some(active_epoch), None) => {
+            // If next_image is partial and HEADER_BLOCK has no ImageHeader,
+            // then there is no early rejection, proceed.
+            if !complete || active_epoch.can_update_to(Epoch::from(0u32)) {


Same vague concerns as before: being able to update to an invalid / missing epoch (but only from epoch 0) seems odd to me.

labbott reviewed Jun 10, 2024

View reviewed changes

drv/lpc55-update-server/src/images.rs Outdated Show resolved Hide resolved

labbott reviewed Jun 11, 2024

View reviewed changes

drv/lpc55-update-server/src/images.rs Outdated Show resolved Hide resolved

labbott reviewed Jun 11, 2024

View reviewed changes

drv/lpc55-update-server/src/images.rs Outdated Show resolved Hide resolved

labbott reviewed Jun 11, 2024

View reviewed changes

drv/lpc55-update-server/src/images.rs Outdated Show resolved Hide resolved

lzrd force-pushed the epoch branch from 0279d8c to 33bb79b Compare June 30, 2024 04:32

lzrd force-pushed the epoch branch 5 times, most recently from 6ae7cb6 to 8e820c1 Compare August 16, 2024 17:30

lzrd marked this pull request as ready for review August 20, 2024 20:02

lzrd requested review from cbiffle and flihp as code owners August 20, 2024 20:02

lzrd force-pushed the epoch branch 3 times, most recently from 9030646 to cb0b519 Compare September 17, 2024 22:10

lzrd force-pushed the epoch branch from cb0b519 to c11a051 Compare September 30, 2024 23:01

lzrd force-pushed the epoch branch 4 times, most recently from db63fff to 4b27b65 Compare October 10, 2024 22:46

lzrd added 5 commits October 14, 2024 14:58

ImageAccess abstraction for later RoT update_server caboose access an…

1f7abf1

…d rollback protection.

Changes from PR review

38d7017

Address PR comments, remove calls to saturated_sub.

b381b21

Remove last of the saturated math calls. Rename FlashRange::{store,exec} to {stored,at_runtime} for clarity. Remove unnecessary image validation logic in fn padded_image_len.

Implement epoch-based rollback protection in LPC55 update_server

e6be5ce

deprecate ImageHeader version and epoch

6f5d861

lzrd force-pushed the epoch branch from 4b27b65 to 3c3d926 Compare October 15, 2024 18:48

lzrd force-pushed the epoch branch from 3c3d926 to aca5140 Compare October 16, 2024 04:58

mkeeter reviewed Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a software rollback protection policy in RoT update_server. #1809

Implement a software rollback protection policy in RoT update_server. #1809

lzrd commented Jun 10, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

mkeeter Nov 14, 2024

	pub const CABOOSE_TAG_EPOC: [u8; 4] = [b'E', b'P', b'O', b'C'];
	pub const CABOOSE_TAG_EPOC: [u8; 4] = *b"EPOC";

	.get((start_offset)..ram_end_offset)
	.get(start_offset..ram_end_offset)

Implement a software rollback protection policy in RoT update_server. #1809

Are you sure you want to change the base?

Implement a software rollback protection policy in RoT update_server. #1809

Conversation

lzrd commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment