From 0565eba3a78e23bb2c1cb5cfd20bfdae59c4aa3b Mon Sep 17 00:00:00 2001 From: matejcik Date: Tue, 2 Apr 2024 15:49:33 +0200 Subject: [PATCH] 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 | 5 +- core/embed/rust/Cargo.toml | 1 + core/embed/rust/src/strutil.rs | 9 +- core/embed/rust/src/translations/blob.rs | 36 +++++- core/embed/rust/src/translations/flash.rs | 115 +++++++++++------- core/embed/rust/src/translations/mod.rs | 5 +- core/embed/rust/src/translations/obj.rs | 34 +++--- .../src/translations/translated_string.rs | 19 ++- 8 files changed, 153 insertions(+), 71 deletions(-) diff --git a/core/embed/rust/Cargo.lock b/core/embed/rust/Cargo.lock index 76905fa42..c953f2f21 100644 --- a/core/embed/rust/Cargo.lock +++ b/core/embed/rust/Cargo.lock @@ -284,9 +284,9 @@ checksum = "43b2853a4d09f215c24cc5489c992ce46052d359b5109343cbafbf26bc62f8a3" [[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", ] diff --git a/core/embed/rust/Cargo.toml b/core/embed/rust/Cargo.toml index 17521bf78..cff98702d 100644 --- a/core/embed/rust/Cargo.toml +++ b/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 } diff --git a/core/embed/rust/src/strutil.rs b/core/embed/rust/src/strutil.rs index 0054584ef..47e0ba335 100644 --- a/core/embed/rust/src/strutil.rs +++ b/core/embed/rust/src/strutil.rs @@ -87,10 +87,17 @@ impl TString<'_> { self.len() == 0 } + /// 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(&self, fun: F) -> T where F: for<'a> FnOnce(&'a str) -> T, - T: 'static, { match self { #[cfg(feature = "micropython")] diff --git a/core/embed/rust/src/translations/blob.rs b/core/embed/rust/src/translations/blob.rs index 2bf6f52a4..3ba42610b 100644 --- a/core/embed/rust/src/translations/blob.rs +++ b/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> { + /// 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> { 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> { diff --git a/core/embed/rust/src/translations/flash.rs b/core/embed/rust/src/translations/flash.rs index 831f981f9..76cdb4ae0 100644 --- a/core/embed/rust/src/translations/flash.rs +++ b/core/embed/rust/src/translations/flash.rs @@ -1,28 +1,40 @@ +use spin::{RwLock, RwLockReadGuard}; + use crate::{error::Error, trezorhal::translations}; use super::blob::Translations; -static mut TRANSLATIONS_ON_FLASH: Option = None; +static TRANSLATIONS_ON_FLASH: RwLock> = 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). - if unsafe { TRANSLATIONS_ON_FLASH.is_some() } { - return Err(value_error!("Translations blob already set")); - } + let blob = TRANSLATIONS_ON_FLASH.read(); + { + 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() }; + // 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). - if unsafe { TRANSLATIONS_ON_FLASH.is_some() } { - return Err(value_error!("Translations blob already set")); - } + let blob = TRANSLATIONS_ON_FLASH.read(); + 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) }; + // SAFETY: The blob is not set, so there are no references to it. + unsafe { translations::write(data, offset) } + }; if result { Ok(()) } else { @@ -32,6 +44,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>, Error> { // load from flash let flash_data = unsafe { translations::get_blob() }; @@ -44,39 +59,57 @@ unsafe fn try_init<'a>() -> Result>, 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 - unsafe { - // SAFETY: it is OK to look - if TRANSLATIONS_ON_FLASH.is_some() { - return; - } - // 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() { - // SAFETY: We are in a single-threaded environment so setting is OK. - // (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(), - } + let blob = TRANSLATIONS_ON_FLASH.upgradeable_read(); + if blob.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 unsafe { try_init() } { + Ok(Some(t)) => *blob = Some(t), + Ok(None) => {} + // SAFETY: No reference to flash data exists so it is OK to erase it. + Err(_) => unsafe { translations::erase() }, } } -/// # Safety -/// Invalidates all references coming from the flash-based blob. -/// In other words, none should exist when this function is called. -pub unsafe fn deinit() { - // SAFETY: Given the above, we can safely clear the cached object. - unsafe { TRANSLATIONS_ON_FLASH = None }; +/// Deinitialize translations subsystem. +/// +/// If the blob is locked by a reader, `deinit()` will return an error. +pub fn deinit() -> Result<(), Error> { + let Some(mut blob) = TRANSLATIONS_ON_FLASH.try_write() else { + return Err(value_error!("Translations are in use.")); + }; + *blob = None; + Ok(()) } +/// Get a reference to the translations blob. +/// /// # Safety -/// Gives out a reference to a TranslationsBlob which can be invalidated -/// by calling `erase()`. The caller must not store this reference, nor any that -/// come from it, beyond the lifetime of the current function. -pub unsafe fn get<'a>() -> Option<&'a Translations<'a>> { - // SAFETY: We are in a single-threaded environment. - unsafe { TRANSLATIONS_ON_FLASH.as_ref() } +/// +/// This function relies on `Translations` to Do The Right Thing™ by only +/// 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>>, Error> { + TRANSLATIONS_ON_FLASH + .try_read() + .ok_or_else(|| value_error!("Translations are in use.")) } diff --git a/core/embed/rust/src/translations/mod.rs b/core/embed/rust/src/translations/mod.rs index 639112c92..0931c0de2 100644 --- a/core/embed/rust/src/translations/mod.rs +++ b/core/embed/rust/src/translations/mod.rs @@ -23,7 +23,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)) { diff --git a/core/embed/rust/src/translations/obj.rs b/core/embed/rust/src/translations/obj.rs index 80e26a24c..7ba6fb4db 100644 --- a/core/embed/rust/src/translations/obj.rs +++ b/core/embed/rust/src/translations/obj.rs @@ -21,9 +21,8 @@ impl TryFrom for StrBuffer { type Error = Error; fn try_from(value: TranslatedString) -> Result { - // SAFETY: The translated string is copied into a new memory. Reference to flash - // data is discarded at the end of this function. - let translated = value.translate(unsafe { super::flash::get() }); + let blob = super::flash::get()?; + let translated = value.translate(blob.as_ref()); 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 for StrBuffer { } fn translate(translation: TranslatedString) -> Result { - // SAFETY: TryFrom<&str> for Obj allocates a copy of the passed in string. - // The reference to flash data is discarded at the end of this function. - let stored_translations = unsafe { super::flash::get() }; - translation.translate(stored_translations).try_into() + translation + .translate(super::flash::get()?.as_ref()) + .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 = || { - // SAFETY: reference is discarded at the end of this function. - match unsafe { super::flash::get() } { - Some(translations) => make_translations_header(&translations.header), - None => Ok(Obj::const_none()), - } + let block = || match super::flash::get()?.as_ref() { + 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 lang_name = unsafe { super::flash::get() }.map(|t| t.header.language); + let blob = super::flash::get()?; + 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 - // not upholding the safety parameters. - unsafe { super::flash::deinit() }; - Obj::const_none() + let block = || { + super::flash::deinit()?; + Ok(Obj::const_none()) + }; + unsafe { util::try_or_raise(block) } } extern "C" fn erase() -> Obj { diff --git a/core/embed/rust/src/translations/translated_string.rs b/core/embed/rust/src/translations/translated_string.rs index b41ef228a..ba06742f6 100644 --- a/core/embed/rust/src/translations/translated_string.rs +++ b/core/embed/rust/src/translations/translated_string.rs @@ -10,16 +10,27 @@ 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(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 // the closure. (I don't understand how, but it does), see soundness test below. - // For good measure, we limit the return value to 'static. - let translations = unsafe { super::flash::get() }; - fun(self.translate(translations)) + let translations = unwrap!(super::flash::get()); + fun(self.translate(translations.as_ref())) } pub const fn as_tstring(self) -> TString<'static> {