feat(core): report nRF SMP-push progress
What changed, and why it matters
This commit adds a progress bar feature for firmware updates sent to the nRF wireless chip inside Trezor devices. It threads a callback function through the upload code so the bootloader can show how much of the image has been transferred. The existing public syscall entry point is deliberately left unchanged, so normal app behavior is unaffected. There is no security fix or vulnerability here.
No security action required. Treat as a normal feature commit. If reviewing, verify that the callback is only invoked from non-syscall contexts as documented, and that the chunk-length fix correctly handles images smaller than CHUNK_SIZE.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The change introduces nrf_update_with_progress() alongside the existing nrf_update(). A per-chunk callback is passed through smp_upload_app_image() and into the Rust upload_image() routine, which now invokes the callback after each chunk. The callback is typed as an extern “C” function pointer in Rust and is optional (NULL allowed). The original nrf_update() delegates to the new function with NULL, preserving the existing coreapp syscall ABI. A minor correctness fix is included: the first and last chunks now use the actual chunk length rather than assuming a full CHUNK_SIZE, which avoids reading past the end of a short final chunk.
Changed components
core/embed/io/nrf/stm32u5/nrf_update.ccore/embed/io/nrf/rust_smp.hcore/embed/io/src/smp/api.rscore/embed/io/src/smp/upload.rscore/embed/io/nrf/inc/io/nrf.hInspect captured patch +65 / −8
### core/embed/io/nrf/inc/io/nrf.h
@@ -187,6 +187,35 @@ bool nrf_update_required(const uint8_t *image_ptr, size_t image_len);
*/
bool nrf_update(const uint8_t *image_ptr, size_t image_len);
+/**
+ * @brief Per-chunk progress callback for the nRF SMP upload.
+ *
+ * Reports an absolute position: `done` of `total` bytes transferred. See
+ * nrf_update_with_progress for why the sequence may step backwards.
+ */
+typedef void (*nrf_progress_callback_t)(uint32_t done, uint32_t total);
+
+/**
+ * @brief Like nrf_update, but reports SMP-upload progress via `progress`.
+ * For direct (non-syscall) callers such as the bootloader OTA workflow, which
+ * drive a progress bar during the push.
+ *
+ * `done` is NOT monotonic across the internal retries: a failed attempt is
+ * restarted from offset 0, so the next report drops back to one chunk. That is
+ * deliberate -- the upload really did start over, and a bar that visibly
+ * restarts is truthful where a stalled one would not be. Treat each
+ * (done, total) as an absolute position and render it as such; do NOT
+ * accumulate deltas or assume the sequence only rises.
+ *
+ * @param image_ptr Pointer to the firmware image in memory
+ * @param image_len Length of the firmware image in bytes
+ * @param progress May be NULL; NOT usable across the kernel/user syscall
+ * boundary -- see nrf_update.
+ * @return true if the update process was initiated
+ */
+bool nrf_update_with_progress(const uint8_t *image_ptr, size_t image_len,
+ nrf_progress_callback_t progress);
+
/**
* @brief Authenticate pairing of nRF chip with Trezor
*
### core/embed/io/nrf/rust_smp.h
@@ -21,6 +21,8 @@
#include <trezor_types.h>
+#include <io/nrf.h>
+
/**
* @brief Parsed nRF application version in format
* "major.minor.revision[.build_num]".
@@ -88,9 +90,11 @@ void smp_process_rx_byte(uint8_t byte);
* @param len Size of image buffer in bytes.
* @param image_hash Pointer to hash bytes (may be NULL if not used).
* @param image_hash_len Length of hash (e.g. 32 for SHA-256).
+ * @param progress Optional; NULL for no reporting.
* @return true if upload completed successfully, false on error.
*
* @note Caller must ensure image fits partition and hash matches expected size.
*/
bool smp_upload_app_image(const uint8_t* data, size_t len,
- const uint8_t* image_hash, size_t image_hash_len);
+ const uint8_t* image_hash, size_t image_hash_len,
+ nrf_progress_callback_t progress);
### core/embed/io/nrf/stm32u5/nrf_update.c
@@ -207,7 +207,8 @@ bool nrf_update_required(const uint8_t *image_ptr, size_t image_len) {
return true;
}
-bool nrf_update(const uint8_t *image_ptr, size_t image_len) {
+bool nrf_update_with_progress(const uint8_t *image_ptr, size_t image_len,
+ nrf_progress_callback_t progress) {
nrf_reboot_to_bootloader();
nrf_set_dfu_mode(true);
@@ -223,7 +224,7 @@ bool nrf_update(const uint8_t *image_ptr, size_t image_len) {
bool result = false;
do {
result = smp_upload_app_image(image_ptr, image_len, sha256,
- SHA256_DIGEST_LENGTH);
+ SHA256_DIGEST_LENGTH, progress);
try_cntr++;
} while (!result && try_cntr < 3);
@@ -237,5 +238,13 @@ bool nrf_update(const uint8_t *image_ptr, size_t image_len) {
return result;
}
+bool nrf_update(const uint8_t *image_ptr, size_t image_len) {
+ // No progress reporting on this (syscall) path -- the SMP upload's per-chunk
+ // callback would cross the kernel/user boundary. The bootloader OTA path
+ // calls nrf_update_with_progress directly (no syscall) to drive its progress
+ // bar.
+ return nrf_update_with_progress(image_ptr, image_len, NULL);
+}
+
#endif
#endif
### core/embed/io/src/smp/api.rs
@@ -59,12 +59,13 @@ unsafe extern "C" fn smp_upload_app_image(
len: cty::size_t,
image_hash: *const cty::uint8_t,
image_hash_len: cty::size_t,
+ progress: Option<extern "C" fn(cty::uint32_t, cty::uint32_t)>,
) -> bool {
// SAFETY: caller must provide valid pointers + lengths
let data = unsafe { CSlice::from_ptr_and_len(data, len) };
let image_hash = unsafe { CSlice::from_ptr_and_len(image_hash, image_hash_len) };
- upload::upload_image(data.as_slice(), image_hash.as_slice())
+ upload::upload_image(data.as_slice(), image_hash.as_slice(), progress)
}
#[unsafe(no_mangle)]
### core/embed/io/src/smp/upload.rs
@@ -10,7 +10,19 @@ use super::{
const CHUNK_SIZE: usize = 256;
const MAX_PACKET_SIZE: usize = 512;
-pub fn upload_image(image_data: &[u8], image_hash: &[u8]) -> bool {
+pub fn upload_image(
+ image_data: &[u8],
+ image_hash: &[u8],
+ progress: Option<extern "C" fn(u32, u32)>,
+) -> bool {
+ let total = image_data.len() as u32;
+ let first_chunk_len = image_data.len().min(CHUNK_SIZE);
+ let report = |done: usize| {
+ if let Some(cb) = progress {
+ cb((done as u32).min(total), total);
+ }
+ };
+
let mut cbor_data = [0u8; MAX_PACKET_SIZE];
let mut data = [0u8; MAX_PACKET_SIZE];
let mut buffer = [0u8; MAX_PACKET_SIZE];
@@ -29,7 +41,7 @@ pub fn upload_image(image_data: &[u8], image_hash: &[u8]) -> bool {
unwrap!(enc.str("hash"));
unwrap!(enc.bytes(image_hash));
unwrap!(enc.str("data"));
- unwrap!(enc.bytes(&image_data[..CHUNK_SIZE]));
+ unwrap!(enc.bytes(&image_data[..first_chunk_len]));
let data_len = writer.bytes_written();
unwrap!(receiver_acquire());
@@ -64,7 +76,8 @@ pub fn upload_image(image_data: &[u8], image_hash: &[u8]) -> bool {
return false;
}
- let mut offset = CHUNK_SIZE;
+ let mut offset = first_chunk_len;
+ report(offset);
for chunk in image_data.chunks(CHUNK_SIZE).skip(1) {
let mut cbor_data = [0u8; MAX_PACKET_SIZE];
@@ -106,7 +119,8 @@ pub fn upload_image(image_data: &[u8], image_hash: &[u8]) -> bool {
return false;
}
- offset += CHUNK_SIZE;
+ offset += chunk.len();
+ report(offset);
}
trueWhy this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.