refactor(core): improve safety of translation blobs

* the public interface to Translations is now completely safe
* it is more obvious that `map_translated` needs to work the way it does
* documentation is improved

core/embed/rust/Cargo.lock

@@ -284,9 +284,9 @@ checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64"
 
 [[package]]
 name = "spin"
-version = "0.9.2"
+version = "0.9.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "511254be0c5bcf062b019a6c89c01a664aa359ded62f78aa72c6fc137c0590e5"
+checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67"
 dependencies = [
  "lock_api",
 ]
@@ -326,6 +326,7 @@ dependencies = [
  "num-traits",
  "qrcodegen",
  "serde_json",
+ "spin",
  "trezor-tjpgdec",
  "zeroize",
 ]

core/embed/rust/Cargo.toml

@@ -77,6 +77,7 @@ debug = 2
 
 [dependencies]
 qrcodegen = { version = "1.8.0", path = "../../vendor/QR-Code-generator/rust-no-heap" }
+spin = { version = "0.9.8", features = ["rwlock"], default-features = false }
 trezor-tjpgdec = { version = "0.1.0", path = "../../../rust/trezor-tjpgdec" }
 zeroize = { version = "1.7.0", default-features = false, optional = true }
 

core/embed/rust/src/strutil.rs

@@ -102,10 +102,17 @@ impl TString<'_> {
         self.map(|s| s.is_empty())
     }
 
+    /// Maps the string to a value using a closure.
+    ///
+    /// # Safety
+    ///
+    /// The properties of this function are bounded by the properties of
+    /// `TranslatedString::map_translated`. The reference to the string is
+    /// guaranteed to be valid throughout the closure, but must not escape
+    /// it. The `for<'a>` bound on the closure's argument ensures this.
     pub fn map<F, T>(&self, fun: F) -> T
     where
         F: for<'a> FnOnce(&'a str) -> T,
-        T: 'static,
     {
         match self {
             #[cfg(feature = "micropython")]

core/embed/rust/src/translations/blob.rs

@@ -122,7 +122,7 @@ impl<'a> Table<'a> {
 }
 
 pub struct Translations<'a> {
-    pub header: TranslationsHeader<'a>,
+    header: TranslationsHeader<'a>,
     translations: &'a [u8],
     translations_offsets: &'a [u16],
     fonts: Table<'a>,
@@ -196,7 +196,15 @@ impl<'a> Translations<'a> {
     }
 
     /// Returns the translation at the given index.
-    pub fn translation(&self, index: usize) -> Option<&str> {
+    ///
+    /// SAFETY: Do not mess with the lifetimes in this signature.
+    ///
+    /// The lifetimes are a useful lie that bind the lifetime of the returned
+    /// string not to the underlying data, but to the _reference_ to the
+    /// translations object. This is to facilitate safe interface to
+    /// flash-based translations. See docs for `flash::get` for details.
+    #[allow(clippy::needless_lifetimes)]
+    pub fn translation<'b>(&'b self, index: usize) -> Option<&'b str> {
         if index + 1 >= self.translations_offsets.len() {
             // The index is out of bounds.
             // (The last entry is a sentinel, so the last valid index is len - 2)
@@ -223,11 +231,33 @@ impl<'a> Translations<'a> {
         str::from_utf8(string).ok()
     }
 
-    pub fn font(&'a self, index: u16) -> Option<Table<'a>> {
+    /// Returns the font table at the given index.
+    ///
+    /// SAFETY: Do not mess with the lifetimes in this signature.
+    ///
+    /// The lifetimes are a useful lie that bind the lifetime of the returned
+    /// string not to the underlying data, but to the _reference_ to the
+    /// translations object. This is to facilitate safe interface to
+    /// flash-based translations. See docs for `flash::get` for details.
+    #[allow(clippy::needless_lifetimes)]
+    pub fn font<'b>(&'b self, index: u16) -> Option<Table<'b>> {
         self.fonts
             .get(index)
             .and_then(|data| Table::new(InputStream::new(data)).ok())
     }
+
+    /// Returns the header of the translations blob.
+    ///
+    /// SAFETY: Do not mess with the lifetimes in this signature.
+    ///
+    /// The lifetimes are a useful lie that bind the lifetime of the returned
+    /// string not to the underlying data, but to the _reference_ to the
+    /// translations object. This is to facilitate safe interface to
+    /// flash-based translations. See docs for `flash::get` for details.
+    #[allow(clippy::needless_lifetimes)]
+    pub fn header<'b>(&'b self) -> &'b TranslationsHeader<'b> {
+        &self.header
+    }
 }
 
 pub struct TranslationsHeader<'a> {

core/embed/rust/src/translations/flash.rs

@@ -1,28 +1,44 @@
+use spin::{RwLock, RwLockReadGuard};
+
 use crate::{error::Error, trezorhal::translations};
 
 use super::blob::Translations;
 
-static mut TRANSLATIONS_ON_FLASH: Option<Translations> = None;
+static TRANSLATIONS_ON_FLASH: RwLock<Option<Translations>> = RwLock::new(None);
 
+/// Erase translations blob from flash.
+///
+/// The blob must be deinitialized via `deinit()` before calling this function.
 pub fn erase() -> Result<(), Error> {
-    // SAFETY: Looking is safe (in a single threaded environment).
+    // Write-lock is not necessary but it hints that nobody should call `erase()`
-    if unsafe { TRANSLATIONS_ON_FLASH.is_some() } {
+    // while others are looking.
+    let blob = unwrap!(TRANSLATIONS_ON_FLASH.try_write());
+    {
+        if blob.is_some() {
             return Err(value_error!("Translations blob already set"));
         }
 
         // SAFETY: The blob is not set, so there are no references to it.
         unsafe { translations::erase() };
+    }
     Ok(())
 }
 
+/// Write translations blob to flash.
+///
+/// The blob must be deinitialized via `deinit()` before calling this function.
 pub fn write(data: &[u8], offset: usize) -> Result<(), Error> {
-    // SAFETY: Looking is safe (in a single threaded environment).
+    // Write-lock is not necessary but it hints that nobody should call `erase()`
-    if unsafe { TRANSLATIONS_ON_FLASH.is_some() } {
+    // while others are looking.
+    let blob = unwrap!(TRANSLATIONS_ON_FLASH.try_write());
+    let result = {
+        if blob.is_some() {
             return Err(value_error!("Translations blob already set"));
         }
 
         // SAFETY: The blob is not set, so there are no references to it.
-    let result = unsafe { translations::write(data, offset) };
+        unsafe { translations::write(data, offset) }
+    };
     if result {
         Ok(())
     } else {
@@ -32,6 +48,9 @@ pub fn write(data: &[u8], offset: usize) -> Result<(), Error> {
 
 /// Load translations from flash, validate, and cache references to lookup
 /// tables.
+///
+/// # Safety
+/// Result depends on flash contents, see `translations::get_blob()`.
 unsafe fn try_init<'a>() -> Result<Option<Translations<'a>>, Error> {
     // load from flash
     let flash_data = unsafe { translations::get_blob() };
@@ -44,37 +63,58 @@ unsafe fn try_init<'a>() -> Result<Option<Translations<'a>>, Error> {
     Translations::new(flash_data).map(Some)
 }
 
+/// Initialize translations subsystem with current data from flash
+///
+/// Will erase any data from the translations section if the blob is invalid. At
+/// end, either the blob is available via `get()`, or there is a None value.
+///
+/// Does nothing if the data is already loaded. Call `deinit()` first to force
+/// reload.
 pub fn init() {
-    // unsafe block because every individual operation here is unsafe
+    let blob = unwrap!(TRANSLATIONS_ON_FLASH.try_upgradeable_read());
-    unsafe {
+    if blob.is_some() {
-        // SAFETY: it is OK to look
-        if TRANSLATIONS_ON_FLASH.is_some() {
         return;
     }
+
+    let mut blob = blob.upgrade();
     // SAFETY: try_init unconditionally loads the translations from flash.
     // No other reference exists (TRANSLATIONS_ON_FLASH is None) so this is safe.
-        match try_init() {
+    match unsafe { try_init() } {
-            // SAFETY: We are in a single-threaded environment so setting is OK.
+        Ok(Some(t)) => *blob = Some(t),
-            // (note that from this point on a reference to flash data is held)
-            Ok(Some(t)) => TRANSLATIONS_ON_FLASH = Some(t),
         Ok(None) => {}
         // SAFETY: No reference to flash data exists so it is OK to erase it.
-            Err(_) => translations::erase(),
+        Err(_) => unsafe { translations::erase() },
-        }
     }
 }
 
-// SAFETY: Invalidates all references coming from the flash-based blob.
+/// Deinitialize translations subsystem.
-// In other words, none should exist when this function is called.
+///
-pub unsafe fn deinit() {
+/// If the blob is locked by a reader, `deinit()` will return an error.
-    // SAFETY: Given the above, we can safely clear the cached object.
+pub fn deinit() -> Result<(), Error> {
-    unsafe { TRANSLATIONS_ON_FLASH = None };
+    let Some(mut blob) = TRANSLATIONS_ON_FLASH.try_write() else {
+        return Err(value_error!("Translations are in use."));
+    };
+    *blob = None;
+    Ok(())
 }
 
-// SAFETY: Gives out a reference to a TranslationsBlob which can be invalidated
+/// Get a reference to the translations blob.
-// by calling `erase()`. The caller must not store this reference, nor any that
+///
-// come from it, beyond the lifetime of the current function.
+/// # Safety
-pub unsafe fn get<'a>() -> Option<&'a Translations<'a>> {
+///
-    // SAFETY: We are in a single-threaded environment.
+/// This function relies on `Translations` to Do The Right Thing™ by only
-    unsafe { TRANSLATIONS_ON_FLASH.as_ref() }
+/// returning references whose lifetime is tied to the lifetime _of the
+/// reference_, as opposed to the underlying data.
+///
+/// Due to us placing the `Translations` blob in a `static` variable, the
+/// lifetime of its data must be `'static`. The true lifetime, however, is
+/// "between init() and deinit()".
+///
+/// So instead we tie all references to the lifetime of the returned
+/// `RwLockReadGuard`, through making sure that `Translations` only ever returns
+/// references that live as long as the reference giving them out.
+pub fn get() -> Result<RwLockReadGuard<'static, Option<Translations<'static>>>, Error> {
+    TRANSLATIONS_ON_FLASH
+        .try_read()
+        .ok_or_else(|| value_error!("Translations are in use."))
 }

core/embed/rust/src/translations/mod.rs

@@ -22,7 +22,10 @@ pub unsafe extern "C" fn get_utf8_glyph(codepoint: cty::uint16_t, font: cty::c_i
     // SAFETY: Reference is discarded at the end of the function.
     // We do return a _pointer_ to the same memory location, but the pointer is
     // always valid.
-    let Some(tr) = (unsafe { flash::get() }) else {
+    let Ok(translations) = flash::get() else {
+        return core::ptr::null();
+    };
+    let Some(tr) = translations.as_ref() else {
         return core::ptr::null();
     };
     if let Some(glyph) = tr.font(font_abs).and_then(|t| t.get(codepoint)) {

core/embed/rust/src/translations/obj.rs

@@ -21,9 +21,8 @@ impl TryFrom<TranslatedString> for StrBuffer {
     type Error = Error;
 
     fn try_from(value: TranslatedString) -> Result<Self, Self::Error> {
-        // SAFETY: The translated string is copied into a new memory. Reference to flash
+        let blob = super::flash::get()?;
-        // data is discarded at the end of this function.
+        let translated = value.translate(blob.as_ref());
-        let translated = value.translate(unsafe { super::flash::get() });
         StrBuffer::alloc(translated)
         // TODO fall back to English (which is static and can be converted
         // infallibly) if the allocation fails?
@@ -31,10 +30,9 @@ impl TryFrom<TranslatedString> for StrBuffer {
 }
 
 fn translate(translation: TranslatedString) -> Result<Obj, Error> {
-    // SAFETY: TryFrom<&str> for Obj allocates a copy of the passed in string.
+    translation
-    // The reference to flash data is discarded at the end of this function.
+        .translate(super::flash::get()?.as_ref())
-    let stored_translations = unsafe { super::flash::get() };
+        .try_into()
-    translation.translate(stored_translations).try_into()
 }
 
 // SAFETY: Caller is supposed to be MicroPython, or copy MicroPython contracts
@@ -99,12 +97,9 @@ pub unsafe extern "C" fn translations_header_new(
 }
 
 pub extern "C" fn translations_header_from_flash(_cls_in: Obj) -> Obj {
-    let block = || {
+    let block = || match super::flash::get()?.as_ref() {
-        // SAFETY: reference is discarded at the end of this function.
+        Some(translations) => make_translations_header(translations.header()),
-        match unsafe { super::flash::get() } {
-            Some(translations) => make_translations_header(&translations.header),
         None => Ok(Obj::const_none()),
-        }
     };
     unsafe { util::try_or_raise(block) }
 }
@@ -126,8 +121,8 @@ extern "C" fn area_bytesize() -> Obj {
 
 extern "C" fn get_language() -> Obj {
     let block = || {
-        // SAFETY: reference is discarded at the end of the block
+        let blob = super::flash::get()?;
-        let lang_name = unsafe { super::flash::get() }.map(|t| t.header.language);
+        let lang_name = blob.as_ref().map(|t| t.header().language);
         lang_name.unwrap_or(super::DEFAULT_LANGUAGE).try_into()
     };
     unsafe { util::try_or_raise(block) }
@@ -142,10 +137,11 @@ extern "C" fn init() -> Obj {
 }
 
 extern "C" fn deinit() -> Obj {
-    // SAFETY: Safe by itself. Any unsafety stems from some other piece of code
+    let block = || {
-    // not upholding the safety parameters.
+        super::flash::deinit()?;
-    unsafe { super::flash::deinit() };
+        Ok(Obj::const_none())
-    Obj::const_none()
+    };
+    unsafe { util::try_or_raise(block) }
 }
 
 extern "C" fn erase() -> Obj {

core/embed/rust/src/translations/translated_string.rs

@@ -10,16 +10,25 @@ impl TranslatedString {
             .unwrap_or(self.untranslated())
     }
 
+    /// Maps the translated string to a value using a closure.
+    ///
+    /// # Safety
+    ///
+    /// This is the only safe way to access a reference to the flash data. This
+    /// function guarantees that the reference is valid throughout the
+    /// closure, but as soon as the closure returns, the lock held on flash
+    /// data is released. This means that translations could get deinited
+    /// and rewritten, invalidating the reference.
+    ///
+    /// To guarantee that the reference does not escape the closure, we use a
+    /// HRTB of the closure's argument, which ensures that the lifetime of
+    /// the reference is too short to store it outside in any form.
     pub fn map_translated<F, T>(self, fun: F) -> T
     where
         F: for<'a> FnOnce(&'a str) -> T,
-        T: 'static,
     {
-        // SAFETY: The bound on F _somehow_ ensures that the reference cannot escape
+        let translations = unwrap!(super::flash::get());
-        // the closure. (I don't understand how, but it does), see soundness test below.
+        fun(self.translate(translations.as_ref()))
-        // For good measure, we limit the return value to 'static.
-        let translations = unsafe { super::flash::get() };
-        fun(self.translate(translations))
     }
 
     pub const fn as_tstring(self) -> TString<'static> {