fix(legacy): Improve handling of busy deadline overflow.
What the developer wrote
Authored by Andrew Kozlik
fix(legacy): Improve handling of busy deadline overflow.
[no changelog]
What changed, and why it matters
This commit fixes a subtle arithmetic bug in older Trezor hardware wallets related to how a 'busy' screen timeout is calculated. The old code added a delay value to the current time and compared it directly, which could misbehave when the internal millisecond counter wraps around to zero after a long period. The new code stores the start time and duration separately and checks whether the elapsed time is still within that duration, which handles wraparound correctly. This could affect whether the device reports itself as busy, shows the right screen, or clears the busy state at the expected time.
Treat as a low-to-moderate reliability/security fix. Review whether the SetBusy message can be triggered by an untrusted host and whether incorrect busy-state handling could be abused to suppress prompts or alter device behavior. No immediate emergency response is indicated, but the fix should be included in the next firmware release for legacy devices.
Security signals we found
Integer overflow/wraparound in timeout deadline calculation
Behavioral change in device busy-state reporting and screen selection
Refactoring of global mutable state into accessor functions
Potential for persistent or prematurely cleared busy screen state
Evidence from the diff
The patch replaces a deadline-based busy timeout with an elapsed-time-based one. Previously, system_millis_busy_deadline was computed as timer_ms() + expiry_ms and later compared with timer_ms(). Because timer_ms() returns a uint32_t millisecond counter, the addition could overflow and the comparison could produce incorrect results near the wraparound boundary. The fix introduces trezor_set_busy() and trezor_is_busy(), storing system_millis_busy_start and system_millis_busy_length, and uses unsigned subtraction (timer_ms() - start < length) which is wraparound-safe. Call sites in fsm_msg_common.h, layout2.c, and trezor.c are updated to use the new helpers.
Changed components
legacy/firmware/trezor.clegacy/firmware/trezor.hlegacy/firmware/fsm_msg_common.hlegacy/firmware/layout2.cInspect captured patch +21 / −10
diff --git a/legacy/firmware/fsm_msg_common.h b/legacy/firmware/fsm_msg_common.h
index 849d579b..eab9d7c1 100644
--- a/legacy/firmware/fsm_msg_common.h
+++ b/legacy/firmware/fsm_msg_common.h
@@ -75,7 +75,7 @@ bool get_features(Features *resp) {
resp->has_safety_checks = true;
resp->safety_checks = config_getSafetyCheckLevel();
resp->has_busy = true;
- resp->busy = (system_millis_busy_deadline > timer_ms());
+ resp->busy = trezor_is_busy();
if (session_isUnlocked()) {
resp->has_wipe_code_protection = true;
resp->wipe_code_protection = config_hasWipeCode();
@@ -592,9 +592,9 @@ void fsm_msgGetFirmwareHash(const GetFirmwareHash *msg) {
void fsm_msgSetBusy(const SetBusy *msg) {
if (msg->has_expiry_ms) {
- system_millis_busy_deadline = timer_ms() + msg->expiry_ms;
+ trezor_set_busy(msg->expiry_ms);
} else {
- system_millis_busy_deadline = 0;
+ trezor_set_busy(0);
}
fsm_sendSuccess(NULL);
layoutHome();
diff --git a/legacy/firmware/layout2.c b/legacy/firmware/layout2.c
index 6aba1a80..3091c04b 100644
--- a/legacy/firmware/layout2.c
+++ b/legacy/firmware/layout2.c
@@ -300,7 +300,7 @@ void layoutProgressSwipe(const char *desc, int permil) {
}
void layoutScreensaver(void) {
- if (system_millis_busy_deadline > timer_ms()) {
+ if (trezor_is_busy()) {
// Busy screen overrides the screensaver.
layoutBusyscreen();
} else {
@@ -316,7 +316,7 @@ void layoutHome(void) {
system_millis_lock_start = timer_ms();
}
- if (system_millis_busy_deadline > timer_ms()) {
+ if (trezor_is_busy()) {
layoutBusyscreen();
} else {
layoutHomescreen();
diff --git a/legacy/firmware/trezor.c b/legacy/firmware/trezor.c
index 5edfd955..a29c7e99 100644
--- a/legacy/firmware/trezor.c
+++ b/legacy/firmware/trezor.c
@@ -64,7 +64,8 @@ void secp256k1_default_error_callback_fn(const char *str, void *data) {
uint32_t system_millis_lock_start = 0;
/* Busyscreen timeout */
-uint32_t system_millis_busy_deadline = 0;
+static uint32_t system_millis_busy_start = 0;
+static uint32_t system_millis_busy_length = 0;
void check_lock_screen(void) {
buttonUpdate();
@@ -117,11 +118,19 @@ void check_lock_screen(void) {
}
}
+void trezor_set_busy(uint32_t length_ms) {
+ system_millis_busy_start = timer_ms();
+ system_millis_busy_length = length_ms;
+}
+
+bool trezor_is_busy(void) {
+ return timer_ms() - system_millis_busy_start < system_millis_busy_length;
+}
+
void check_busy_screen(void) {
// Clear the busy screen once it expires.
- if (system_millis_busy_deadline != 0 &&
- system_millis_busy_deadline < timer_ms()) {
- system_millis_busy_deadline = 0;
+ if (system_millis_busy_length != 0 && !trezor_is_busy()) {
+ system_millis_busy_length = 0;
layoutHome();
}
}
diff --git a/legacy/firmware/trezor.h b/legacy/firmware/trezor.h
index bc8fd936..87a144a7 100644
--- a/legacy/firmware/trezor.h
+++ b/legacy/firmware/trezor.h
@@ -20,6 +20,7 @@
#ifndef __TREZOR_H__
#define __TREZOR_H__
+#include <stdbool.h>
#include <stdint.h>
#include "version.h"
@@ -38,6 +39,7 @@
extern uint32_t system_millis_lock_start;
/* Busyscreen timeout */
-extern uint32_t system_millis_busy_deadline;
+void trezor_set_busy(uint32_t length_ms);
+bool trezor_is_busy(void);
#endif
Why this scored 37/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.