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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion app/gimletlet/app-mgmt.toml
Original file line number Diff line number Diff line change
@@ -49,7 +49,7 @@ task-slots = ["sys", "user_leds"]

[tasks.net]
name = "task-net"
stacksize = 3000
stacksize = 8000
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

priority = 3
features = ["mgmt", "h753", "use-spi-core", "spi2"]
max-sizes = {flash = 131072, ram = 16384, sram1_mac = 16384}
2 changes: 0 additions & 2 deletions app/gimletlet/base-gimletlet2.toml
Original file line number Diff line number Diff line change
@@ -3,8 +3,6 @@ target = "thumbv7em-none-eabihf"
chip = "../../chips/stm32h7"
memory = "memory-large.toml"
stacksize = 896
epoch = 0
version = 0
fwid = true

[kernel]
2 changes: 0 additions & 2 deletions app/grapefruit/app.toml
Original file line number Diff line number Diff line change
@@ -4,8 +4,6 @@ target = "thumbv7em-none-eabihf"
chip = "../../chips/stm32h7"
memory = "memory-large.toml"
stacksize = 896
epoch = 0
version = 0
fwid = true

[kernel]
2 changes: 1 addition & 1 deletion app/lpc55xpresso/app.toml
Original file line number Diff line number Diff line change
@@ -48,7 +48,7 @@ start = true
[tasks.update_server]
name = "lpc55-update-server"
priority = 3
max-sizes = {flash = 26720, ram = 16704}
max-sizes = {flash = 30368, ram = 16704}
stacksize = 8192
start = true
sections = {bootstate = "usbsram"}
4 changes: 1 addition & 3 deletions app/oxide-rot-1/app-dev.toml
Original file line number Diff line number Diff line change
@@ -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]
@@ -53,7 +51,7 @@ start = true
[tasks.update_server]
name = "lpc55-update-server"
priority = 3
max-sizes = {flash = 26080, ram = 17000, usbsram = 4096}
max-sizes = {flash = 30368, ram = 17344, usbsram = 4096}
# TODO: Size this appropriately
stacksize = 8192
start = true
2 changes: 0 additions & 2 deletions app/oxide-rot-1/app.toml
Original file line number Diff line number Diff line change
@@ -5,8 +5,6 @@ board = "oxide-rot-1"
chip = "../../chips/lpc55"
stacksize = 1024
image-names = ["a", "b"]
epoch = 0
version = 0
fwid = true

[kernel]
2 changes: 0 additions & 2 deletions app/rot-carrier/app.toml
Original file line number Diff line number Diff line change
@@ -4,8 +4,6 @@ board = "rot-carrier-2"
chip = "../../chips/lpc55"
stacksize = 1024
image-names = ["a", "b"]
epoch = 0
version = 0
fwid = true

[kernel]
13 changes: 0 additions & 13 deletions build/xtask/src/config.rs
Original file line number Diff line number Diff line change
@@ -24,10 +24,6 @@ struct RawConfig {
board: String,
chip: String,
#[serde(default)]
epoch: u32,
#[serde(default)]
version: u32,
#[serde(default)]
fwid: bool,
memory: Option<String>,
#[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<String>,
pub signing: Option<RoTMfgSettings>,
@@ -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::<Vec<_>>().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(),
12 changes: 2 additions & 10 deletions build/xtask/src/dist.rs
Original file line number Diff line number Diff line change
@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

input: &Path,
output: &Path,
map: &IndexMap<String, Range<u32>>,
@@ -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(
10 changes: 10 additions & 0 deletions drv/lpc55-update-api/src/lib.rs
Original file line number Diff line number Diff line change
@@ -242,6 +242,16 @@ impl From<RotSlot> for SlotId {
}
}

impl SlotId {
pub fn other(&self) -> SlotId {
if *self == SlotId::A {
SlotId::B
} else {
SlotId::A
}
Comment on lines +247 to +251
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if *self == SlotId::A {
SlotId::B
} else {
SlotId::A
}
match self {
SlotId::A => SlotId::B,
SlotId::B => SlotId::A,
}

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

}
}

impl TryFrom<u16> for SlotId {
type Error = ();
fn try_from(i: u16) -> Result<Self, Self::Error> {
13 changes: 0 additions & 13 deletions drv/lpc55-update-server/build.rs
Original file line number Diff line number Diff line change
@@ -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<dyn std::error::Error + Send + Sync>> {
build_util::expose_target_board();
build_util::build_notifications()?;
@@ -18,15 +15,5 @@ fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
"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(())
}
681 changes: 620 additions & 61 deletions drv/lpc55-update-server/src/images.rs

Large diffs are not rendered by default.

351 changes: 141 additions & 210 deletions drv/lpc55-update-server/src/main.rs

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion drv/sprot-api/README.md
Original file line number Diff line number Diff line change
@@ -461,7 +461,7 @@ SpRot.status() => Status {
}
```
Update API, retrieve current version.
Update API, retrieve current version (deprecated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

This information is redundant with information in the Status structure.
```sh
$ humility hiffy -c SpRot.current_version
13 changes: 0 additions & 13 deletions drv/stm32h7-update-server/build.rs
Original file line number Diff line number Diff line change
@@ -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<dyn std::error::Error + Send + Sync>> {
build_util::build_notifications()?;
idol::Generator::new()
@@ -16,15 +13,5 @@ fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
"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(())
}
6 changes: 3 additions & 3 deletions drv/stm32h7-update-server/src/main.rs
Original file line number Diff line number Diff line change
@@ -416,13 +416,14 @@ impl idl::InOrderUpdateImpl for ServerImpl<'_> {
Ok(BLOCK_SIZE_BYTES)
}

/// Deprecated. The version and epoch values are in the Caboose.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

fn current_version(
&mut self,
_: &RecvMessage,
) -> Result<ImageVersion, RequestError<Infallible>> {
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};

2 changes: 2 additions & 0 deletions drv/update-api/src/lib.rs
Original file line number Diff line number Diff line change
@@ -65,6 +65,7 @@ pub enum UpdateError {
ImageMismatch,
SignatureNotValidated,
VersionNotSupported,
RollbackProtection,
}

impl From<UpdateError> for GwUpdateError {
@@ -103,6 +104,7 @@ impl From<UpdateError> for GwUpdateError {
UpdateError::ImageMismatch => Self::ImageMismatch,
UpdateError::SignatureNotValidated => Self::SignatureNotValidated,
UpdateError::VersionNotSupported => Self::VersionNotSupported,
UpdateError::RollbackProtection => Self::RollbackProtection,
}
}
}
14 changes: 12 additions & 2 deletions sys/abi/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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