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

Conversation

lzrd
Copy link
Contributor

@lzrd lzrd commented Sep 6, 2023

…mage.

Note: The measurement is not plumbed through to the management plane at this time. It will wait for other API changes.

Also, figure out the programmed pages once while performing the slot measurement and use those results when the image is being verified.

Tweak .gitignore for tags and cscope users.

@lzrd
Copy link
Contributor Author

lzrd commented Sep 6, 2023

This PR does not plumb the fwid up to the control plane when the image is not valid.
There are some more API changes coming for bootloader update and I'd prefer to wait until then.

@lzrd
Copy link
Contributor Author

lzrd commented Sep 6, 2023

With the minor kernel size increase, the other app.toml files will need to be updated as well.

@lzrd
Copy link
Contributor Author

lzrd commented Sep 6, 2023

#1520

Copy link
Collaborator

@labbott labbott left a comment

Choose a reason for hiding this comment

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

So I agree with calculating the FWID but I'm not a fan of caching the programmed page range. That just seems like a place to introduce errors. Do we really need to cache it?

lib/lpc55-rot-startup/src/images.rs Outdated Show resolved Hide resolved
let programmed = Range {
start: flash.start,
end: end.unwrap_or(flash.start),
};
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?

//
// 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

}
}

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:


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

}

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

lzrd added 4 commits October 19, 2023 13:25
…mage.

Note: The measurement is not plumbed through to the management plane at
this time. It will wait for other API changes.

Also, figure out the programmed pages once while performing the slot measurement
and use those results when the image is being verified.

Tweak .gitignore for tags and cscope users.
Remove personal .gititnore patterns and move them to the file specified by ~/.gitconfig: excludesfile = ...
Various app.toml updates to accommodate flash/ram requirements
Set fwid = true in app.toml files.
@lzrd
Copy link
Contributor Author

lzrd commented May 1, 2024

Closed by #1675

@lzrd lzrd closed this May 1, 2024
@lzrd lzrd deleted the fwid-always branch May 1, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants