From 1f7abf11447d6828bb8d43853804eb58c7e8c75f Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Wed, 12 Jun 2024 13:44:42 -0700 Subject: [PATCH 1/6] ImageAccess abstraction for later RoT update_server caboose access and rollback protection. --- app/lpc55xpresso/app.toml | 2 +- app/oxide-rot-1/app-dev.toml | 2 +- drv/lpc55-update-server/src/images.rs | 558 +++++++++++++++++++++++--- drv/lpc55-update-server/src/main.rs | 303 +++++--------- 4 files changed, 609 insertions(+), 256 deletions(-) diff --git a/app/lpc55xpresso/app.toml b/app/lpc55xpresso/app.toml index 34bd7efec..b6abfe0ab 100644 --- a/app/lpc55xpresso/app.toml +++ b/app/lpc55xpresso/app.toml @@ -48,7 +48,7 @@ start = true [tasks.update_server] name = "lpc55-update-server" priority = 3 -max-sizes = {flash = 26720, ram = 16704} +max-sizes = {flash = 27008, ram = 16704} stacksize = 8192 start = true sections = {bootstate = "usbsram"} diff --git a/app/oxide-rot-1/app-dev.toml b/app/oxide-rot-1/app-dev.toml index c1b7fa643..c5b2d422d 100644 --- a/app/oxide-rot-1/app-dev.toml +++ b/app/oxide-rot-1/app-dev.toml @@ -53,7 +53,7 @@ start = true [tasks.update_server] name = "lpc55-update-server" priority = 3 -max-sizes = {flash = 26080, ram = 17000, usbsram = 4096} +max-sizes = {flash = 27904, ram = 17344, usbsram = 4096} # TODO: Size this appropriately stacksize = 8192 start = true diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index bd9f085fb..4a69baafe 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -2,12 +2,32 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +/// Implement LPC55_`update_server`'s knowledge about bits inside an image. +/// +/// The update server needs to work with partial or otherwise invalid images. +/// Signature checks are only performed at boot time. The update server +/// does match FWIDs against the boot-time-info in some cases. But, because +/// flash areas are mutated during update_server operations and stuff can +/// happen, any data in the non-active Hubris image needs to be treated as +/// untrusted (update_server does not alter its own image). +/// Data structures, pointers, and offsets within an image are tested to +/// ensure no mischief during `update_server` operations. The remainder +/// of reliability and security concerns rely on the boot-time policies +/// of the LPC55 ROM and the stage0 bootloader. +use crate::{ + indirect_flash_read, round_up_to_flash_page, SIZEOF_U32, U32_SIZE, +}; +use abi::{ImageHeader, CABOOSE_MAGIC, HEADER_MAGIC}; use core::ops::Range; -use drv_lpc55_update_api::BLOCK_SIZE_BYTES; -use drv_lpc55_update_api::{RotComponent, SlotId}; +use drv_lpc55_flash::BYTES_PER_FLASH_PAGE; +use drv_lpc55_update_api::{RawCabooseError, RotComponent, SlotId}; use drv_update_api::UpdateError; -use userlib::UnwrapLite; +use zerocopy::{AsBytes, FromBytes}; +// Our layout of flash banks on the LPC55. +// Addresses are from the linker. +// The bootloader (`bootleby`) resides at __STAGE0_BASE and +// only cares about IMAGE_A and IMAGE_B. // We shouldn't actually dereference these. The types are not correct. // They are just here to allow a mechanism for getting the addresses. extern "C" { @@ -26,79 +46,499 @@ extern "C" { // Location of the NXP header pub const HEADER_BLOCK: usize = 0; -// NXP LPC55's mixed header/vector table offsets -const RESET_VECTOR_OFFSET: usize = 0x04; -pub const LENGTH_OFFSET: usize = 0x20; -pub const HEADER_OFFSET: u32 = 0x130; -const MAGIC_OFFSET: usize = HEADER_OFFSET as usize; +// An image may have an ImageHeader located after the +// LPC55's mixed header/vector table. +pub const IMAGE_HEADER_OFFSET: u32 = 0x130; -// Perform some sanity checking on the header block. -pub fn validate_header_block( - component: RotComponent, - slot: SlotId, - block: &[u8; BLOCK_SIZE_BYTES], -) -> Result<(), UpdateError> { - let exec = image_range(component, slot).1; +/// Address ranges that may contain an image during storage and active use. +/// `store` and `exec` ranges are the same except for `stage0next`. +pub struct FlashRange { + pub store: Range, + pub exec: Range, +} + +/// Get the flash storage address range and flash execution address range. +pub fn flash_range(component: RotComponent, slot: SlotId) -> FlashRange { + // Safety: Linker defined symbols are trusted. + unsafe { + match (component, slot) { + (RotComponent::Hubris, SlotId::A) => FlashRange { + store: __IMAGE_A_BASE.as_ptr() as u32 + ..__IMAGE_A_END.as_ptr() as u32, + exec: __IMAGE_A_BASE.as_ptr() as u32 + ..__IMAGE_A_END.as_ptr() as u32, + }, + (RotComponent::Hubris, SlotId::B) => FlashRange { + store: __IMAGE_B_BASE.as_ptr() as u32 + ..__IMAGE_B_END.as_ptr() as u32, + exec: __IMAGE_B_BASE.as_ptr() as u32 + ..__IMAGE_B_END.as_ptr() as u32, + }, + (RotComponent::Stage0, SlotId::A) => FlashRange { + store: __IMAGE_STAGE0_BASE.as_ptr() as u32 + ..__IMAGE_STAGE0_END.as_ptr() as u32, + exec: __IMAGE_STAGE0_BASE.as_ptr() as u32 + ..__IMAGE_STAGE0_END.as_ptr() as u32, + }, + (RotComponent::Stage0, SlotId::B) => FlashRange { + store: __IMAGE_STAGE0NEXT_BASE.as_ptr() as u32 + ..__IMAGE_STAGE0NEXT_END.as_ptr() as u32, + exec: __IMAGE_STAGE0_BASE.as_ptr() as u32 + ..__IMAGE_STAGE0_END.as_ptr() as u32, + }, + } + } +} + +/// Does (component, slot) refer to the currently running Hubris image? +pub fn same_image(component: RotComponent, slot: SlotId) -> bool { + // Safety: We are trusting the linker. + flash_range(component, slot).store.start + == unsafe { &__this_image } as *const _ as u32 +} + +// LPC55 defined image content + +/// Image header for the LPC55S6x device as documented in NXP UM11126 +#[repr(C)] +#[derive(Default, AsBytes, FromBytes)] +pub struct ImageVectorsLpc55 { + initial_sp: u32, // 0x00 + initial_pc: u32, // 0x04 + _vector_table_0: [u32; 6], // 0x08, 0c, 10, 14, 18, 1c + nxp_image_length: u32, // 0x20 + nxp_image_type: u32, // 0x24 + nxp_offset_to_specific_header: u32, // 0x28 + _vector_table_1: [u32; 2], // 0x2c, 0x30 + nxp_image_executation_address: u32, // 0x32 + // Additional trailing vectors are not + // interesting here. + // _vector_table_2[u32; 2] +} + +impl ImageVectorsLpc55 { + const IMAGE_TYPE_PLAIN_SIGNED_XIP_IMAGE: u32 = 4; + + pub fn is_image_type_signed_xip(&self) -> bool { + self.nxp_image_type == Self::IMAGE_TYPE_PLAIN_SIGNED_XIP_IMAGE + } // This part aliases flash in two positions that differ in bit 28. To allow // for either position to be used in new images, we clear bit 28 in all of // the numbers used for comparison below, by ANDing them with this mask: - const ADDRMASK: u32 = !(1 << 28); + pub fn normalized_initial_pc(&self) -> u32 { + const ADDRMASK: u32 = !(1 << 28); + self.initial_pc & ADDRMASK + } + + // Length of image from offset zero to end of the signature block (without padding) + // a.k.a. ImageVectorsLpc55.nxp_image_length + pub fn image_length(&self) -> Option { + if self.is_image_type_signed_xip() { + Some(self.nxp_image_length) + } else { + None + } + } + + /// Image length padded to nearest page size. + pub fn padded_image_len(&self) -> Option { + round_up_to_flash_page(self.image_length()?) + } + + /// Determine the bounds of an image assuming the given flash bank + /// addresses. + pub fn padded_image_range(&self, exec: &Range) -> Option> { + let image_start = exec.start; + let image_end = exec.start.saturating_add(self.padded_image_len()?); + // ImageHeader is optional. Assuming code cannot be earlier than + // IMAGE_HEADER_OFFSET. + let initial_pc_start = image_start.saturating_add(IMAGE_HEADER_OFFSET); + let initial_pc_end = + image_start.saturating_add(self.nxp_offset_to_specific_header); + let pc = exec.start.saturating_add(self.normalized_initial_pc()); + + if !exec.contains(&image_end) + || !exec.contains(&initial_pc_start) + || !exec.contains(&initial_pc_end) + || !(initial_pc_start..initial_pc_end).contains(&pc) + { + // One of these did not fit in the destination flash bank or + // exeecutable area of the image in that bank. + return None; + } + Some(image_start..image_end) + } +} + +impl TryFrom<&[u8]> for ImageVectorsLpc55 { + type Error = (); + + fn try_from(buffer: &[u8]) -> Result { + match ImageVectorsLpc55::read_from_prefix(buffer) { + Some(vectors) => Ok(vectors), + None => Err(()), + } + } +} + +/// Sanity check the image header block. +/// Return the offset to the end of the executable code which is also +/// the end of optional caboose and the beginning of the signature block. +pub fn validate_header_block( + header_access: &ImageAccess<'_>, +) -> Result { + let mut vectors = ImageVectorsLpc55::new_zeroed(); + let mut header = ImageHeader::new_zeroed(); + + if header_access.read_bytes(0, vectors.as_bytes_mut()).is_err() + || header_access + .read_bytes(IMAGE_HEADER_OFFSET, header.as_bytes_mut()) + .is_err() + { + // can't read block0 + return Err(UpdateError::InvalidHeaderBlock); + } + + if !vectors.is_image_type_signed_xip() + || vectors.nxp_offset_to_specific_header >= vectors.nxp_image_length + { + // Not a signed XIP image or no signature block. + // If we figure out a reasonable minimum size for the signature block + // we should test for that. + return Err(UpdateError::InvalidHeaderBlock); + } - let reset_vector = u32::from_le_bytes( - block[RESET_VECTOR_OFFSET..][..4].try_into().unwrap_lite(), - ) & ADDRMASK; + // We don't rely on the ImageHeader, but if it is there, it needs to be valid. + // Note that ImageHeader.epoch is needed for pre-flash-erase tests for + // rollback protection. + // TODO: Improve estimate of where the first executable instruction can be. + let code_offset = if header.magic == HEADER_MAGIC { + if header.total_image_len != vectors.nxp_offset_to_specific_header { + // ImageHeader disagrees with LPC55 vectors. + return Err(UpdateError::InvalidHeaderBlock); + } + IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32) + } else { + IMAGE_HEADER_OFFSET + }; - // Ensure the image is destined for the right target - if !exec.contains(&reset_vector) { + if vectors.nxp_image_length as usize > header_access.exec().len() { + // Image extends outside of flash bank. return Err(UpdateError::InvalidHeaderBlock); } - // Ensure the MAGIC is correct. - // Bootloaders have been released without an ImageHeader. Allow those. - let magic = - u32::from_le_bytes(block[MAGIC_OFFSET..][..4].try_into().unwrap_lite()); - if component == RotComponent::Hubris && magic != abi::HEADER_MAGIC { + let caboose_end = header_access + .exec() + .start + .saturating_add(vectors.nxp_offset_to_specific_header); + let text_start = header_access.exec().start.saturating_add(code_offset); + if !(text_start..caboose_end).contains(&vectors.normalized_initial_pc()) { return Err(UpdateError::InvalidHeaderBlock); } - Ok(()) + Ok(vectors.nxp_offset_to_specific_header) } -pub fn same_image(component: RotComponent, slot: SlotId) -> bool { - // Safety: We are trusting the linker. - image_range(component, slot).0.start - == unsafe { &__this_image } as *const _ as u32 +/// Get the range of the coboose contained within an image if it exists. +/// +/// This implementation has similar logic to the one in `stm32h7-update-server`, +/// but uses ImageAccess for images that, during various operations, +/// may be in RAM, Flash, or split between both. +pub fn caboose_slice( + image: &ImageAccess<'_>, +) -> Result, RawCabooseError> { + // The ImageHeader is optional since the offset to the start of + // the signature block (end of image) is also found in an LPC55 + // Type 4 (Signed XIP) image. + // + // In this context, NoImageHeader actually means that the image + // is not well formed. + let image_end_offset = validate_header_block(image) + .map_err(|_| RawCabooseError::NoImageHeader)?; + + // By construction, the last word of the caboose is its size as a `u32` + let caboose_size_offset: u32 = image_end_offset.saturating_sub(U32_SIZE); + let caboose_size = image + .read_word(caboose_size_offset) + .map_err(|_| RawCabooseError::ReadFailed)?; + + // Security considerations: + // If there is no caboose, then we may be indexing 0xff padding, or + // code/data and padding, or just code/data, and interpreting that as + // the caboose size. After an alignment and size check, some values + // could still get through. The pathological case would be code/data + // that indexes the CABOOSE_MAGIC constant in the code here that is + // testing for the caboose. The parts of the image that could then + // be accessed are already openly available. There doesn't seem + // to be an opportunity for any denial of service. + let caboose_magic_offset = image_end_offset.saturating_sub(caboose_size); + if ((caboose_magic_offset % U32_SIZE) != 0) + || !(IMAGE_HEADER_OFFSET..caboose_size_offset) + .contains(&caboose_magic_offset) + { + return Err(RawCabooseError::MissingCaboose); + } + + let caboose_magic = image + .read_word(caboose_magic_offset as u32) + .map_err(|_| RawCabooseError::MissingCaboose)?; + + if caboose_magic == CABOOSE_MAGIC { + let caboose_range = ((caboose_magic_offset as u32) + U32_SIZE) + ..(caboose_size_offset as u32); + Ok(caboose_range) + } else { + Err(RawCabooseError::MissingCaboose) + } } -/// Return the flash storage address range and flash execution address range. -/// These are only different for the staged stage0 image. -pub fn image_range( - component: RotComponent, - slot: SlotId, -) -> (Range, Range) { - unsafe { - match (component, slot) { - (RotComponent::Hubris, SlotId::A) => ( - __IMAGE_A_BASE.as_ptr() as u32..__IMAGE_A_END.as_ptr() as u32, - __IMAGE_A_BASE.as_ptr() as u32..__IMAGE_A_END.as_ptr() as u32, - ), - (RotComponent::Hubris, SlotId::B) => ( - __IMAGE_B_BASE.as_ptr() as u32..__IMAGE_B_END.as_ptr() as u32, - __IMAGE_B_BASE.as_ptr() as u32..__IMAGE_B_END.as_ptr() as u32, - ), - (RotComponent::Stage0, SlotId::A) => ( - __IMAGE_STAGE0_BASE.as_ptr() as u32 - ..__IMAGE_STAGE0_END.as_ptr() as u32, - __IMAGE_STAGE0_BASE.as_ptr() as u32 - ..__IMAGE_STAGE0_END.as_ptr() as u32, - ), - (RotComponent::Stage0, SlotId::B) => ( - __IMAGE_STAGE0NEXT_BASE.as_ptr() as u32 - ..__IMAGE_STAGE0NEXT_END.as_ptr() as u32, - __IMAGE_STAGE0_BASE.as_ptr() as u32 - ..__IMAGE_STAGE0_END.as_ptr() as u32, - ), +// Accessor keeps the implementation details of ImageAccess private +enum Accessor<'a> { + // Flash driver, flash device range + Flash { + flash: &'a drv_lpc55_flash::Flash<'a>, + span: FlashRange, + }, + Ram { + buffer: &'a [u8], + span: FlashRange, + }, + // Hybrid is used for later implementation of rollback protection. + // The buffer is used in place of the beginning of the flash range. + _Hybrid { + buffer: &'a [u8], + flash: &'a drv_lpc55_flash::Flash<'a>, + span: FlashRange, + }, +} + +impl Accessor<'_> { + fn exec(&self) -> &Range { + match self { + Accessor::Flash { flash: _, span } + | Accessor::Ram { buffer: _, span } + | Accessor::_Hybrid { + buffer: _, + flash: _, + span, + } => &span.exec, } } } + +/// The update_server needs to deal with images that are: +/// - Entirely in flash +/// - Header block in RAM with the remainder unavailable +/// - Header block in RAM with the remainder in flash. +/// - Entire image in RAM (cached Stage0) +/// Calls to methods use image relative addresses. +/// ImageAccess::Flash{_, span} gives the physical flash addresses +/// being accessed. +/// +/// Any address here is the "storage" address which for stage0next +/// and any image held temporarily in RAM is different than the +/// execution address. +pub struct ImageAccess<'a> { + accessor: Accessor<'a>, +} + +impl ImageAccess<'_> { + pub fn new_flash<'a>( + flash: &'a drv_lpc55_flash::Flash<'a>, + component: RotComponent, + slot: SlotId, + ) -> ImageAccess<'a> { + let span = flash_range(component, slot); + ImageAccess { + accessor: Accessor::Flash { flash, span }, + } + } + + pub fn new_ram( + buffer: &[u8], + component: RotComponent, + slot: SlotId, + ) -> ImageAccess<'_> { + let span = flash_range(component, slot); + ImageAccess { + accessor: Accessor::Ram { buffer, span }, + } + } + + pub fn _new_hybrid<'a>( + flash: &'a drv_lpc55_flash::Flash<'a>, + buffer: &'a [u8], + component: RotComponent, + slot: SlotId, + ) -> ImageAccess<'a> { + assert!((buffer.len() % BYTES_PER_FLASH_PAGE) == 0); + assert!(((buffer.as_ptr() as u32) % U32_SIZE) == 0); + let span = flash_range(component, slot); + ImageAccess { + accessor: Accessor::_Hybrid { + flash, + buffer, + span, + }, + } + } + + fn exec(&self) -> &Range { + self.accessor.exec() + } + + /// True if the u32 at offset is contained within the image. + pub fn is_addressable(&self, offset: u32) -> bool { + let len = match &self.accessor { + Accessor::Flash { flash: _, span } + | Accessor::_Hybrid { + buffer: _, + flash: _, + span, + } + | Accessor::Ram { buffer: _, span } => { + span.store.end.saturating_sub(span.store.start) + } + }; + offset < len && offset.saturating_add(U32_SIZE) <= len + } + + // Fetch a u32 from an image. + pub fn read_word(&self, offset: u32) -> Result { + if !self.is_addressable(offset) { + return Err(UpdateError::OutOfBounds); + } + match &self.accessor { + Accessor::Flash { flash, span } => { + let addr = span.store.start.saturating_add(offset); + let mut word = 0u32; + indirect_flash_read(flash, addr, word.as_bytes_mut())?; + Ok(word) + } + Accessor::Ram { buffer, span: _ } => { + match buffer + .get(offset as usize..(offset as usize + SIZEOF_U32)) + .and_then(u32::read_from) + { + Some(word) => Ok(word), + None => Err(UpdateError::OutOfBounds), + } + } + Accessor::_Hybrid { + buffer, + flash, + span, + } => { + if offset < buffer.len() as u32 { + match buffer + .get(offset as usize..(offset as usize + SIZEOF_U32)) + .and_then(u32::read_from) + { + Some(word) => Ok(word), + // Note: The case of a transfer spanning the RAM/Flash + // boundary would need to be unaligned given that the + // RAM portion must be whole u32-aligned pages. + None => Err(UpdateError::OutOfBounds), + } + } else { + let addr = span.store.start.saturating_add(offset); + let mut word = 0u32; + indirect_flash_read(flash, addr, word.as_bytes_mut())?; + Ok(word) + } + } + } + } + + pub fn read_bytes( + &self, + offset: u32, + buffer: &mut [u8], + ) -> Result<(), UpdateError> { + let len = buffer.len() as u32; + match &self.accessor { + Accessor::Flash { flash, span } => { + let start = offset.saturating_add(span.store.start); + let end = + offset.saturating_add(span.store.start).saturating_add(len); + if span.store.contains(&start) + && (span.store.start..=span.store.end).contains(&end) + { + Ok(indirect_flash_read(flash, start, buffer)?) + } else { + Err(UpdateError::OutOfBounds) + } + } + Accessor::Ram { + buffer: src, + span: _, + } => { + if let Some(data) = src.get( + (offset as usize)..(offset.saturating_add(len) as usize), + ) { + buffer.copy_from_slice(data); + Ok(()) + } else { + Err(UpdateError::OutOfBounds) + } + } + Accessor::_Hybrid { + buffer: ram, + flash, + span, + } => { + let mut offset = offset as usize; + let mut remainder = buffer.len(); + // Transfer data from the RAM portion of the image + if offset < ram.len() { + let ram_end_offset = ram.len().min(offset + remainder); + // Transfer starts within the RAM part of this image. + let data = ram + .get((offset)..ram_end_offset) + .ok_or(UpdateError::OutOfBounds)?; + buffer.copy_from_slice(data); + remainder -= data.len(); + offset = ram_end_offset; + } + // Transfer data from the flash-backed portion of the image. + if remainder > 0 { + let start = + offset.saturating_add(span.store.start as usize); + let end = start.saturating_add(remainder); + if span.store.contains(&(start as u32)) + && (span.store.start..=span.store.end) + .contains(&(end as u32)) + { + indirect_flash_read(flash, start as u32, buffer)?; + } else { + return Err(UpdateError::OutOfBounds); + } + } + Ok(()) + } + } + } + + /// Get the rounded up length of an LPC55 image if present. + pub fn padded_image_len(&self) -> Result { + let vectors = match self.accessor { + Accessor::Flash { .. } => { + let buffer = + &mut [0u8; core::mem::size_of::()]; + self.read_bytes(0u32, buffer.as_bytes_mut())?; + ImageVectorsLpc55::read_from_prefix(&buffer[..]) + .ok_or(UpdateError::OutOfBounds) + } + Accessor::Ram { buffer, span: _ } + | Accessor::_Hybrid { + buffer, + flash: _, + span: _, + } => ImageVectorsLpc55::read_from_prefix(buffer) + .ok_or(UpdateError::OutOfBounds), + }?; + let len = vectors.image_length().ok_or(UpdateError::BadLength)?; + round_up_to_flash_page(len).ok_or(UpdateError::BadLength) + } +} diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 7e013d63f..0e7b4f607 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -9,8 +9,9 @@ #![no_std] #![no_main] +use crate::images::{validate_header_block, ImageVectorsLpc55}; use core::convert::Infallible; -use core::mem::MaybeUninit; +use core::mem::{size_of, MaybeUninit}; use core::ops::Range; use drv_lpc55_flash::{BYTES_PER_FLASH_PAGE, BYTES_PER_FLASH_WORD}; use drv_lpc55_update_api::{ @@ -28,14 +29,18 @@ use stage0_handoff::{ RotBootStateV2, }; use userlib::*; -use zerocopy::{AsBytes, FromBytes}; +use zerocopy::AsBytes; mod images; -use crate::images::*; +use crate::images::{ + caboose_slice, flash_range, same_image, ImageAccess, HEADER_BLOCK, +}; -const U32_SIZE: u32 = core::mem::size_of::() as u32; const PAGE_SIZE: u32 = BYTES_PER_FLASH_PAGE as u32; +const SIZEOF_U32: usize = size_of::(); +const U32_SIZE: u32 = SIZEOF_U32 as u32; + #[used] #[link_section = ".bootstate"] static BOOTSTATE: MaybeUninit<[u8; 0x1000]> = MaybeUninit::uninit(); @@ -88,7 +93,7 @@ struct ServerImpl<'a> { // Used to enforce sequential writes from the control plane. next_block: Option, // Keep the fw cache 32-bit aligned to make NXP header access easier. - fw_cache: &'a mut [u32; FW_CACHE_MAX / core::mem::size_of::()], + fw_cache: &'a mut [u32; FW_CACHE_MAX / SIZEOF_U32], } const BLOCK_SIZE_BYTES: usize = BYTES_PER_FLASH_PAGE; @@ -188,8 +193,9 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { .read_range(0..len, &mut header_block[..]) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; header_block[len..].fill(ERASE_BYTE); - if let Err(e) = validate_header_block(component, slot, header_block) - { + let next_image = + ImageAccess::new_ram(header_block, component, slot); + if let Err(e) = validate_header_block(&next_image) { self.header_block = None; return Err(e.into()); } @@ -253,7 +259,7 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { // Now erase the unused portion of the flash slot so that // flash slot has predictable contents and the FWID for it // has some meaning. - let range = image_range(component, slot).0; + let range = &flash_range(component, slot).store; let erase_start = range.start + (endblock as u32 * PAGE_SIZE); self.flash_erase_range(erase_start..range.end)?; self.state = UpdateState::Finished; @@ -368,30 +374,26 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { fn read_raw_caboose( &mut self, - _msg: &RecvMessage, + msg: &RecvMessage, slot: SlotId, offset: u32, data: Leased, ) -> Result<(), RequestError> { - let caboose = caboose_slice(&self.flash, RotComponent::Hubris, slot)?; - if offset as usize + data.len() > caboose.len() { - return Err(RawCabooseError::InvalidRead.into()); - } - copy_from_caboose_chunk( - &self.flash, - caboose, - offset..offset + data.len() as u32, + self.component_read_raw_caboose( + msg, + RotComponent::Hubris, + slot, + offset, data, ) } fn caboose_size( &mut self, - _: &RecvMessage, + msg: &RecvMessage, slot: SlotId, ) -> Result> { - let caboose = caboose_slice(&self.flash, RotComponent::Hubris, slot)?; - Ok(caboose.end - caboose.start) + self.component_caboose_size(msg, RotComponent::Hubris, slot) } fn switch_default_image( @@ -452,8 +454,10 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { component: RotComponent, slot: SlotId, ) -> Result> { - let caboose = caboose_slice(&self.flash, component, slot)?; - Ok(caboose.end - caboose.start) + let image = ImageAccess::new_flash(&self.flash, component, slot); + let caboose = caboose_slice(&image)?; + let caboose_len = caboose.end.saturating_sub(caboose.start); + Ok(caboose_len) } fn component_read_raw_caboose( @@ -464,12 +468,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { offset: u32, data: Leased, ) -> Result<(), idol_runtime::RequestError> { - let caboose = caboose_slice(&self.flash, component, slot)?; - if offset as usize + data.len() > caboose.len() { + let image = ImageAccess::new_flash(&self.flash, component, slot); + let caboose = caboose_slice(&image)?; + let caboose_len = caboose.end.saturating_sub(caboose.start); + if offset as usize + data.len() > (caboose_len as usize) { return Err(RawCabooseError::InvalidRead.into()); } - copy_from_caboose_chunk( - &self.flash, + copy_from_caboose_chunk_to_lease( + &image, caboose, offset..offset + data.len() as u32, data, @@ -695,9 +701,8 @@ impl ServerImpl<'_> { // signature routine. // Read stage0next contents into RAM. - let staged = image_range(RotComponent::Stage0, SlotId::B); - let len = self.read_flash_image_to_cache(staged.0)?; - let bootloader = &self.fw_cache[..len / core::mem::size_of::()]; + let len = self.read_stage0next_to_cache()?; + let bootloader = &self.fw_cache[..len / SIZEOF_U32]; let mut hash = Sha3_256::new(); for page in bootloader.as_bytes().chunks(512) { @@ -722,12 +727,9 @@ impl ServerImpl<'_> { }; // Don't risk an update if the cache already matches the bootloader. - let stage0 = image_range(RotComponent::Stage0, SlotId::A); - match self.compare_cache_to_flash(&stage0.0) { + match self.compare_cache_to_stage0() { Err(UpdateError::ImageMismatch) => { - if let Err(e) = - self.write_cache_to_flash(RotComponent::Stage0, SlotId::A) - { + if let Err(e) = self.write_cache_to_stage0() { // N.B. An error here is bad since it means we've likely // bricked the machine if we reset now. // We do not want the RoT reset. @@ -757,8 +759,10 @@ impl ServerImpl<'_> { // Finish by erasing the unused portion of flash bank. // An error here means that the stage0 slot may not be clean but at least // it has the intended bootloader written. - let erase_start = stage0.0.start.checked_add(len as u32).unwrap_lite(); - self.flash_erase_range(erase_start..stage0.0.end)?; + let stage0 = &flash_range(RotComponent::Stage0, SlotId::A).store; + if let Some(erase_start) = stage0.start.checked_add(len as u32) { + self.flash_erase_range(erase_start..stage0.end)?; + } Ok(()) } @@ -781,105 +785,86 @@ impl ServerImpl<'_> { } } - fn compare_cache_to_flash( - &self, - span: &Range, - ) -> Result<(), UpdateError> { - // Is there a cached image? - // no, return error - + fn compare_cache_to_stage0(&self) -> Result<(), UpdateError> { // Lengths are rounded up to a flash page boundary. - let clen = self.cache_image_len()?; - let flen = self.flash_image_len(span)?; - if clen != flen { + let clen = self.cached_stage0_image_len()?; + + let stage0 = ImageAccess::new_flash( + &self.flash, + RotComponent::Stage0, + SlotId::A, + ); + if let Ok(stage0_len) = stage0.padded_image_len() { + if clen != stage0_len as usize { + // Different sizes cannot match. + // Ok to update if signature on stage0next is good. + return Err(UpdateError::ImageMismatch); + } + } else { + // We can't get a length for stage0. + // Perhaps it has been corrupted by an earlier update attempt + // and we're desperately trying to fix it by writing a good stage0 + // image. + // Ok to update if signature on stage0next is good. return Err(UpdateError::ImageMismatch); - } + }; // compare flash page to cache - let cached = - self.fw_cache[0..flen / core::mem::size_of::()].as_bytes(); + let cached = self.fw_cache[0..clen / SIZEOF_U32].as_bytes(); let mut flash_page = [0u8; BYTES_PER_FLASH_PAGE]; - for addr in (0..flen).step_by(BYTES_PER_FLASH_PAGE) { - let size = if addr + BYTES_PER_FLASH_PAGE > flen { - flen - addr - } else { - BYTES_PER_FLASH_PAGE - }; - - indirect_flash_read( - &self.flash, - addr as u32, - &mut flash_page[..size], - )?; - if flash_page[0..size] != cached[addr..addr + size] { + for addr in (0u32..(clen as u32)).step_by(BYTES_PER_FLASH_PAGE) { + // If we encounter an unreadable page in stage0 then + // ok to update if signature on stage0next later tests ok. + stage0 + .read_bytes(addr, flash_page.as_bytes_mut()) + .map_err(|_| UpdateError::ImageMismatch)?; + if flash_page + != cached[(addr as usize) + ..(addr as usize).saturating_add(BYTES_PER_FLASH_PAGE)] + { return Err(UpdateError::ImageMismatch); } } + // Images already match, don't wear out flash more than necessary. Ok(()) } - // Looking at a region of flash, determine if there is a possible NXP - // image programmed. Return the length in bytes of the flash pages - // comprising the image including padding to fill to a page boundary. - fn flash_image_len(&self, span: &Range) -> Result { - let buf = &mut [0u32; 1]; - indirect_flash_read( - &self.flash, - span.start + LENGTH_OFFSET as u32, - buf[..].as_bytes_mut(), - )?; - if let Some(len) = round_up_to_flash_page(buf[0]) { - // The minimum image size should be further constrained - // but this is enough bytes for an NXP header and not - // bigger than the flash slot. - if len as usize <= span.len() && len >= HEADER_OFFSET { - return Ok(len as usize); + fn cached_stage0_image_len(&self) -> Result { + if let Ok(vectors) = ImageVectorsLpc55::try_from( + self.fw_cache[0..BLOCK_SIZE_BYTES].as_bytes(), + ) { + // Get the length of the image iff it fits in its ultimate destination. + let exec_range = &flash_range(RotComponent::Stage0, SlotId::A).exec; + if let Some(image) = vectors.padded_image_range(exec_range) { + return Ok(image.len()); } } Err(UpdateError::BadLength) } - fn cache_image_len(&self) -> Result { - let len = round_up_to_flash_page( - self.fw_cache[LENGTH_OFFSET / core::mem::size_of::()], - ) - .ok_or(UpdateError::BadLength)?; - - if len as usize > self.fw_cache.as_bytes().len() || len < HEADER_OFFSET - { - return Err(UpdateError::BadLength); - } - Ok(len as usize) - } - - fn read_flash_image_to_cache( - &mut self, - span: Range, - ) -> Result { - // Returns error if flash page is erased. - let staged = image_range(RotComponent::Stage0, SlotId::B); - let len = self.flash_image_len(&staged.0)?; - if len as u32 > span.end || len > self.fw_cache.as_bytes().len() { + // Note: The only use case is to read stage0next into RAM. + fn read_stage0next_to_cache(&mut self) -> Result { + let stage0next = ImageAccess::new_flash( + &self.flash, + RotComponent::Stage0, + SlotId::B, + ); + let len = stage0next.padded_image_len()? as usize; + if len > self.fw_cache.as_bytes().len() { + // This stage0 image is too big for our buffer. return Err(UpdateError::BadLength); } - indirect_flash_read( - &self.flash, - span.start, - self.fw_cache[0..len / core::mem::size_of::()].as_bytes_mut(), - )?; + stage0next + .read_bytes(0, self.fw_cache.as_bytes_mut()[0..len].as_mut())?; Ok(len) } - fn write_cache_to_flash( - &mut self, - component: RotComponent, - slot: SlotId, - ) -> Result<(), UpdateError> { - let clen = self.cache_image_len()?; + fn write_cache_to_stage0(&mut self) -> Result<(), UpdateError> { + let clen = self.cached_stage0_image_len()?; if clen % BYTES_PER_FLASH_PAGE != 0 { return Err(UpdateError::BadLength); } - let span = image_range(component, slot).0; - if span.end < span.start + clen as u32 { + let span = &flash_range(RotComponent::Stage0, SlotId::A); + if span.store.end < span.store.start.saturating_add(clen as u32) { return Err(UpdateError::BadLength); } // Sanity check could be repeated here. @@ -891,8 +876,8 @@ impl ServerImpl<'_> { let flash_page = block.try_into().unwrap_lite(); do_block_write( &mut self.flash, - component, - slot, + RotComponent::Stage0, + SlotId::A, block_num, flash_page, )?; @@ -1188,7 +1173,7 @@ fn target_addr( slot: SlotId, page_num: u32, ) -> Option { - let range = image_range(component, slot).0; + let range = &flash_range(component, slot).store; // This is safely calculating addr = base + page_num * PAGE_SIZE let addr = page_num @@ -1203,81 +1188,8 @@ fn target_addr( Some(addr) } -/// Finds the memory range which contains the caboose for the given slot -/// -/// This implementation has similar logic to the one in `stm32h7-update-server`, -/// but uses indirect reads instead of mapping the alternate bank into flash. -fn caboose_slice( - flash: &drv_lpc55_flash::Flash<'_>, - component: RotComponent, - slot: SlotId, -) -> Result, RawCabooseError> { - let flash_range = image_range(component, slot).0; - - // If all is going according to plan, there will be a valid Hubris image - // flashed into the other slot, delimited by `__IMAGE_A/B_BASE` and - // `__IMAGE_A/B_END` (which are symbols injected by the linker). - // - // We'll first want to read the image header, which is at a fixed - // location at the end of the vector table. The length of the vector - // table is fixed in hardware, so this should never change. - const HEADER_OFFSET: u32 = 0x130; - let mut header = ImageHeader::new_zeroed(); - - indirect_flash_read( - flash, - flash_range.start + HEADER_OFFSET, - header.as_bytes_mut(), - ) - .map_err(|_| RawCabooseError::ReadFailed)?; - if header.magic != HEADER_MAGIC { - return Err(RawCabooseError::NoImageHeader); - } - - // Calculate where the image header implies that the image should end - // - // This is a one-past-the-end value. - let image_end = flash_range.start + header.total_image_len; - - // Then, check that value against the BANK2 bounds. - // - // Safety: populated by the linker, so this should be valid - if image_end > flash_range.end { - return Err(RawCabooseError::MissingCaboose); - } - - // By construction, the last word of the caboose is its size as a `u32` - let mut caboose_size = 0u32; - indirect_flash_read( - flash, - image_end - U32_SIZE, - caboose_size.as_bytes_mut(), - ) - .map_err(|_| RawCabooseError::ReadFailed)?; - - let caboose_start = image_end.saturating_sub(caboose_size); - let caboose_range = if caboose_start < flash_range.start { - // This branch will be encountered if there's no caboose, because - // then the nominal caboose size will be 0xFFFFFFFF, which will send - // us out of the bank2 region. - return Err(RawCabooseError::MissingCaboose); - } else { - // Safety: we know this pointer is within the programmed flash region, - // since it's checked above. - let mut v = 0u32; - indirect_flash_read(flash, caboose_start, v.as_bytes_mut()) - .map_err(|_| RawCabooseError::ReadFailed)?; - if v == CABOOSE_MAGIC { - caboose_start + U32_SIZE..image_end - U32_SIZE - } else { - return Err(RawCabooseError::MissingCaboose); - } - }; - Ok(caboose_range) -} - -fn copy_from_caboose_chunk( - flash: &drv_lpc55_flash::Flash<'_>, +fn copy_from_caboose_chunk_to_lease( + image: &ImageAccess<'_>, caboose: core::ops::Range, pos: core::ops::Range, data: Leased, @@ -1289,17 +1201,18 @@ fn copy_from_caboose_chunk( } const BUF_SIZE: usize = 128; - let mut offset = 0; + let mut offset = 0u32; let mut buf = [0u8; BUF_SIZE]; while remaining > 0 { let count = remaining.min(buf.len() as u32); let buf = &mut buf[..count as usize]; - indirect_flash_read(flash, caboose.start + pos.start + offset, buf) + image + .read_bytes(caboose.start + pos.start + offset, buf) .map_err(|_| RequestError::from(RawCabooseError::ReadFailed))?; data.write_range(offset as usize..(offset + count) as usize, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; - offset += count; - remaining -= count; + offset = offset.saturating_add(count); + remaining = remaining.saturating_sub(count); } Ok(()) } @@ -1351,7 +1264,7 @@ fn main() -> ! { // Go ahead and put the HASHCRYPT unit into reset. syscon.enter_reset(drv_lpc55_syscon_api::Peripheral::HashAes); let fw_cache = mutable_statics::mutable_statics! { - static mut FW_CACHE: [u32; FW_CACHE_MAX / core::mem::size_of::()] = [|| 0; _]; + static mut FW_CACHE: [u32; FW_CACHE_MAX / SIZEOF_U32] = [|| 0; _]; }; let mut server = ServerImpl { header_block: None, From 38d7017b0d8ec0d40d83e7cb83e9321c8f16ae70 Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Tue, 9 Jul 2024 12:50:02 -0700 Subject: [PATCH 2/6] Changes from PR review --- drv/lpc55-update-server/src/images.rs | 24 +++++++++--------------- drv/lpc55-update-server/src/main.rs | 5 +++-- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index 4a69baafe..fc8a3d148 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -19,7 +19,6 @@ use crate::{ }; use abi::{ImageHeader, CABOOSE_MAGIC, HEADER_MAGIC}; use core::ops::Range; -use drv_lpc55_flash::BYTES_PER_FLASH_PAGE; use drv_lpc55_update_api::{RawCabooseError, RotComponent, SlotId}; use drv_update_api::UpdateError; use zerocopy::{AsBytes, FromBytes}; @@ -91,7 +90,7 @@ pub fn flash_range(component: RotComponent, slot: SlotId) -> FlashRange { } /// Does (component, slot) refer to the currently running Hubris image? -pub fn same_image(component: RotComponent, slot: SlotId) -> bool { +pub fn is_current_hubris_image(component: RotComponent, slot: SlotId) -> bool { // Safety: We are trusting the linker. flash_range(component, slot).store.start == unsafe { &__this_image } as *const _ as u32 @@ -293,7 +292,7 @@ pub fn caboose_slice( } } -// Accessor keeps the implementation details of ImageAccess private +/// Accessor keeps the implementation details of ImageAccess private enum Accessor<'a> { // Flash driver, flash device range Flash { @@ -372,8 +371,6 @@ impl ImageAccess<'_> { component: RotComponent, slot: SlotId, ) -> ImageAccess<'a> { - assert!((buffer.len() % BYTES_PER_FLASH_PAGE) == 0); - assert!(((buffer.as_ptr() as u32) % U32_SIZE) == 0); let span = flash_range(component, slot); ImageAccess { accessor: Accessor::_Hybrid { @@ -459,9 +456,8 @@ impl ImageAccess<'_> { let len = buffer.len() as u32; match &self.accessor { Accessor::Flash { flash, span } => { - let start = offset.saturating_add(span.store.start); - let end = - offset.saturating_add(span.store.start).saturating_add(len); + let start = span.store.start.saturating_add(offset); + let end = start.saturating_add(len); if span.store.contains(&start) && (span.store.start..=span.store.end).contains(&end) { @@ -503,14 +499,12 @@ impl ImageAccess<'_> { } // Transfer data from the flash-backed portion of the image. if remainder > 0 { - let start = - offset.saturating_add(span.store.start as usize); - let end = start.saturating_add(remainder); - if span.store.contains(&(start as u32)) - && (span.store.start..=span.store.end) - .contains(&(end as u32)) + let start = span.store.start.saturating_add(offset as u32); + let end = start.saturating_add(remainder as u32); + if span.store.contains(&start) + && (span.store.start..=span.store.end).contains(&end) { - indirect_flash_read(flash, start as u32, buffer)?; + indirect_flash_read(flash, start, buffer)?; } else { return Err(UpdateError::OutOfBounds); } diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 0e7b4f607..48d9f4a33 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -33,7 +33,8 @@ use zerocopy::AsBytes; mod images; use crate::images::{ - caboose_slice, flash_range, same_image, ImageAccess, HEADER_BLOCK, + caboose_slice, flash_range, is_current_hubris_image, ImageAccess, + HEADER_BLOCK, }; const PAGE_SIZE: u32 = BYTES_PER_FLASH_PAGE as u32; @@ -1143,7 +1144,7 @@ fn do_block_write( let page_num = block_num as u32; // Can only update opposite image - if same_image(component, slot) { + if is_current_hubris_image(component, slot) { return Err(UpdateError::RunningImage); } From b381b210eb9afc8cd0b27260bd9e537eef7aa5d5 Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Wed, 7 Aug 2024 14:11:32 -0700 Subject: [PATCH 3/6] Address PR comments, remove calls to saturated_sub. 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. --- drv/lpc55-update-server/src/images.rs | 302 ++++++++++++++------------ drv/lpc55-update-server/src/main.rs | 52 +++-- 2 files changed, 194 insertions(+), 160 deletions(-) diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index fc8a3d148..49d66210e 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -19,6 +19,7 @@ use crate::{ }; use abi::{ImageHeader, CABOOSE_MAGIC, HEADER_MAGIC}; use core::ops::Range; +use core::ptr::addr_of; use drv_lpc55_update_api::{RawCabooseError, RotComponent, SlotId}; use drv_update_api::UpdateError; use zerocopy::{AsBytes, FromBytes}; @@ -26,9 +27,7 @@ use zerocopy::{AsBytes, FromBytes}; // Our layout of flash banks on the LPC55. // Addresses are from the linker. // The bootloader (`bootleby`) resides at __STAGE0_BASE and -// only cares about IMAGE_A and IMAGE_B. -// We shouldn't actually dereference these. The types are not correct. -// They are just here to allow a mechanism for getting the addresses. +// only references IMAGE_A and IMAGE_B. extern "C" { static __IMAGE_A_BASE: [u32; 0]; static __IMAGE_B_BASE: [u32; 0]; @@ -50,39 +49,45 @@ pub const HEADER_BLOCK: usize = 0; pub const IMAGE_HEADER_OFFSET: u32 = 0x130; /// Address ranges that may contain an image during storage and active use. -/// `store` and `exec` ranges are the same except for `stage0next`. +/// `stored` and `at_runtime` ranges are the same except for `stage0next`. +// TODO: Make these RangeInclusive in case we ever need to model +// some slot at the end of the address space. pub struct FlashRange { - pub store: Range, - pub exec: Range, + pub stored: Range, + pub at_runtime: Range, } /// Get the flash storage address range and flash execution address range. pub fn flash_range(component: RotComponent, slot: SlotId) -> FlashRange { - // Safety: Linker defined symbols are trusted. + // Safety: this block requires unsafe code to generate references to the + // extern "C" statics. Because we're only getting their addresses (all + // operations below are just as_ptr), we can't really trigger any UB here. + // The addresses themselves are assumed to be valid because they're + // produced by the linker, which we implicitly trust. unsafe { match (component, slot) { (RotComponent::Hubris, SlotId::A) => FlashRange { - store: __IMAGE_A_BASE.as_ptr() as u32 + stored: __IMAGE_A_BASE.as_ptr() as u32 ..__IMAGE_A_END.as_ptr() as u32, - exec: __IMAGE_A_BASE.as_ptr() as u32 + at_runtime: __IMAGE_A_BASE.as_ptr() as u32 ..__IMAGE_A_END.as_ptr() as u32, }, (RotComponent::Hubris, SlotId::B) => FlashRange { - store: __IMAGE_B_BASE.as_ptr() as u32 + stored: __IMAGE_B_BASE.as_ptr() as u32 ..__IMAGE_B_END.as_ptr() as u32, - exec: __IMAGE_B_BASE.as_ptr() as u32 + at_runtime: __IMAGE_B_BASE.as_ptr() as u32 ..__IMAGE_B_END.as_ptr() as u32, }, (RotComponent::Stage0, SlotId::A) => FlashRange { - store: __IMAGE_STAGE0_BASE.as_ptr() as u32 + stored: __IMAGE_STAGE0_BASE.as_ptr() as u32 ..__IMAGE_STAGE0_END.as_ptr() as u32, - exec: __IMAGE_STAGE0_BASE.as_ptr() as u32 + at_runtime: __IMAGE_STAGE0_BASE.as_ptr() as u32 ..__IMAGE_STAGE0_END.as_ptr() as u32, }, (RotComponent::Stage0, SlotId::B) => FlashRange { - store: __IMAGE_STAGE0NEXT_BASE.as_ptr() as u32 + stored: __IMAGE_STAGE0NEXT_BASE.as_ptr() as u32 ..__IMAGE_STAGE0NEXT_END.as_ptr() as u32, - exec: __IMAGE_STAGE0_BASE.as_ptr() as u32 + at_runtime: __IMAGE_STAGE0_BASE.as_ptr() as u32 ..__IMAGE_STAGE0_END.as_ptr() as u32, }, } @@ -91,9 +96,11 @@ pub fn flash_range(component: RotComponent, slot: SlotId) -> FlashRange { /// Does (component, slot) refer to the currently running Hubris image? pub fn is_current_hubris_image(component: RotComponent, slot: SlotId) -> bool { - // Safety: We are trusting the linker. - flash_range(component, slot).store.start - == unsafe { &__this_image } as *const _ as u32 + // Safety: extern statics aren't controlled by Rust so poking them can + // cause UB; in this case, it's zero length and we are only taking its + // numerical address, so we're not at risk. + flash_range(component, slot).stored.start + == unsafe { addr_of!(__this_image) as u32 } } // LPC55 defined image content @@ -147,25 +154,13 @@ impl ImageVectorsLpc55 { /// Determine the bounds of an image assuming the given flash bank /// addresses. - pub fn padded_image_range(&self, exec: &Range) -> Option> { - let image_start = exec.start; - let image_end = exec.start.saturating_add(self.padded_image_len()?); - // ImageHeader is optional. Assuming code cannot be earlier than - // IMAGE_HEADER_OFFSET. - let initial_pc_start = image_start.saturating_add(IMAGE_HEADER_OFFSET); - let initial_pc_end = - image_start.saturating_add(self.nxp_offset_to_specific_header); - let pc = exec.start.saturating_add(self.normalized_initial_pc()); - - if !exec.contains(&image_end) - || !exec.contains(&initial_pc_start) - || !exec.contains(&initial_pc_end) - || !(initial_pc_start..initial_pc_end).contains(&pc) - { - // One of these did not fit in the destination flash bank or - // exeecutable area of the image in that bank. - return None; - } + pub fn padded_image_range( + &self, + at_runtime: &Range, + ) -> Option> { + let image_start = at_runtime.start; + let image_end = + at_runtime.start.checked_add(self.padded_image_len()?)?; Some(image_start..image_end) } } @@ -190,15 +185,16 @@ pub fn validate_header_block( let mut vectors = ImageVectorsLpc55::new_zeroed(); let mut header = ImageHeader::new_zeroed(); + // Read block 0 and the header contained within (if available). if header_access.read_bytes(0, vectors.as_bytes_mut()).is_err() || header_access .read_bytes(IMAGE_HEADER_OFFSET, header.as_bytes_mut()) .is_err() { - // can't read block0 return Err(UpdateError::InvalidHeaderBlock); } + // Check image type and presence of signature block. if !vectors.is_image_type_signed_xip() || vectors.nxp_offset_to_specific_header >= vectors.nxp_image_length { @@ -209,29 +205,40 @@ pub fn validate_header_block( } // We don't rely on the ImageHeader, but if it is there, it needs to be valid. - // Note that ImageHeader.epoch is needed for pre-flash-erase tests for - // rollback protection. + // Note that `ImageHeader.epoch` is used by rollback protection for early + // rejection of invalid images. // TODO: Improve estimate of where the first executable instruction can be. let code_offset = if header.magic == HEADER_MAGIC { if header.total_image_len != vectors.nxp_offset_to_specific_header { // ImageHeader disagrees with LPC55 vectors. return Err(UpdateError::InvalidHeaderBlock); } + // Adding constants should be resolved at compile time: no call to panic. IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32) } else { IMAGE_HEADER_OFFSET }; - if vectors.nxp_image_length as usize > header_access.exec().len() { + if vectors.nxp_image_length as usize > header_access.at_runtime().len() { // Image extends outside of flash bank. return Err(UpdateError::InvalidHeaderBlock); } + // Check that the initial PC is pointing to a reasonable location. + // We only have information from image block zero, so this is just + // a basic sanity check. + // A check at signing time can be more exact, but this helps reject + // ridiculous images before any flash is erased. let caboose_end = header_access - .exec() + .at_runtime() + .start + .checked_add(vectors.nxp_offset_to_specific_header) + .ok_or(UpdateError::InvalidHeaderBlock)?; + let text_start = header_access + .at_runtime() .start - .saturating_add(vectors.nxp_offset_to_specific_header); - let text_start = header_access.exec().start.saturating_add(code_offset); + .checked_add(code_offset) + .ok_or(UpdateError::InvalidHeaderBlock)?; if !(text_start..caboose_end).contains(&vectors.normalized_initial_pc()) { return Err(UpdateError::InvalidHeaderBlock); } @@ -239,7 +246,7 @@ pub fn validate_header_block( Ok(vectors.nxp_offset_to_specific_header) } -/// Get the range of the coboose contained within an image if it exists. +/// Get the range of the caboose contained within an image if it exists. /// /// This implementation has similar logic to the one in `stm32h7-update-server`, /// but uses ImageAccess for images that, during various operations, @@ -257,21 +264,26 @@ pub fn caboose_slice( .map_err(|_| RawCabooseError::NoImageHeader)?; // By construction, the last word of the caboose is its size as a `u32` - let caboose_size_offset: u32 = image_end_offset.saturating_sub(U32_SIZE); + let caboose_size_offset = image_end_offset + .checked_sub(U32_SIZE) + .ok_or(RawCabooseError::MissingCaboose)?; let caboose_size = image .read_word(caboose_size_offset) .map_err(|_| RawCabooseError::ReadFailed)?; // Security considerations: - // If there is no caboose, then we may be indexing 0xff padding, or - // code/data and padding, or just code/data, and interpreting that as - // the caboose size. After an alignment and size check, some values - // could still get through. The pathological case would be code/data - // that indexes the CABOOSE_MAGIC constant in the code here that is - // testing for the caboose. The parts of the image that could then - // be accessed are already openly available. There doesn't seem - // to be an opportunity for any denial of service. - let caboose_magic_offset = image_end_offset.saturating_sub(caboose_size); + // A maliciously constructed image could be staged in flash + // with an apparently large caboose that would allow some access + // within its own flash slot. Presumably, we would never sign + // such an image so the bootloader would never execute it. + // However, reading out that image's caboose would be allowed. + // The range and size checks on caboose access are meant to keep + // accesses within the Hubris image which is constrained to its + // flash slot. + // There is no sensitive information to be found there. + let caboose_magic_offset = image_end_offset + .checked_sub(caboose_size) + .ok_or(RawCabooseError::MissingCaboose)?; if ((caboose_magic_offset % U32_SIZE) != 0) || !(IMAGE_HEADER_OFFSET..caboose_size_offset) .contains(&caboose_magic_offset) @@ -280,13 +292,14 @@ pub fn caboose_slice( } let caboose_magic = image - .read_word(caboose_magic_offset as u32) + .read_word(caboose_magic_offset) .map_err(|_| RawCabooseError::MissingCaboose)?; if caboose_magic == CABOOSE_MAGIC { - let caboose_range = ((caboose_magic_offset as u32) + U32_SIZE) - ..(caboose_size_offset as u32); - Ok(caboose_range) + let caboose_start = caboose_magic_offset + .checked_add(U32_SIZE) + .ok_or(RawCabooseError::MissingCaboose)?; + Ok(caboose_start..caboose_size_offset) } else { Err(RawCabooseError::MissingCaboose) } @@ -313,31 +326,28 @@ enum Accessor<'a> { } impl Accessor<'_> { - fn exec(&self) -> &Range { + fn at_runtime(&self) -> &Range { match self { - Accessor::Flash { flash: _, span } - | Accessor::Ram { buffer: _, span } - | Accessor::_Hybrid { - buffer: _, - flash: _, - span, - } => &span.exec, + Accessor::Flash { span, .. } + | Accessor::Ram { span, .. } + | Accessor::_Hybrid { span, .. } => &span.at_runtime, } } } -/// The update_server needs to deal with images that are: -/// - Entirely in flash -/// - Header block in RAM with the remainder unavailable -/// - Header block in RAM with the remainder in flash. -/// - Entire image in RAM (cached Stage0) -/// Calls to methods use image relative addresses. -/// ImageAccess::Flash{_, span} gives the physical flash addresses -/// being accessed. +/// In addition to images that are located in their respective +/// flash slots, the `update_server` needs to read data from +/// complete and partial images in RAM or split between RAM +/// and flash. +/// The specific cases are when the +/// - image is entirely in flash. +/// - header block is in RAM with the remainder unavailable. +/// - header block is in RAM with the remainder in flash. +/// - entire image is in RAM (in the case of a cached Stage0 image). /// -/// Any address here is the "storage" address which for stage0next -/// and any image held temporarily in RAM is different than the -/// execution address. +/// Calls to methods use offsets into the image which is helpful +/// when dealing with the offsets and sizes found in image headers +/// and the caboose. pub struct ImageAccess<'a> { accessor: Accessor<'a>, } @@ -381,65 +391,65 @@ impl ImageAccess<'_> { } } - fn exec(&self) -> &Range { - self.accessor.exec() + fn at_runtime(&self) -> &Range { + self.accessor.at_runtime() } - /// True if the u32 at offset is contained within the image. + /// True if the u32 at offset is contained within the slot. pub fn is_addressable(&self, offset: u32) -> bool { - let len = match &self.accessor { - Accessor::Flash { flash: _, span } - | Accessor::_Hybrid { - buffer: _, - flash: _, - span, - } - | Accessor::Ram { buffer: _, span } => { - span.store.end.saturating_sub(span.store.start) - } - }; - offset < len && offset.saturating_add(U32_SIZE) <= len + let len = self.at_runtime().len() as u32; + if let Some(end) = offset.checked_add(U32_SIZE) { + end <= len + } else { + false + } } - // Fetch a u32 from an image. + /// Fetch a u32 from an image. pub fn read_word(&self, offset: u32) -> Result { if !self.is_addressable(offset) { return Err(UpdateError::OutOfBounds); } match &self.accessor { Accessor::Flash { flash, span } => { - let addr = span.store.start.saturating_add(offset); + let addr = span + .stored + .start + .checked_add(offset) + .ok_or(UpdateError::OutOfBounds)?; let mut word = 0u32; indirect_flash_read(flash, addr, word.as_bytes_mut())?; Ok(word) } - Accessor::Ram { buffer, span: _ } => { - match buffer - .get(offset as usize..(offset as usize + SIZEOF_U32)) + Accessor::Ram { buffer, .. } => { + let word_end = (offset as usize) + .checked_add(SIZEOF_U32) + .ok_or(UpdateError::OutOfBounds)?; + Ok(buffer + .get(offset as usize..word_end) .and_then(u32::read_from) - { - Some(word) => Ok(word), - None => Err(UpdateError::OutOfBounds), - } + .ok_or(UpdateError::OutOfBounds)?) } Accessor::_Hybrid { buffer, flash, span, } => { - if offset < buffer.len() as u32 { - match buffer - .get(offset as usize..(offset as usize + SIZEOF_U32)) + if (offset as usize) < buffer.len() { + // Word is in the RAM portion + let word_end = (offset as usize) + .checked_add(SIZEOF_U32) + .ok_or(UpdateError::OutOfBounds)?; + Ok(buffer + .get(offset as usize..word_end) .and_then(u32::read_from) - { - Some(word) => Ok(word), - // Note: The case of a transfer spanning the RAM/Flash - // boundary would need to be unaligned given that the - // RAM portion must be whole u32-aligned pages. - None => Err(UpdateError::OutOfBounds), - } + .ok_or(UpdateError::OutOfBounds)?) } else { - let addr = span.store.start.saturating_add(offset); + let addr = span + .stored + .start + .checked_add(offset) + .ok_or(UpdateError::OutOfBounds)?; let mut word = 0u32; indirect_flash_read(flash, addr, word.as_bytes_mut())?; Ok(word) @@ -456,23 +466,25 @@ impl ImageAccess<'_> { let len = buffer.len() as u32; match &self.accessor { Accessor::Flash { flash, span } => { - let start = span.store.start.saturating_add(offset); - let end = start.saturating_add(len); - if span.store.contains(&start) - && (span.store.start..=span.store.end).contains(&end) + let start = span + .stored + .start + .checked_add(offset) + .ok_or(UpdateError::OutOfBounds)?; + let end = + start.checked_add(len).ok_or(UpdateError::OutOfBounds)?; + if span.stored.contains(&start) + && (span.stored.start..=span.stored.end).contains(&end) { Ok(indirect_flash_read(flash, start, buffer)?) } else { Err(UpdateError::OutOfBounds) } } - Accessor::Ram { - buffer: src, - span: _, - } => { - if let Some(data) = src.get( - (offset as usize)..(offset.saturating_add(len) as usize), - ) { + Accessor::Ram { buffer: src, .. } => { + let end = + offset.checked_add(len).ok_or(UpdateError::OutOfBounds)?; + if let Some(data) = src.get((offset as usize)..(end as usize)) { buffer.copy_from_slice(data); Ok(()) } else { @@ -484,25 +496,36 @@ impl ImageAccess<'_> { flash, span, } => { - let mut offset = offset as usize; + let mut start_offset = offset as usize; let mut remainder = buffer.len(); + let end_offset = start_offset + .checked_add(remainder) + .ok_or(UpdateError::OutOfBounds)?; // Transfer data from the RAM portion of the image - if offset < ram.len() { - let ram_end_offset = ram.len().min(offset + remainder); + if start_offset < ram.len() { + let ram_end_offset = ram.len().min(end_offset); // Transfer starts within the RAM part of this image. let data = ram - .get((offset)..ram_end_offset) + .get((start_offset)..ram_end_offset) .ok_or(UpdateError::OutOfBounds)?; buffer.copy_from_slice(data); - remainder -= data.len(); - offset = ram_end_offset; + remainder = remainder + .checked_sub(data.len()) + .ok_or(UpdateError::OutOfBounds)?; + start_offset = ram_end_offset; } // Transfer data from the flash-backed portion of the image. if remainder > 0 { - let start = span.store.start.saturating_add(offset as u32); - let end = start.saturating_add(remainder as u32); - if span.store.contains(&start) - && (span.store.start..=span.store.end).contains(&end) + let start = span + .stored + .start + .checked_add(start_offset as u32) + .ok_or(UpdateError::OutOfBounds)?; + let end = start + .checked_add(remainder as u32) + .ok_or(UpdateError::OutOfBounds)?; + if span.stored.contains(&start) + && (span.stored.start..=span.stored.end).contains(&end) { indirect_flash_read(flash, start, buffer)?; } else { @@ -524,13 +547,10 @@ impl ImageAccess<'_> { ImageVectorsLpc55::read_from_prefix(&buffer[..]) .ok_or(UpdateError::OutOfBounds) } - Accessor::Ram { buffer, span: _ } - | Accessor::_Hybrid { - buffer, - flash: _, - span: _, - } => ImageVectorsLpc55::read_from_prefix(buffer) - .ok_or(UpdateError::OutOfBounds), + Accessor::Ram { buffer, .. } | Accessor::_Hybrid { buffer, .. } => { + ImageVectorsLpc55::read_from_prefix(buffer) + .ok_or(UpdateError::OutOfBounds) + } }?; let len = vectors.image_length().ok_or(UpdateError::BadLength)?; round_up_to_flash_page(len).ok_or(UpdateError::BadLength) diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 48d9f4a33..5a0041c3f 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -62,7 +62,7 @@ enum Trace { ringbuf!(Trace, 16, Trace::None); -/// FW_CACHE_MAX accomodates the largest production +/// FW_CACHE_MAX accommodates the largest production /// bootloader image while allowing some room for growth. /// /// NOTE: The erase/flash of stage0 can be interrupted by a power failure or @@ -181,7 +181,7 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { return Err(UpdateError::BadLength.into()); } - // Match the behvaior of the CMSIS flash driver where erased bytes are + // Match the behavior of the CMSIS flash driver where erased bytes are // read as 0xff so the image is padded with 0xff const ERASE_BYTE: u8 = 0xff; let mut flash_page = [ERASE_BYTE; BLOCK_SIZE_BYTES]; @@ -260,7 +260,7 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { // Now erase the unused portion of the flash slot so that // flash slot has predictable contents and the FWID for it // has some meaning. - let range = &flash_range(component, slot).store; + let range = &flash_range(component, slot).stored; let erase_start = range.start + (endblock as u32 * PAGE_SIZE); self.flash_erase_range(erase_start..range.end)?; self.state = UpdateState::Finished; @@ -277,7 +277,7 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { } // TODO(AJS): Remove this in favor of `status`, once SP code is updated. - // This has ripple effects up thorugh control-plane-agent. + // This has ripple effects up through control-plane-agent. fn current_version( &mut self, _: &RecvMessage, @@ -457,8 +457,11 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { ) -> Result> { let image = ImageAccess::new_flash(&self.flash, component, slot); let caboose = caboose_slice(&image)?; - let caboose_len = caboose.end.saturating_sub(caboose.start); - Ok(caboose_len) + if let Some(caboose_len) = caboose.end.checked_sub(caboose.start) { + Ok(caboose_len) + } else { + Err(RawCabooseError::MissingCaboose.into()) + } } fn component_read_raw_caboose( @@ -471,7 +474,9 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { ) -> Result<(), idol_runtime::RequestError> { let image = ImageAccess::new_flash(&self.flash, component, slot); let caboose = caboose_slice(&image)?; - let caboose_len = caboose.end.saturating_sub(caboose.start); + let Some(caboose_len) = caboose.end.checked_sub(caboose.start) else { + return Err(RawCabooseError::MissingCaboose.into()); + }; if offset as usize + data.len() > (caboose_len as usize) { return Err(RawCabooseError::InvalidRead.into()); } @@ -686,7 +691,7 @@ impl ServerImpl<'_> { // through 3) to ensure that RoT image signatures are valid before any // system continues to step 4. // - // TBD: While Failures up to step 3 do not adversly affect the RoT, + // TBD: While Failures up to step 3 do not adversely affect the RoT, // resetting the RoT to evaluate signatures may be service affecting // to the system depending on how the RoT and SP interact with respect // to their reset handling and the RoT measurement of the SP. @@ -697,7 +702,7 @@ impl ServerImpl<'_> { // updating stage0next to the original stage0 contents that were // validated reset and then copying those to stage0. // - // It is assumed that a hash collision is not computaionally feasible + // It is assumed that a hash collision is not computationally feasible // for either the image hash done by rot-startup or used by the ROM // signature routine. @@ -760,7 +765,7 @@ impl ServerImpl<'_> { // Finish by erasing the unused portion of flash bank. // An error here means that the stage0 slot may not be clean but at least // it has the intended bootloader written. - let stage0 = &flash_range(RotComponent::Stage0, SlotId::A).store; + let stage0 = &flash_range(RotComponent::Stage0, SlotId::A).stored; if let Some(erase_start) = stage0.start.checked_add(len as u32) { self.flash_erase_range(erase_start..stage0.end)?; } @@ -818,10 +823,11 @@ impl ServerImpl<'_> { stage0 .read_bytes(addr, flash_page.as_bytes_mut()) .map_err(|_| UpdateError::ImageMismatch)?; - if flash_page - != cached[(addr as usize) - ..(addr as usize).saturating_add(BYTES_PER_FLASH_PAGE)] - { + let Some(end) = (addr as usize).checked_add(BYTES_PER_FLASH_PAGE) + else { + return Err(UpdateError::ImageMismatch); + }; + if flash_page != cached[(addr as usize)..end] { return Err(UpdateError::ImageMismatch); } } @@ -834,7 +840,8 @@ impl ServerImpl<'_> { self.fw_cache[0..BLOCK_SIZE_BYTES].as_bytes(), ) { // Get the length of the image iff it fits in its ultimate destination. - let exec_range = &flash_range(RotComponent::Stage0, SlotId::A).exec; + let exec_range = + &flash_range(RotComponent::Stage0, SlotId::A).at_runtime; if let Some(image) = vectors.padded_image_range(exec_range) { return Ok(image.len()); } @@ -865,7 +872,10 @@ impl ServerImpl<'_> { return Err(UpdateError::BadLength); } let span = &flash_range(RotComponent::Stage0, SlotId::A); - if span.store.end < span.store.start.saturating_add(clen as u32) { + let Some(end) = span.stored.start.checked_add(clen as u32) else { + return Err(UpdateError::BadLength); + }; + if span.stored.end < end { return Err(UpdateError::BadLength); } // Sanity check could be repeated here. @@ -1174,7 +1184,7 @@ fn target_addr( slot: SlotId, page_num: u32, ) -> Option { - let range = &flash_range(component, slot).store; + let range = &flash_range(component, slot).stored; // This is safely calculating addr = base + page_num * PAGE_SIZE let addr = page_num @@ -1212,8 +1222,12 @@ fn copy_from_caboose_chunk_to_lease( .map_err(|_| RequestError::from(RawCabooseError::ReadFailed))?; data.write_range(offset as usize..(offset + count) as usize, buf) .map_err(|_| RequestError::Fail(ClientError::WentAway))?; - offset = offset.saturating_add(count); - remaining = remaining.saturating_sub(count); + offset = offset + .checked_add(count) + .ok_or(RawCabooseError::ReadFailed)?; + remaining = remaining + .checked_sub(count) + .ok_or(RawCabooseError::ReadFailed)?; } Ok(()) } From e6be5ce201c615d846dcd69e4126747f297ec96b Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Sat, 29 Jun 2024 19:06:35 -0700 Subject: [PATCH 4/6] Implement epoch-based rollback protection in LPC55 update_server --- Cargo.lock | 2 +- app/gimlet/base.toml | 1 + app/gimletlet/app-mgmt.toml | 2 +- app/lpc55xpresso/app.toml | 2 +- app/oxide-rot-1/app-dev.toml | 2 +- drv/lpc55-update-api/src/lib.rs | 10 ++ drv/lpc55-update-server/src/images.rs | 158 +++++++++++++++++++++++--- drv/lpc55-update-server/src/main.rs | 27 +++-- drv/update-api/src/lib.rs | 2 + 9 files changed, 177 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3bf1cf0cf..90c70dba6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2679,7 +2679,7 @@ checksum = "e6d5a32815ae3f33302d95fdcb2ce17862f8c65363dcfd29360480ba1001fc9c" [[package]] name = "gateway-messages" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/management-gateway-service#c8abf88bc0aeb3aa411eea276c73abdea405f7ba" +source = "git+https://github.com/oxidecomputer/management-gateway-service#32d875c31325f6fe1ab389c5564439ba04889b7c" dependencies = [ "bitflags 2.6.0", "hubpack", diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 3b66000a7..3997a53f9 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -3,6 +3,7 @@ chip = "../../chips/stm32h7" memory = "memory-large.toml" stacksize = 896 fwid = true +epoch = 0 [kernel] name = "gimlet" diff --git a/app/gimletlet/app-mgmt.toml b/app/gimletlet/app-mgmt.toml index 7dd5019ec..1ea187a92 100644 --- a/app/gimletlet/app-mgmt.toml +++ b/app/gimletlet/app-mgmt.toml @@ -49,7 +49,7 @@ task-slots = ["sys", "user_leds"] [tasks.net] name = "task-net" -stacksize = 3000 +stacksize = 8000 priority = 3 features = ["mgmt", "h753", "use-spi-core", "spi2"] max-sizes = {flash = 131072, ram = 16384, sram1_mac = 16384} diff --git a/app/lpc55xpresso/app.toml b/app/lpc55xpresso/app.toml index b6abfe0ab..14426026d 100644 --- a/app/lpc55xpresso/app.toml +++ b/app/lpc55xpresso/app.toml @@ -48,7 +48,7 @@ start = true [tasks.update_server] name = "lpc55-update-server" priority = 3 -max-sizes = {flash = 27008, ram = 16704} +max-sizes = {flash = 30368, ram = 16704} stacksize = 8192 start = true sections = {bootstate = "usbsram"} diff --git a/app/oxide-rot-1/app-dev.toml b/app/oxide-rot-1/app-dev.toml index c5b2d422d..c7ba72312 100644 --- a/app/oxide-rot-1/app-dev.toml +++ b/app/oxide-rot-1/app-dev.toml @@ -53,7 +53,7 @@ start = true [tasks.update_server] name = "lpc55-update-server" priority = 3 -max-sizes = {flash = 27904, ram = 17344, usbsram = 4096} +max-sizes = {flash = 30368, ram = 17344, usbsram = 4096} # TODO: Size this appropriately stacksize = 8192 start = true diff --git a/drv/lpc55-update-api/src/lib.rs b/drv/lpc55-update-api/src/lib.rs index c85366998..62da0b2f2 100644 --- a/drv/lpc55-update-api/src/lib.rs +++ b/drv/lpc55-update-api/src/lib.rs @@ -242,6 +242,16 @@ impl From for SlotId { } } +impl SlotId { + pub fn other(&self) -> SlotId { + if *self == SlotId::A { + SlotId::B + } else { + SlotId::A + } + } +} + impl TryFrom for SlotId { type Error = (); fn try_from(i: u16) -> Result { diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index 49d66210e..e859e5589 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -20,7 +20,10 @@ use crate::{ use abi::{ImageHeader, CABOOSE_MAGIC, HEADER_MAGIC}; use core::ops::Range; use core::ptr::addr_of; -use drv_lpc55_update_api::{RawCabooseError, RotComponent, SlotId}; +use drv_caboose::CabooseReader; +use drv_lpc55_update_api::{ + RawCabooseError, RotComponent, SlotId, BLOCK_SIZE_BYTES, +}; use drv_update_api::UpdateError; use zerocopy::{AsBytes, FromBytes}; @@ -47,6 +50,7 @@ pub const HEADER_BLOCK: usize = 0; // 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']; /// Address ranges that may contain an image during storage and active use. /// `stored` and `at_runtime` ranges are the same except for `stage0next`. @@ -99,8 +103,7 @@ pub fn is_current_hubris_image(component: RotComponent, slot: SlotId) -> bool { // Safety: extern statics aren't controlled by Rust so poking them can // cause UB; in this case, it's zero length and we are only taking its // numerical address, so we're not at risk. - flash_range(component, slot).stored.start - == unsafe { addr_of!(__this_image) as u32 } + flash_range(component, slot).stored.start == addr_of!(__this_image) as u32 } // LPC55 defined image content @@ -181,7 +184,7 @@ impl TryFrom<&[u8]> for ImageVectorsLpc55 { /// the end of optional caboose and the beginning of the signature block. pub fn validate_header_block( header_access: &ImageAccess<'_>, -) -> Result { +) -> Result<(Option, u32), UpdateError> { let mut vectors = ImageVectorsLpc55::new_zeroed(); let mut header = ImageHeader::new_zeroed(); @@ -208,15 +211,17 @@ pub fn validate_header_block( // Note that `ImageHeader.epoch` is used by rollback protection for early // rejection of invalid images. // TODO: Improve estimate of where the first executable instruction can be. - let code_offset = if header.magic == HEADER_MAGIC { + let (code_offset, epoch) = if header.magic == HEADER_MAGIC { if header.total_image_len != vectors.nxp_offset_to_specific_header { // ImageHeader disagrees with LPC55 vectors. return Err(UpdateError::InvalidHeaderBlock); } - // Adding constants should be resolved at compile time: no call to panic. - IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32) + ( + IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32), + Some(Epoch::from(header.epoch)), + ) } else { - IMAGE_HEADER_OFFSET + (IMAGE_HEADER_OFFSET, None) }; if vectors.nxp_image_length as usize > header_access.at_runtime().len() { @@ -243,7 +248,7 @@ pub fn validate_header_block( return Err(UpdateError::InvalidHeaderBlock); } - Ok(vectors.nxp_offset_to_specific_header) + Ok((epoch, vectors.nxp_offset_to_specific_header)) } /// Get the range of the caboose contained within an image if it exists. @@ -260,7 +265,7 @@ pub fn caboose_slice( // // In this context, NoImageHeader actually means that the image // is not well formed. - let image_end_offset = validate_header_block(image) + let (_epoch, image_end_offset) = validate_header_block(image) .map_err(|_| RawCabooseError::NoImageHeader)?; // By construction, the last word of the caboose is its size as a `u32` @@ -318,7 +323,7 @@ enum Accessor<'a> { }, // Hybrid is used for later implementation of rollback protection. // The buffer is used in place of the beginning of the flash range. - _Hybrid { + Hybrid { buffer: &'a [u8], flash: &'a drv_lpc55_flash::Flash<'a>, span: FlashRange, @@ -330,7 +335,7 @@ impl Accessor<'_> { match self { Accessor::Flash { span, .. } | Accessor::Ram { span, .. } - | Accessor::_Hybrid { span, .. } => &span.at_runtime, + | Accessor::Hybrid { span, .. } => &span.at_runtime, } } } @@ -375,7 +380,7 @@ impl ImageAccess<'_> { } } - pub fn _new_hybrid<'a>( + pub fn new_hybrid<'a>( flash: &'a drv_lpc55_flash::Flash<'a>, buffer: &'a [u8], component: RotComponent, @@ -383,7 +388,7 @@ impl ImageAccess<'_> { ) -> ImageAccess<'a> { let span = flash_range(component, slot); ImageAccess { - accessor: Accessor::_Hybrid { + accessor: Accessor::Hybrid { flash, buffer, span, @@ -430,7 +435,7 @@ impl ImageAccess<'_> { .and_then(u32::read_from) .ok_or(UpdateError::OutOfBounds)?) } - Accessor::_Hybrid { + Accessor::Hybrid { buffer, flash, span, @@ -491,7 +496,7 @@ impl ImageAccess<'_> { Err(UpdateError::OutOfBounds) } } - Accessor::_Hybrid { + Accessor::Hybrid { buffer: ram, flash, span, @@ -547,7 +552,7 @@ impl ImageAccess<'_> { ImageVectorsLpc55::read_from_prefix(&buffer[..]) .ok_or(UpdateError::OutOfBounds) } - Accessor::Ram { buffer, .. } | Accessor::_Hybrid { buffer, .. } => { + Accessor::Ram { buffer, .. } | Accessor::Hybrid { buffer, .. } => { ImageVectorsLpc55::read_from_prefix(buffer) .ok_or(UpdateError::OutOfBounds) } @@ -556,3 +561,122 @@ impl ImageAccess<'_> { round_up_to_flash_page(len).ok_or(UpdateError::BadLength) } } + +#[derive(Clone, PartialEq)] +pub struct Epoch { + value: u32, +} + +/// Convert from the ImageHeader.epoch format +impl From for Epoch { + fn from(number: u32) -> Self { + Epoch { value: number } + } +} + +/// Convert from the caboose EPOC value format. +// +// Invalid EPOC values converted to Epoch{value:0} include: +// - empty slice +// - any non-ASCII-digits in slice +// - any non-UTF8 in slice +// - leading '+' normally allowed by parse() +// - values greater than u32::MAX +// +// Hand coding reduces size by about 950 bytes. +impl From<&[u8]> for Epoch { + fn from(chars: &[u8]) -> Self { + let epoch = + if chars.first().map(|c| c.is_ascii_digit()).unwrap_or(false) { + if let Ok(chars) = core::str::from_utf8(chars) { + chars.parse::().unwrap_or(0) + } else { + 0 + } + } else { + 0 + }; + Epoch::from(epoch) + } +} + +impl Epoch { + pub fn can_update_to(&self, next: Epoch) -> bool { + self.value >= next.value + } +} + +/// Check a next image against an active image to determine +/// if rollback policy allows the next image. +/// If ImageHeader and Caboose are absent or Caboose does +/// not have an `EPOC` tag, then the Epoch is defaulted to zero. +/// 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( + next_image: ImageAccess<'_>, + active_image: ImageAccess<'_>, + complete: bool, +) -> Result<(), UpdateError> { + let next_epoch = get_image_epoch(&next_image)?; + let active_epoch = get_image_epoch(&active_image)?; + match (active_epoch, next_epoch) { + // No active_epoch is treated as zero; update can proceed. + (None, _) => Ok(()), + (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)) { + Ok(()) + } else { + Err(UpdateError::RollbackProtection) + } + } + (Some(active_epoch), Some(next_epoch)) => { + if active_epoch.can_update_to(next_epoch) { + Ok(()) + } else { + Err(UpdateError::RollbackProtection) + } + } + } +} + +/// Get ImageHeader epoch and/or caboose EPOC from an Image if it exists. +/// Return default of zero epoch if neither is present. +/// This function is called at points where the image's signature has not been +/// checked or the image is incomplete. Sanity checks are required before using +/// any data. +fn get_image_epoch( + image: &ImageAccess<'_>, +) -> Result, UpdateError> { + let (header_epoch, _caboose_offset) = validate_header_block(image)?; + + if let Ok(span) = caboose_slice(image) { + let mut block = [0u8; BLOCK_SIZE_BYTES]; + let caboose = block[0..span.len()].as_bytes_mut(); + image.read_bytes(span.start, caboose)?; + let reader = CabooseReader::new(caboose); + let caboose_epoch = if let Ok(epoc) = reader.get(CABOOSE_TAG_EPOC) { + Some(Epoch::from(epoc)) + } else { + None + }; + match (header_epoch, caboose_epoch) { + (None, None) => Ok(None), + (Some(header_epoch), None) => Ok(Some(header_epoch)), + (None, Some(caboose_epoch)) => Ok(Some(caboose_epoch)), + (Some(header_epoch), Some(caboose_epoch)) => { + if caboose_epoch == header_epoch { + Ok(Some(caboose_epoch)) + } else { + // Epochs present in both and not matching is invalid. + // The image will be rejected after epoch 0. + Ok(Some(Epoch::from(0u32))) + } + } + } + } else { + Ok(header_epoch) + } +} diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 5a0041c3f..5e63dc28e 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -9,7 +9,9 @@ #![no_std] #![no_main] -use crate::images::{validate_header_block, ImageVectorsLpc55}; +use crate::images::{ + check_rollback_policy, validate_header_block, ImageVectorsLpc55, +}; use core::convert::Infallible; use core::mem::{size_of, MaybeUninit}; use core::ops::Range; @@ -200,6 +202,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { self.header_block = None; return Err(e.into()); } + let active_image = + ImageAccess::new_flash(&self.flash, component, slot.other()); + if let Err(e) = + check_rollback_policy(active_image, next_image, false) + { + self.header_block = None; + return Err(e.into()); + } } else { // Block order is enforced above. If we're here then we have // seen block zero already. @@ -249,13 +259,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { } let (component, slot) = self.image.unwrap_lite(); - do_block_write( - &mut self.flash, - component, - slot, - HEADER_BLOCK, - self.header_block.as_ref().unwrap_lite(), - )?; + let buffer = self.header_block.as_ref().unwrap_lite(); + let next_image = + ImageAccess::new_hybrid(&self.flash, buffer, component, slot); + let active_image = + ImageAccess::new_flash(&self.flash, component, slot.other()); + check_rollback_policy(active_image, next_image, true)?; + + do_block_write(&mut self.flash, component, slot, HEADER_BLOCK, buffer)?; // Now erase the unused portion of the flash slot so that // flash slot has predictable contents and the FWID for it diff --git a/drv/update-api/src/lib.rs b/drv/update-api/src/lib.rs index 4f77a7333..ecebe76ee 100644 --- a/drv/update-api/src/lib.rs +++ b/drv/update-api/src/lib.rs @@ -65,6 +65,7 @@ pub enum UpdateError { ImageMismatch, SignatureNotValidated, VersionNotSupported, + RollbackProtection, } impl From for GwUpdateError { @@ -103,6 +104,7 @@ impl From for GwUpdateError { UpdateError::ImageMismatch => Self::ImageMismatch, UpdateError::SignatureNotValidated => Self::SignatureNotValidated, UpdateError::VersionNotSupported => Self::VersionNotSupported, + UpdateError::RollbackProtection => Self::RollbackProtection, } } } From 6f5d861f5774990aaa07720138e87edccaaf0cd4 Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Fri, 9 Aug 2024 13:27:36 -0700 Subject: [PATCH 5/6] deprecate ImageHeader version and epoch --- Cargo.lock | 4 +-- app/gimlet/base.toml | 1 - app/gimletlet/base-gimletlet2.toml | 2 -- app/grapefruit/app.toml | 2 -- app/oxide-rot-1/app-dev.toml | 2 -- app/oxide-rot-1/app.toml | 2 -- app/rot-carrier/app.toml | 2 -- build/xtask/src/config.rs | 13 --------- build/xtask/src/dist.rs | 12 ++------- drv/lpc55-update-server/build.rs | 13 --------- drv/lpc55-update-server/src/images.rs | 39 +++++++-------------------- drv/lpc55-update-server/src/main.rs | 6 ++--- drv/sprot-api/README.md | 2 +- drv/stm32h7-update-server/build.rs | 13 --------- drv/stm32h7-update-server/src/main.rs | 6 ++--- sys/abi/src/lib.rs | 14 ++++++++-- 16 files changed, 33 insertions(+), 100 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 90c70dba6..4b2dde874 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2953,8 +2953,8 @@ dependencies = [ [[package]] name = "hubtools" -version = "0.4.6" -source = "git+https://github.com/oxidecomputer/hubtools#943c4bbe6b50d1ab635d085d6204895fb4154e79" +version = "0.4.7" +source = "git+https://github.com/oxidecomputer/hubtools#d38e22464c3991ffad8754020ad0dd533b7ccdde" dependencies = [ "hex", "lpc55_areas", diff --git a/app/gimlet/base.toml b/app/gimlet/base.toml index 3997a53f9..3b66000a7 100644 --- a/app/gimlet/base.toml +++ b/app/gimlet/base.toml @@ -3,7 +3,6 @@ chip = "../../chips/stm32h7" memory = "memory-large.toml" stacksize = 896 fwid = true -epoch = 0 [kernel] name = "gimlet" diff --git a/app/gimletlet/base-gimletlet2.toml b/app/gimletlet/base-gimletlet2.toml index 677522298..7f7122007 100644 --- a/app/gimletlet/base-gimletlet2.toml +++ b/app/gimletlet/base-gimletlet2.toml @@ -3,8 +3,6 @@ target = "thumbv7em-none-eabihf" chip = "../../chips/stm32h7" memory = "memory-large.toml" stacksize = 896 -epoch = 0 -version = 0 fwid = true [kernel] diff --git a/app/grapefruit/app.toml b/app/grapefruit/app.toml index 0bf0e79a9..f7e8b9993 100644 --- a/app/grapefruit/app.toml +++ b/app/grapefruit/app.toml @@ -4,8 +4,6 @@ target = "thumbv7em-none-eabihf" chip = "../../chips/stm32h7" memory = "memory-large.toml" stacksize = 896 -epoch = 0 -version = 0 fwid = true [kernel] diff --git a/app/oxide-rot-1/app-dev.toml b/app/oxide-rot-1/app-dev.toml index c7ba72312..65ca146cc 100644 --- a/app/oxide-rot-1/app-dev.toml +++ b/app/oxide-rot-1/app-dev.toml @@ -8,8 +8,6 @@ board = "oxide-rot-1-selfsigned" chip = "../../chips/lpc55" stacksize = 1024 image-names = ["a", "b"] -epoch = 0 -version = 0 fwid = true [kernel] diff --git a/app/oxide-rot-1/app.toml b/app/oxide-rot-1/app.toml index 41a05d564..7d76c2033 100644 --- a/app/oxide-rot-1/app.toml +++ b/app/oxide-rot-1/app.toml @@ -5,8 +5,6 @@ board = "oxide-rot-1" chip = "../../chips/lpc55" stacksize = 1024 image-names = ["a", "b"] -epoch = 0 -version = 0 fwid = true [kernel] diff --git a/app/rot-carrier/app.toml b/app/rot-carrier/app.toml index f1182218d..b9f222b11 100644 --- a/app/rot-carrier/app.toml +++ b/app/rot-carrier/app.toml @@ -4,8 +4,6 @@ board = "rot-carrier-2" chip = "../../chips/lpc55" stacksize = 1024 image-names = ["a", "b"] -epoch = 0 -version = 0 fwid = true [kernel] diff --git a/build/xtask/src/config.rs b/build/xtask/src/config.rs index 351b515d5..82997c515 100644 --- a/build/xtask/src/config.rs +++ b/build/xtask/src/config.rs @@ -24,10 +24,6 @@ struct RawConfig { board: String, chip: String, #[serde(default)] - epoch: u32, - #[serde(default)] - version: u32, - #[serde(default)] fwid: bool, memory: Option, #[serde(default)] @@ -50,8 +46,6 @@ pub struct Config { pub target: String, pub board: String, pub chip: String, - pub epoch: u32, - pub version: u32, pub fwid: bool, pub image_names: Vec, pub signing: Option, @@ -174,8 +168,6 @@ impl Config { board: toml.board, image_names: img_names, chip: toml.chip, - epoch: toml.epoch, - version: toml.version, fwid: toml.fwid, signing: toml.signing, stacksize: toml.stacksize, @@ -254,11 +246,6 @@ impl Config { let task_names = self.tasks.keys().cloned().collect::>().join(","); env.insert("HUBRIS_TASKS".to_string(), task_names); - env.insert( - "HUBRIS_BUILD_VERSION".to_string(), - format!("{}", self.version), - ); - env.insert("HUBRIS_BUILD_EPOCH".to_string(), format!("{}", self.epoch)); env.insert("HUBRIS_BOARD".to_string(), self.board.to_string()); env.insert( "HUBRIS_APP_TOML".to_string(), diff --git a/build/xtask/src/dist.rs b/build/xtask/src/dist.rs index 8b0ef6ca4..f3ffa2b78 100644 --- a/build/xtask/src/dist.rs +++ b/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, input: &Path, output: &Path, map: &IndexMap>, @@ -1606,16 +1606,8 @@ fn update_image_header( // `xtask build kernel`, we need a result from this calculation // but `end` will be `None`. Substitute a placeholder: let end = end.unwrap_or(flash.start); - let len = end - flash.start; - - let header = abi::ImageHeader { - version: cfg.toml.version, - epoch: cfg.toml.epoch, - magic: abi::HEADER_MAGIC, - total_image_len: len, - ..Default::default() - }; + let header = abi::ImageHeader::new(len); header .write_to_prefix( diff --git a/drv/lpc55-update-server/build.rs b/drv/lpc55-update-server/build.rs index 05130a837..623f6553b 100644 --- a/drv/lpc55-update-server/build.rs +++ b/drv/lpc55-update-server/build.rs @@ -2,9 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use std::fs::File; -use std::io::Write; - fn main() -> Result<(), Box> { build_util::expose_target_board(); build_util::build_notifications()?; @@ -18,15 +15,5 @@ fn main() -> Result<(), Box> { "server_stub.rs", idol::server::ServerStyle::InOrder, )?; - - let out = build_util::out_dir(); - let mut ver_file = File::create(out.join("consts.rs")).unwrap(); - - let version: u32 = build_util::env_var("HUBRIS_BUILD_VERSION")?.parse()?; - let epoch: u32 = build_util::env_var("HUBRIS_BUILD_EPOCH")?.parse()?; - - writeln!(ver_file, "const HUBRIS_BUILD_VERSION: u32 = {};", version)?; - writeln!(ver_file, "const HUBRIS_BUILD_EPOCH: u32 = {};", epoch)?; - Ok(()) } diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index e859e5589..cdc7e0e65 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -184,7 +184,7 @@ impl TryFrom<&[u8]> for ImageVectorsLpc55 { /// the end of optional caboose and the beginning of the signature block. pub fn validate_header_block( header_access: &ImageAccess<'_>, -) -> Result<(Option, u32), UpdateError> { +) -> Result { let mut vectors = ImageVectorsLpc55::new_zeroed(); let mut header = ImageHeader::new_zeroed(); @@ -211,17 +211,14 @@ pub fn validate_header_block( // Note that `ImageHeader.epoch` is used by rollback protection for early // rejection of invalid images. // TODO: Improve estimate of where the first executable instruction can be. - let (code_offset, epoch) = if header.magic == HEADER_MAGIC { + let code_offset = if header.magic == HEADER_MAGIC { if header.total_image_len != vectors.nxp_offset_to_specific_header { // ImageHeader disagrees with LPC55 vectors. return Err(UpdateError::InvalidHeaderBlock); } - ( - IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32), - Some(Epoch::from(header.epoch)), - ) + IMAGE_HEADER_OFFSET + (core::mem::size_of::() as u32) } else { - (IMAGE_HEADER_OFFSET, None) + IMAGE_HEADER_OFFSET }; if vectors.nxp_image_length as usize > header_access.at_runtime().len() { @@ -248,7 +245,7 @@ pub fn validate_header_block( return Err(UpdateError::InvalidHeaderBlock); } - Ok((epoch, vectors.nxp_offset_to_specific_header)) + Ok(vectors.nxp_offset_to_specific_header) } /// Get the range of the caboose contained within an image if it exists. @@ -265,7 +262,7 @@ pub fn caboose_slice( // // In this context, NoImageHeader actually means that the image // is not well formed. - let (_epoch, image_end_offset) = validate_header_block(image) + let image_end_offset = validate_header_block(image) .map_err(|_| RawCabooseError::NoImageHeader)?; // By construction, the last word of the caboose is its size as a `u32` @@ -650,33 +647,17 @@ pub fn check_rollback_policy( fn get_image_epoch( image: &ImageAccess<'_>, ) -> Result, UpdateError> { - let (header_epoch, _caboose_offset) = validate_header_block(image)?; - if let Ok(span) = caboose_slice(image) { let mut block = [0u8; BLOCK_SIZE_BYTES]; let caboose = block[0..span.len()].as_bytes_mut(); image.read_bytes(span.start, caboose)?; let reader = CabooseReader::new(caboose); - let caboose_epoch = if let Ok(epoc) = reader.get(CABOOSE_TAG_EPOC) { - Some(Epoch::from(epoc)) + if let Ok(epoc) = reader.get(CABOOSE_TAG_EPOC) { + Ok(Some(Epoch::from(epoc))) } else { - None - }; - match (header_epoch, caboose_epoch) { - (None, None) => Ok(None), - (Some(header_epoch), None) => Ok(Some(header_epoch)), - (None, Some(caboose_epoch)) => Ok(Some(caboose_epoch)), - (Some(header_epoch), Some(caboose_epoch)) => { - if caboose_epoch == header_epoch { - Ok(Some(caboose_epoch)) - } else { - // Epochs present in both and not matching is invalid. - // The image will be rejected after epoch 0. - Ok(Some(Epoch::from(0u32))) - } - } + Ok(None) } } else { - Ok(header_epoch) + Ok(None) } } diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 5e63dc28e..2e3d5a476 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -289,13 +289,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { // TODO(AJS): Remove this in favor of `status`, once SP code is updated. // This has ripple effects up through control-plane-agent. + /// Deprecated. The version and epoch are in the Caboose fn current_version( &mut self, _: &RecvMessage, ) -> Result> { Ok(ImageVersion { - epoch: HUBRIS_BUILD_EPOCH, - version: HUBRIS_BUILD_VERSION, + epoch: 0, + version: 0, }) } @@ -1312,7 +1313,6 @@ fn main() -> ! { } } -include!(concat!(env!("OUT_DIR"), "/consts.rs")); include!(concat!(env!("OUT_DIR"), "/notifications.rs")); mod idl { use super::{ diff --git a/drv/sprot-api/README.md b/drv/sprot-api/README.md index 3d17d8801..98194b403 100644 --- a/drv/sprot-api/README.md +++ b/drv/sprot-api/README.md @@ -461,7 +461,7 @@ SpRot.status() => Status { } ``` -Update API, retrieve current version. +Update API, retrieve current version (deprecated) This information is redundant with information in the Status structure. ```sh $ humility hiffy -c SpRot.current_version diff --git a/drv/stm32h7-update-server/build.rs b/drv/stm32h7-update-server/build.rs index b5eef4730..da633af33 100644 --- a/drv/stm32h7-update-server/build.rs +++ b/drv/stm32h7-update-server/build.rs @@ -2,9 +2,6 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use std::fs::File; -use std::io::Write; - fn main() -> Result<(), Box> { build_util::build_notifications()?; idol::Generator::new() @@ -16,15 +13,5 @@ fn main() -> Result<(), Box> { "server_stub.rs", idol::server::ServerStyle::InOrder, )?; - - let out = build_util::out_dir(); - let mut ver_file = File::create(out.join("consts.rs")).unwrap(); - - let version: u32 = build_util::env_var("HUBRIS_BUILD_VERSION")?.parse()?; - let epoch: u32 = build_util::env_var("HUBRIS_BUILD_EPOCH")?.parse()?; - - writeln!(ver_file, "const HUBRIS_BUILD_VERSION: u32 = {};", version)?; - writeln!(ver_file, "const HUBRIS_BUILD_EPOCH: u32 = {};", epoch)?; - Ok(()) } diff --git a/drv/stm32h7-update-server/src/main.rs b/drv/stm32h7-update-server/src/main.rs index ce7a73fea..353d750ea 100644 --- a/drv/stm32h7-update-server/src/main.rs +++ b/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. fn current_version( &mut self, _: &RecvMessage, ) -> Result> { Ok(ImageVersion { - epoch: HUBRIS_BUILD_EPOCH, - version: HUBRIS_BUILD_VERSION, + epoch: 0, + version: 0, }) } @@ -569,7 +570,6 @@ fn main() -> ! { } } -include!(concat!(env!("OUT_DIR"), "/consts.rs")); mod idl { use super::{CabooseError, ImageVersion, SlotId}; diff --git a/sys/abi/src/lib.rs b/sys/abi/src/lib.rs index 4563b4b43..79958cbe6 100644 --- a/sys/abi/src/lib.rs +++ b/sys/abi/src/lib.rs @@ -521,8 +521,18 @@ pub struct ImageHeader { pub magic: u32, pub total_image_len: u32, pub _pad: [u32; 16], // previous location of SAU entries - pub version: u32, - pub epoch: u32, + pub _version: u32, + pub _epoch: u32, +} + +impl ImageHeader { + pub fn new(total_image_len: u32) -> Self { + ImageHeader { + magic: HEADER_MAGIC, + total_image_len, + ..Default::default() + } + } } // Corresponds to the ARM vector table, limited to what we need From aca51403193089c9d4200b3d838aac521abe93d4 Mon Sep 17 00:00:00 2001 From: Ben Stoltz Date: Thu, 10 Oct 2024 13:29:27 -0700 Subject: [PATCH 6/6] Revert to hubtools 0.4.6, don't build with a default epoch value. 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. --- Cargo.lock | 4 ++-- drv/lpc55-update-server/src/images.rs | 4 ++-- drv/lpc55-update-server/src/main.rs | 8 -------- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4b2dde874..b3defb3a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2953,8 +2953,8 @@ dependencies = [ [[package]] name = "hubtools" -version = "0.4.7" -source = "git+https://github.com/oxidecomputer/hubtools#d38e22464c3991ffad8754020ad0dd533b7ccdde" +version = "0.4.6" +source = "git+https://github.com/oxidecomputer/hubtools#f48e2da029ba6552cff5c07ff8a2fc21cc56aa32" dependencies = [ "hex", "lpc55_areas", diff --git a/drv/lpc55-update-server/src/images.rs b/drv/lpc55-update-server/src/images.rs index cdc7e0e65..4de325522 100644 --- a/drv/lpc55-update-server/src/images.rs +++ b/drv/lpc55-update-server/src/images.rs @@ -599,7 +599,7 @@ impl From<&[u8]> for Epoch { impl Epoch { pub fn can_update_to(&self, next: Epoch) -> bool { - self.value >= next.value + self.value <= next.value } } @@ -611,8 +611,8 @@ impl Epoch { /// so that images can be rejected early, i.e. before any flash has /// been altered. pub fn check_rollback_policy( - next_image: ImageAccess<'_>, active_image: ImageAccess<'_>, + next_image: ImageAccess<'_>, complete: bool, ) -> Result<(), UpdateError> { let next_epoch = get_image_epoch(&next_image)?; diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs index 2e3d5a476..84e53a7df 100644 --- a/drv/lpc55-update-server/src/main.rs +++ b/drv/lpc55-update-server/src/main.rs @@ -202,14 +202,6 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> { self.header_block = None; return Err(e.into()); } - let active_image = - ImageAccess::new_flash(&self.flash, component, slot.other()); - if let Err(e) = - check_rollback_policy(active_image, next_image, false) - { - self.header_block = None; - return Err(e.into()); - } } else { // Block order is enforced above. If we're here then we have // seen block zero already.