Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always calculate the firmware measurement even if there is no valid i… #1521

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/oxide-rot-1/app-dev.toml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ task-slots = ["syscon_driver"]
[tasks.sprot]
name = "drv-lpc55-sprot-server"
priority = 6
max-sizes = {flash = 46300, ram = 32768}
max-sizes = {flash = 46272, ram = 32768}
uses = ["flexcomm8", "bootrom"]
features = ["spi0"]
start = true
Expand Down
2 changes: 1 addition & 1 deletion app/oxide-rot-1/app.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ task-slots = ["syscon_driver"]
[tasks.sprot]
name = "drv-lpc55-sprot-server"
priority = 6
max-sizes = {flash = 46300, ram = 32768}
max-sizes = {flash = 46272, ram = 32768}
uses = ["flexcomm8", "bootrom"]
features = ["spi0"]
start = true
Expand Down
4 changes: 2 additions & 2 deletions app/rot-carrier/app.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ fwid = true
[kernel]
name = "rot-carrier"
features = ["dice-self"]
requires = {flash = 53000, ram = 4096}
requires = {flash = 53300, ram = 4096}

[caboose]
tasks = ["caboose_reader", "sprot"]
Expand Down Expand Up @@ -108,7 +108,7 @@ task-slots = ["syscon_driver"]
[tasks.sprot]
name = "drv-lpc55-sprot-server"
priority = 6
max-sizes = {flash = 46300, ram = 32768}
max-sizes = {flash = 46272, ram = 32768}
uses = ["flexcomm8", "bootrom"]
features = ["spi0"]
start = true
Expand Down
139 changes: 83 additions & 56 deletions lib/lpc55-rot-startup/src/images.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,31 @@
// 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 abi::{ImageHeader, ImageVectors};
use abi::ImageHeader;
use drv_lpc55_flash::{Flash, BYTES_PER_FLASH_PAGE};
use sha3::{Digest, Sha3_256};
use stage0_handoff::{ImageVersion, RotImageDetails};
use unwrap_lite::UnwrapLite;

pub fn get_image_b(flash: &mut Flash<'_>) -> Option<Image> {
let imageb = unsafe { &__IMAGE_B_BASE };

let img = Image {
flash: FLASH_B,
vector: imageb,
};

if img.validate(flash) {
pub fn get_image_b(flash: &mut Flash) -> Option<Image> {
let img = Image::new(flash, FLASH_B);
if img.validate() {
Some(img)
} else {
None
}
}

pub fn get_image_a(flash: &mut Flash<'_>) -> Option<Image> {
let imagea = unsafe { &__IMAGE_A_BASE };

let img = Image {
flash: FLASH_A,
vector: imagea,
};

if img.validate(flash) {
pub fn get_image_a(flash: &mut Flash) -> Option<Image> {
let img = Image::new(flash, FLASH_A);
if img.validate() {
Some(img)
} else {
None
}
}

extern "C" {
static __IMAGE_A_BASE: abi::ImageVectors;
static __IMAGE_B_BASE: abi::ImageVectors;
// __vector size is currently defined in the linker script as
//
// __vector_size = SIZEOF(.vector_table);
Expand All @@ -56,20 +42,84 @@ extern "C" {
const PAGE_SIZE: u32 = BYTES_PER_FLASH_PAGE as u32;

pub struct Image {
// The boundaries of the flash slot.
flash: Range<u32>,
vector: &'static ImageVectors,
// The contiguous span of programmed flash pages starting at offset zero.
// Note that any additional programmed pages after the first erased
// page are not interesting for image sanity checks and are not included.
programmed: Range<u32>,
// Measurement over all pages including those that follow any erased page.
fwid: [u8; 32],
}

pub fn image_details(img: Image, flash: &mut Flash<'_>) -> RotImageDetails {
impl Image {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've got a second impl block here

// Before doing any other work with a chunk of flash memory:
//
// - Define the address boundaries.
// - Determine the bounds of the initial programmed extent.
// - Count the total number of fully programmed pages.
// - Measure all programmed pages including those outside of the
// initial programmed extent.
//
// Note: if partially programmed pages is a possibility then that could be
// a problem with respect to catching exfiltration attempts.
pub fn new(dev: &mut Flash, flash: Range<u32>) -> Image {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the pub

let mut end: Option<u32> = None;
let mut hash = Sha3_256::new();
for start in flash.clone().step_by(BYTES_PER_FLASH_PAGE) {
if dev.is_page_range_programmed(start, PAGE_SIZE) {
let page = unsafe {
core::slice::from_raw_parts(
start as *const u8,
BYTES_PER_FLASH_PAGE,
)
};
hash.update(page);
} else if end.is_none() {
end = Some(start);
}
}
let fwid = hash.finalize().try_into().unwrap_lite();
let programmed = Range {
start: flash.start,
end: end.unwrap_or(flash.end),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is intentional but given a range of 3 blocks with the middle block not programmed the programmed range will be reported as Range { start: 0, end: 1 }. Is the intention to ignore all pages after an unprogrammed one is encountered? It will include the 2 programmed page in the hash so the important behavior has been preserved. Representing the programmed pages as a contiguous range when it may not be could cause confusion though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. Maybe "programmed" should be renamed for clarity though.
The intent is to know the contiguous programmed region starting from offset zero.
This range will be used to test that pointers and offsets in the NXP executable image have no erased gap pages.
The first test is that the NXP length field is in that region. Later tests are bounded by that
NXP image length value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment on Image{programmed: } since I didn't come up with a more descriptive name that wasn't awkward. Does that work?

Image {
flash,
programmed,
fwid,
}
}

pub fn is_programmed(&self, addr: &u32) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be pub:

return self.programmed.contains(addr);
}

/// True if the flash slot's span of contiguous programmed pages
/// starting at offset zero includes the given span.
pub fn is_span_programmed(&self, start: u32, length: u32) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be pub:

if let Some(end) = start.checked_add(length) {
if !self.is_programmed(&start) || !self.is_programmed(&end) {
false
} else {
true
}
} else {
false
}
}
}

pub fn image_details(img: Image) -> RotImageDetails {
RotImageDetails {
digest: img.get_fwid(flash),
digest: img.fwid,
version: img.get_image_version(),
}
}

impl Image {
fn get_img_start(&self) -> u32 {
self.vector as *const ImageVectors as u32
self.flash.start
}

fn get_img_size(&self) -> Option<usize> {
Expand All @@ -86,23 +136,18 @@ impl Image {
}

/// Make sure all of the image flash is programmed
fn validate(&self, flash: &mut Flash<'_>) -> bool {
fn validate(&self) -> bool {
let img_start = self.get_img_start();

// Start by making sure we can access the page where the vectors live
let valid = flash.is_page_range_programmed(img_start, PAGE_SIZE);

if !valid {
if !self.is_span_programmed(self.flash.start, PAGE_SIZE) {
return false;
}

let header_ptr = self.get_header();

// Next validate the header location is programmed
let valid =
flash.is_page_range_programmed(header_ptr as u32, PAGE_SIZE);

if !valid {
if !self.is_span_programmed(header_ptr as u32, PAGE_SIZE) {
return false;
}

Expand All @@ -111,7 +156,7 @@ impl Image {
// which we trust.
let header = unsafe { &*header_ptr };

// Does this look correct?
// Does this look like a header?
if header.magic != abi::HEADER_MAGIC {
return false;
}
Expand All @@ -122,30 +167,12 @@ impl Image {
None => return false,
};

// Last step is to make sure the entire range is programmed
flash.is_page_range_programmed(img_start, total_len)
}

pub fn get_fwid(&self, flash: &mut Flash<'_>) -> [u8; 32] {
let mut hash = Sha3_256::new();

for start in self.flash.clone().step_by(BYTES_PER_FLASH_PAGE) {
if flash.is_page_range_programmed(start, PAGE_SIZE) {
// SAFETY: The addresses used in this unsafe code are all
// generated by build.rs from data in the build environment.
// The safety of this code is an extension of our trust in
// the build.
let page = unsafe {
core::slice::from_raw_parts(
start as *const u8,
BYTES_PER_FLASH_PAGE,
)
};
hash.update(page);
}
// Next make sure the marked image length is programmed
if !self.is_span_programmed(img_start, total_len) {
return false;
}

hash.finalize().try_into().unwrap_lite()
return true;
}

pub fn get_image_version(&self) -> ImageVersion {
Expand Down
4 changes: 2 additions & 2 deletions lib/lpc55-rot-startup/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ pub fn startup(
} else {
panic!();
};
let a = img_a.map(|i| images::image_details(i, &mut flash));
let b = img_b.map(|i| images::image_details(i, &mut flash));
let a = img_a.map(|i| images::image_details(i));
let b = img_b.map(|i| images::image_details(i));

let details = RotBootState { active, a, b };

Expand Down
Loading