oxidecomputer / oxidecomputer/hubris

Consider using `extern static` instead of MaybeUninit for accessing bootloader shared data

Open
#2,110 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
3.6k
Forks
239
Avg merge
1d 12h
Merged PRs (30d)
23

Description

The use of MaybeUninit here seems sketchy; it's only defined in terms of Rust behavior, so using assume_init to grab data that happens to be in RAM could be fragile.

(This is admittedly not a new problem introduced in the PR, but the PR adds another case of it)

I think the right way to handle this is using an extern static, although it's still a subject of active discussion (https://github.com/rust-lang/unsafe-code-guidelines/issues/397#issuecomment-2619633222)

Here's an (untested) patch which does just that:

diff --git a/app/lpc55xpresso/app.toml b/app/lpc55xpresso/app.toml
index cc03b0de53..48b479bacb 100644
--- a/app/lpc55xpresso/app.toml
+++ b/app/lpc55xpresso/app.toml
@@ -51,7 +51,8 @@
 max-sizes = {flash = 26720, ram = 16704}
 stacksize = 8192
 start = true
-sections = {bootstate = "usbsram", transient_override = "override"}
+sections = {bootstate = "usbsram"}
+extern-regions = ["transient_override"]
 uses = ["flash_controller", "hash_crypt"]
 notifications = ["flash-irq", "hashcrypt-irq"]
 interrupts = {"flash_controller.irq" = "flash-irq", "hash_crypt.irq" = "hashcrypt-irq"}
diff --git a/app/oxide-rot-1/app-dev.toml b/app/oxide-rot-1/app-dev.toml
index 50a945c016..9d45ad973f 100644
--- a/app/oxide-rot-1/app-dev.toml
+++ b/app/oxide-rot-1/app-dev.toml
@@ -55,7 +55,8 @@
 priority = 3
 stacksize = 8192
 start = true
-sections = {bootstate = "usbsram", transient_override = "override"}
+sections = {bootstate = "usbsram"}
+extern-regions = ["transient_override"]
 uses = ["flash_controller", "hash_crypt"]
 notifications = ["flash-irq", "hashcrypt-irq"]
 interrupts = {"flash_controller.irq" = "flash-irq", "hash_crypt.irq" = "hashcrypt-irq"}
diff --git a/app/oxide-rot-1/app.toml b/app/oxide-rot-1/app.toml
index 9e12953bad..0c45eb254d 100644
--- a/app/oxide-rot-1/app.toml
+++ b/app/oxide-rot-1/app.toml
@@ -43,7 +43,8 @@
 priority = 3
 stacksize = 8192
 start = true
-sections = {bootstate = "usbsram", transient_override = "override"}
+sections = {bootstate = "usbsram"}
+extern-regions = ["transient_override"]
 uses = ["flash_controller", "hash_crypt"]
 notifications = ["flash-irq", "hashcrypt-irq"]
 interrupts = {"flash_controller.irq" = "flash-irq", "hash_crypt.irq" = "hashcrypt-irq"}
diff --git a/app/rot-carrier/app.toml b/app/rot-carrier/app.toml
index 3072a96e1c..74c20d7d64 100644
--- a/app/rot-carrier/app.toml
+++ b/app/rot-carrier/app.toml
@@ -43,7 +43,8 @@
 # TODO size this appropriately
 stacksize = 8192
 start = true
-sections = {bootstate = "usbsram", transient_override = "override"}
+sections = {bootstate = "usbsram"}
+extern-regions = ["transient_override"]
 uses = ["flash_controller", "hash_crypt"]
 notifications = ["flash-irq", "hashcrypt-irq"]
 interrupts = {"flash_controller.irq" = "flash-irq", "hash_crypt.irq" = "hashcrypt-irq"}
diff --git a/chips/lpc55/memory.toml b/chips/lpc55/memory.toml
index 0125777412..c6322ef47d 100644
--- a/chips/lpc55/memory.toml
+++ b/chips/lpc55/memory.toml
@@ -93,7 +93,7 @@
 execute = false
 dma = true
 
-[[override]]
+[[transient_override]]
 name = "a"
 address = 0x2003ffe0
 size = 32
@@ -101,7 +101,7 @@
 write = true
 execute = false
 
-[[override]]
+[[transient_override]]
 name = "b"
 address = 0x2003ffe0
 size = 32
diff --git a/drv/lpc55-update-server/src/main.rs b/drv/lpc55-update-server/src/main.rs
index 8f78b91c42..3cc2898d46 100644
--- a/drv/lpc55-update-server/src/main.rs
+++ b/drv/lpc55-update-server/src/main.rs
@@ -41,10 +41,6 @@
 #[link_section = ".bootstate"]
 static BOOTSTATE: MaybeUninit<[u8; 0x1000]> = MaybeUninit::uninit();
 
-#[used]
-#[link_section = ".transient_override"]
-static mut TRANSIENT_OVERRIDE: MaybeUninit<[u8; 32]> = MaybeUninit::uninit();
-
 #[derive(Copy, Clone, PartialEq)]
 enum UpdateState {
     NoUpdate,
@@ -1416,22 +1412,39 @@
     RotBootStateV2::load_from_addr(addr)
 }
 
+extern "C" {
+    // Symbols injected by the linker.
+    //
+    // This requires adding `extern-regions = ["transient_override"]` to the
+    // task config
+    pub static mut __REGION_TRANSIENT_OVERRIDE_BASE: [u32; 0];
+}
+
 fn set_transient_override(preference: [u8; 32]) {
-    // Safety: Data is consumed by Bootleby on next boot.
-    // There are no concurrent writers possible.
-    // Calling this function multiple times is ok.
-    // Bootleby is careful to vet contents before acting.
-    unsafe {
-        TRANSIENT_OVERRIDE.write(preference);
+    // SAFETY: populated by the linker, getting the address is fine
+    let override_addr =
+        unsafe { __REGION_TRANSIENT_OVERRIDE_BASE.as_ptr() } as *mut u8;
+    for (i, p) in preference.iter().enumerate() {
+        // SAFETY: this points to a valid region of RAM that is otherwise unused
+        // by Rust, so we can write to it.
+        unsafe {
+            core::ptr::write_volatile(override_addr.wrapping_add(i), *p);
+        }
     }
 }
 
 fn get_transient_override() -> [u8; 32] {
-    // Safety: Data is consumed by Bootleby on next boot.
-    // There are no concurrent writers possible.
-    // Bootleby consumes and resets TRANSIENT_OVERRIDE.
-    // The client may be verifying state set during update flows.
-    unsafe { TRANSIENT_OVERRIDE.assume_init() }
+    // SAFETY: populated by the linker, getting the address is fine
+    let override_addr =
+        unsafe { __REGION_TRANSIENT_OVERRIDE_BASE.as_ptr() } as *const u8;
+    let mut out = [0u8; 32];
+    for (i, p) in out.iter_mut().enumerate() {
+        // SAFETY: values are guaranteed to be initialized by stage0
+        unsafe {
+            *p = core::ptr::read_volatile(override_addr.wrapping_add(i));
+        }
+    }
+    out
 }
 
 // Preference constants are taken from bootleby:src/lib.rs

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with drv/lpc55-update-server/src/main.rs, especially set_transient_override and get_transient_override, then inspect the extern-regions entries in the listed app TOML files and the transient_override regions in chips/lpc55/memory.toml. Check whether the linker-provided region safely replaces the MaybeUninit statics and whether the affected LPC55 configurations still build successfully.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
embedded-iot, operating-systems
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.