Skip to content

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

@lzrd

Description

@lzrd

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 (rust-lang/unsafe-code-guidelines#397 (comment))

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions