util/cell: make read/write safe by using critical_section

Removes the need to propagate the ugly unsafe blocks upwards.

We add the Copy requirement in SyncCell read to avoid issues with drop
semantics (e.g. a missed drop that was expected in write(), or a
double-drop of the copy and original in read()).

Author: benma

src/CMakeLists.txt

@@ -400,7 +400,7 @@ foreach(type ${RUST_LIBS})
   # We build the rust standard libs so that everything from the rust runtime
   # is included (like eh_personality).
   if(type STREQUAL "c-unit-tests")
-      set(RUST_CARGO_FLAGS ${RUST_CARGO_FLAGS} -Zbuild-std)
+      set(RUST_CARGO_FLAGS ${RUST_CARGO_FLAGS} -Zbuild-std=std,panic_abort)
   endif()
   set(lib ${RUST_BINARY_DIR}/feature-${type}/${RUST_TARGET_ARCH_DIR}/${RUST_PROFILE}/libbitbox02_rust_c.a)
   # The dummy output is to always trigger rebuild (cargo is clever enough to

src/rust/Cargo.lock

@@ -88,7 +88,7 @@ firmware = [
 testing = ["bitbox02-rust/testing", "bitbox02/testing"]
 
 # Active when the Rust code is compiled to be linked into the C unit tests and simulator.
-c-unit-testing = ["bitbox02-rust/c-unit-testing", "bitbox02/c-unit-testing"]
+c-unit-testing = ["bitbox02-rust/c-unit-testing", "bitbox02/c-unit-testing", "util/c-unit-testing"]
 
 app-ethereum = [
   # enable this feature in the deps

src/rust/bitbox02-rust-c/Cargo.toml

@@ -38,9 +38,8 @@ extern crate util;
 // Whenever execution reaches somewhere it isn't supposed to rust code will "panic". Our panic
 // handler will print the available information on the screen and over RTT. If we compile with
 // `panic=abort` this code will never get executed.
-#[cfg(not(test))]
-#[cfg(not(feature = "testing"))]
 #[cfg_attr(feature = "bootloader", allow(unused_variables))]
+#[cfg(not(any(test, feature = "testing", feature = "c-unit-testing")))]
 #[panic_handler]
 fn panic(info: &core::panic::PanicInfo) -> ! {
     #[cfg(feature = "firmware")]

src/rust/bitbox02-rust-c/src/lib.rs

@@ -24,7 +24,7 @@ pub use bitbox02::keystore::SignResult;
 use bitbox02::{keystore, securechip};
 
 use util::bip32::HARDENED;
-use util::cell::SyncUnsafeCell;
+use util::cell::SyncCell;
 
 use crate::secp256k1::SECP256K1;
 
@@ -33,12 +33,12 @@ use bitcoin::hashes::{Hash, HashEngine, Hmac, HmacEngine, sha256, sha512};
 /// Length of a compressed secp256k1 pubkey.
 const EC_PUBLIC_KEY_LEN: usize = 33;
 
-static ROOT_FINGERPRINT: SyncUnsafeCell<Option<[u8; 4]>> = SyncUnsafeCell::new(None);
+static ROOT_FINGERPRINT: SyncCell<Option<[u8; 4]>> = SyncCell::new(None);
 
 /// Locks the keystore (resets to state before `unlock()`).
 pub fn lock() {
     keystore::_lock();
-    unsafe { ROOT_FINGERPRINT.write(None) }
+    ROOT_FINGERPRINT.write(None)
 }
 
 /// Returns false if the keystore is unlocked (unlock() followed by unlock_bip39()), true otherwise.
@@ -74,9 +74,7 @@ pub async fn unlock_bip39(
     keystore::unlock_bip39_finalize(bip39_seed.as_slice().try_into().unwrap())?;
 
     // Store root fingerprint.
-    unsafe {
-        ROOT_FINGERPRINT.write(Some(root_fingerprint));
-    }
+    ROOT_FINGERPRINT.write(Some(root_fingerprint));
     Ok(())
 }
 
@@ -208,7 +206,7 @@ pub fn root_fingerprint() -> Result<Vec<u8>, ()> {
     if is_locked() {
         return Err(());
     }
-    unsafe { ROOT_FINGERPRINT.read().ok_or(()).map(|fp| fp.to_vec()) }
+    ROOT_FINGERPRINT.read().ok_or(()).map(|fp| fp.to_vec())
 }
 
 /// Stretches the given encryption_key using the securechip. The resulting key is used to encrypt

src/rust/bitbox02-rust/src/keystore.rs

@@ -23,14 +23,15 @@ license = "Apache-2.0"
 [dependencies]
 num-bigint = { workspace = true, default-features = false }
 rtt-target = { version = "0.6.1", optional = true }
-cortex-m = { version = "0.7.7", features = ["critical-section-single-core"], optional = true }
+cortex-m = { version = "0.7.7", features = ["critical-section-single-core"] }
 hex = {workspace = true}
 sha2 = { workspace = true, optional = true }
 p256 = { version = "0.13.2", default-features = false, features = ["arithmetic", "ecdsa"], optional = true }
 bitcoin = {workspace = true}
+critical-section = { version = "1.2.0", default-features = false, features = [] }
 
 [features]
-rtt = ["dep:rtt-target", "dep:cortex-m"]
-testing = []
-c-unit-testing = []
+rtt = ["dep:rtt-target"]
+testing = ["critical-section/std"]
+c-unit-testing = ["critical-section/std"]
 firmware = []

src/rust/util/Cargo.toml

@@ -12,48 +12,43 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Same as `core::sync::SyncUnsafeCell`, which is still in nightly. Can remove once it is stable.
-pub struct SyncUnsafeCell<T: ?Sized> {
+// Link library to provide symbols necessary for critical section even if code isn't called
+// explicitly. See
+// https://github.com/rust-embedded/critical-section/blob/bc0d1736c3b14e340c1b0c29042edb3e6bffe6b8/README.md#undefined-reference-errors
+#[cfg(not(any(feature = "testing", feature = "c-unit-testing")))]
+#[allow(unused_imports)]
+use cortex_m as _;
+
+// A cell useful for global mutable state.
+pub struct SyncCell<T: ?Sized> {
     value: core::cell::UnsafeCell<T>,
 }
 
 // Implement Sync if the wrapped type is Sync.
-unsafe impl<T: ?Sized + Sync> Sync for SyncUnsafeCell<T> {}
+unsafe impl<T: ?Sized + Sync> Sync for SyncCell<T> {}
 
-impl<T> SyncUnsafeCell<T> {
+impl<T> SyncCell<T> {
     pub const fn new(val: T) -> Self {
-        SyncUnsafeCell {
+        SyncCell {
             value: core::cell::UnsafeCell::new(val),
         }
     }
 
     /// Reads the value from `self` without moving it. This leaves the
     /// memory in `self` unchanged.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because it allows accessing the interior value without
-    /// synchronization. The caller must ensure no other code is currently
-    /// writing to this cell during the read operation.
-    pub unsafe fn read(&self) -> T
+    pub fn read(&self) -> T
     where
-        T: Sized,
+        T: Sized + Copy,
     {
-        unsafe { self.value.get().read() }
+        critical_section::with(|_| unsafe { self.value.get().read() })
     }
 
     /// Overwrites a memory location with the given value without reading or
     /// dropping the old value.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because it allows accessing the interior value without
-    /// synchronization. The caller must ensure no other code is currently
-    /// accessing this cell (either reading or writing) during the write operation.
-    pub unsafe fn write(&self, val: T)
+    pub fn write(&self, val: T)
     where
-        T: Sized,
+        T: Sized + Copy,
     {
-        unsafe { self.value.get().write(val) }
+        critical_section::with(|_| unsafe { self.value.get().write(val) })
     }
 }