← Watch feed
fix(core): store packet across session restart during channel preemption
Public commit record
77/100 · Adequate
What the developer wrote
Authored by Martin Milata
fix(core): store packet across session restart during channel preemption
This way TRANSPORT_BUSY is not sent and host doesn't have to retransmit
the packet.
[no changelog]
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Uses a recognizable type or scope✓ Provides an explanatory body
What changed, and why it matters
This commit fixes a behavior in Trezor's core firmware where, during a 'channel preemption' (one computer taking over communication with the device from another), the device would send a TRANSPORT_BUSY message and force the new host to resend its packet. The fix stores the incoming packet across the session restart so it can be processed automatically, avoiding the unnecessary busy signal and retransmission. It is a robustness/usability improvement in the device's USB/transport layer rather than a fix for a clear-cut security vulnerability.
Recommended action
Review as a normal bugfix/robustness improvement. Verify that the global PreemptingPacket buffer cannot be abused across interfaces (it is keyed by iface_num and rejects a second set() while full, which limits cross-interface confusion). Confirm that buffer length is capped at MAX_PACKET_LEN (244 bytes) and that the memoryview is consumed before the next set(). No immediate security response is indicated by the diff alone.
Security signals we found
01
Avoids TRANSPORT_BUSY race/retransmission during channel preemption
02
Adds global mutable buffer for a single in-flight packet across session restart
03
Kills active channel with ChannelPreemptedException before storing packet
04
New tests verify timing-dependent preemption behavior
05
No changelog entry despite functional behavior change
Evidence from the diff
The change introduces a global PreemptingPacket buffer (PREEMPTING_PACKET) in storage/cache_thp.py and wires it into trezor/wire/thp/interface_context.py. When InterfaceContext.read_packet_for_channel detects a packet for a channel that is not currently active, it first attempts to preempt the active channel if it has been idle longer than _PREEMPT_TIMEOUT_MS. The new code stores the just-received packet via PREEMPTING_PACKET.set() before killing the active channel. After the loop/session restarts, the same interface’s read loop retrieves the stored packet with PREEMPTING_PACKET.get() and feeds it into read_packet_for_channel() without requiring the host to retransmit. If preemption fails or the active channel is not stale enough, the device still falls back to sending TRANSPORT_BUSY. Tests test_preemption_busy and test_preemption_wait are added to exercise both the busy and stored-packet paths.
Changed components
core/src/storage/cache_thp.pycore/src/trezor/wire/thp/interface_context.pytests/device_tests/thp/test_multiple_hosts.pytests/ui_tests/fixtures.jsonInspect captured patch +140 / −9
diff --git a/core/src/storage/cache_thp.py b/core/src/storage/cache_thp.py
index 1ec2e104..b7474eb4 100644
--- a/core/src/storage/cache_thp.py
+++ b/core/src/storage/cache_thp.py
@@ -243,3 +243,48 @@ def clear_all_except_one_session_keys(excluded: tuple[AnyBytes, AnyBytes]) -> No
s_last_usage = session.last_usage
session.clear()
session.set_int(LAST_USAGE, s_last_usage)
+
+
+# Used to store single packet across loop restart when channel preemption happens.
+class PreemptingPacket:
+ MAX_PACKET_LEN = 244 # maximum packet len across all interface types
+
+ def __init__(self) -> None:
+ self.packet_buffer = bytearray(self.MAX_PACKET_LEN)
+ self.reset()
+
+ def reset(self) -> None:
+ self.iface_num = None
+ self.cid_hint = 0
+ self.packet_len = 0
+
+ def set(self, iface_num: int, cid_hint: int, packet_buffer: AnyBytes) -> bool:
+ """Store packet across session restart.
+
+ cid_hint = channel_id | ((buffer_hint//8) << 16),
+ see InterfaceContext.read_packet_for_channel
+
+ Returns False if a packet is already stored (possibly by other interface).
+ """
+ assert len(packet_buffer) <= self.MAX_PACKET_LEN
+ if self.iface_num is not None:
+ return False
+ self.iface_num = iface_num
+ self.cid_hint = cid_hint
+ self.packet_len = len(packet_buffer)
+ self.packet_buffer[: self.packet_len] = packet_buffer
+ return True
+
+ def get(self, iface_num: int) -> tuple[int, memoryview] | None:
+ """
+ Take the packet if there is one, return None otherwise. The returned
+ memoryview must be processed before `set()` overwrites the contents.
+ """
+ if self.iface_num != iface_num:
+ return None
+ res = (self.cid_hint, memoryview(self.packet_buffer)[: self.packet_len])
+ self.reset()
+ return res
+
+
+PREEMPTING_PACKET = PreemptingPacket()
diff --git a/core/src/trezor/wire/thp/interface_context.py b/core/src/trezor/wire/thp/interface_context.py
index ea2b54ab..e9dba547 100644
--- a/core/src/trezor/wire/thp/interface_context.py
+++ b/core/src/trezor/wire/thp/interface_context.py
@@ -2,7 +2,7 @@ from micropython import const
from typing import TYPE_CHECKING
import trezorthp
-from storage.cache_thp import clear_sessions_without_channel
+from storage.cache_thp import PREEMPTING_PACKET, clear_sessions_without_channel
from trezor import config, io, loop, utils
from trezor.loop import race, wait
@@ -62,17 +62,32 @@ class ThpContext:
assert self.active_channel is not None
return self.active_channel
- def preempt_active_channel_if_stale(self) -> None:
+ def preempt_active_channel_if_stale(
+ self, iface_num: int, cid_hint: int, packet_buffer: AnyBytes
+ ) -> bool:
+ """
+ If the active channel is idle for more than _PREEMPT_TIMEOUT_MS, kill
+ it and save the packet passed as an argument to be processed as if it
+ was received when the next loop session is started.
+
+ Returns True on success, False if the caller should send TRANSPORT_BUSY.
+ """
if not self.active_channel:
- return
+ return False
last_write_ms = self.active_channel.get_last_write()
if last_write_ms is None or last_write_ms > _PREEMPT_TIMEOUT_MS:
+ self.active_channel.kill(ChannelPreemptedException())
+ saved = PREEMPTING_PACKET.set(iface_num, cid_hint, packet_buffer)
if __debug__:
log.error(
__name__,
f"Interrupted channel {hex(self.active_channel.channel_id)} after {last_write_ms} ms",
)
- self.active_channel.kill(ChannelPreemptedException())
+ log.debug(
+ __name__, f"Packet will be processed in next session: {saved}"
+ )
+ return saved
+ return False
async def close(self) -> None:
for iface_ctx in self._iface_ctxs:
@@ -153,6 +168,12 @@ class InterfaceContext:
verify_fn = self.verify_credential
packet_buffer = self._rx_packet_buf
+ if (pep := PREEMPTING_PACKET.get(iface_num)) is not None:
+ if __debug__:
+ log.debug(__name__, "got packet from previous session", iface=iface)
+ cid_hint, buf = pep
+ self.read_packet_for_channel(cid_hint, buf)
+
while True:
while not self.should_read():
if __debug__ and _TRACE:
@@ -177,7 +198,6 @@ class InterfaceContext:
result = trezorthp.packet_in(iface_num, packet_buffer, verify_fn)
if isinstance(result, int):
self.read_packet_for_channel(result, packet_buffer)
- self.clear_closed_sessions()
continue
if __debug__ and _TRACE and result is not None:
@@ -237,10 +257,13 @@ class InterfaceContext:
self.thp_ctx.channel_ready_box.put(None, replace=True)
if self.active_channel is None or self.active_channel.channel_id != channel_id:
- trezorthp.send_transport_busy(channel_id)
- self.inactive_channels.add(channel_id)
- self.request_write()
- self.thp_ctx.preempt_active_channel_if_stale()
+ preempted = self.thp_ctx.preempt_active_channel_if_stale(
+ self._iface.iface_num(), result, packet_buffer
+ )
+ if not preempted:
+ trezorthp.send_transport_busy(channel_id)
+ self.inactive_channels.add(channel_id)
+ self.request_write()
return
try:
@@ -250,6 +273,7 @@ class InterfaceContext:
log.exception(__name__, exc)
self.active_channel.kill(exc)
self.active_channel = None
+ self.clear_closed_sessions()
def write_loop(self) -> Generator[Any, Any, None]:
"""
diff --git a/tests/device_tests/thp/test_multiple_hosts.py b/tests/device_tests/thp/test_multiple_hosts.py
index 50790556..ecc6893c 100644
--- a/tests/device_tests/thp/test_multiple_hosts.py
+++ b/tests/device_tests/thp/test_multiple_hosts.py
@@ -6,6 +6,7 @@ import pytest
from trezorlib.debuglink import TrezorTestContext
from trezorlib.thp.channel import Channel
from trezorlib.thp.exceptions import ThpError, ThpErrorCode
+from trezorlib.thp.pairing import PairingController
from .connect import prepare_channel_for_pairing
@@ -102,3 +103,52 @@ def test_concurrent_channels(test_ctx: TrezorTestContext) -> None:
for channel in channels[1:]:
test_ctx.channel = channel
test_ctx.ping("hi2")
+
+
+def _open_channel_no_retries(test_ctx: TrezorTestContext) -> Channel:
+ new_channel = Channel.allocate(test_ctx.transport)
+ new_channel.open(credentials=[])
+ new_channel.BUSY_RETRIES = 0
+ return new_channel
+
+
+def _replace_channel_and_do_pairing(
+ test_ctx: TrezorTestContext, channel: Channel
+) -> None:
+ test_ctx.channel = channel
+ test_ctx.client._interact_ctx = test_ctx.client._interact()
+ test_ctx.client.pairing = PairingController(test_ctx.client)
+ test_ctx.client.pairing.skip()
+ test_ctx.client.pairing.finish()
+
+
+# It's possible for this test to fail if CI is very slow. If this happens
+# we should first try marking it with @pytest.mark.flaky(retries=5)
+def test_preemption_busy(test_ctx: TrezorTestContext) -> None:
+ channel_1 = _open_channel_no_retries(test_ctx)
+ channel_2 = _open_channel_no_retries(test_ctx)
+
+ # GetFeatures is in AVOID_RESTARTING_FOR and keeps channel active
+ test_ctx.refresh_features()
+
+ # hopefully less than _PREEMPT_TIMEOUT_MS passed and we get TRANSPORT_BUSY
+ with pytest.raises(ThpError, match="TRANSPORT_BUSY"):
+ _replace_channel_and_do_pairing(test_ctx, channel_1)
+ # channel_1 is desynced now
+
+ time.sleep(1.1) # _PREEMPT_TIMEOUT_MS + epsilon
+
+ # initial test_ctx.channel is preempted, no TRANSPORT_BUSY is sent
+ _replace_channel_and_do_pairing(test_ctx, channel_2)
+
+
+def test_preemption_wait(test_ctx: TrezorTestContext) -> None:
+ channel_1 = _open_channel_no_retries(test_ctx)
+
+ # GetFeatures is in AVOID_RESTARTING_FOR and keeps channel active
+ test_ctx.refresh_features()
+
+ time.sleep(1.1) # _PREEMPT_TIMEOUT_MS + epsilon
+
+ # no TRANSPORT_BUSY after _PREEMPT_TIMEOUT_MS
+ _replace_channel_and_do_pairing(test_ctx, channel_1)
diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json
index 7b319970..6567ccec 100644
--- a/tests/ui_tests/fixtures.json
+++ b/tests/ui_tests/fixtures.json
@@ -36213,6 +36213,8 @@
"T3W1_cs_thp-test_multiple_hosts.py::test_concurrent_channels": "5c78624eed096a49a8dbbffc88cde453c24a12aebba816b29f07ab9b92f00070",
"T3W1_cs_thp-test_multiple_hosts.py::test_concurrent_handshakes": "2b06a6c36a96faaad9d1237b47e37374c5452331ad8fa3738accbd2e09ec1500",
"T3W1_cs_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "2b06a6c36a96faaad9d1237b47e37374c5452331ad8fa3738accbd2e09ec1500",
+"T3W1_cs_thp-test_multiple_hosts.py::test_preemption_busy": "328ffdadc28837d3a341a676d953ec691439edd46c6601f1034b5a2337819e13",
+"T3W1_cs_thp-test_multiple_hosts.py::test_preemption_wait": "328ffdadc28837d3a341a676d953ec691439edd46c6601f1034b5a2337819e13",
"T3W1_cs_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "54b78c31ccf8a3750e92e187765166b25164c64810e2a02b0da1ea3a0c50878c",
"T3W1_cs_thp-test_pairing.py::test_channel_replacement": "c23ce500a4f20d0defb880d9f4af28e70ce3bd43bd48dc4d335b8261d28bb81b",
"T3W1_cs_thp-test_pairing.py::test_connection_confirmation_cancel": "45ead11a7a7d7c1ef2772cdf9e393081981f996dcd46165921c0d22caef86693",
@@ -38025,6 +38027,8 @@
"T3W1_de_thp-test_multiple_hosts.py::test_concurrent_channels": "fda760554b47497c35b92b1ef09521bb83a1d67fb413198d7ac8ba96612e3223",
"T3W1_de_thp-test_multiple_hosts.py::test_concurrent_handshakes": "32c06c9f4ef6d91d183e5480b273a0aec9bd56572528ddbfb70b7cd609cbef50",
"T3W1_de_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "32c06c9f4ef6d91d183e5480b273a0aec9bd56572528ddbfb70b7cd609cbef50",
+"T3W1_de_thp-test_multiple_hosts.py::test_preemption_busy": "b9d98afd8f5ba9989e9e211a73a2dfec157382f161278f4d332c312d1ee6553b",
+"T3W1_de_thp-test_multiple_hosts.py::test_preemption_wait": "b9d98afd8f5ba9989e9e211a73a2dfec157382f161278f4d332c312d1ee6553b",
"T3W1_de_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "2890efe790592e3b4be654453b97c7837fe1772893576dae50a4e26fe352488f",
"T3W1_de_thp-test_pairing.py::test_channel_replacement": "55e1079ccf5e4830b1e2a61bf87b47e1d9c63cdb115d90b05ab0f3af91dbbb59",
"T3W1_de_thp-test_pairing.py::test_connection_confirmation_cancel": "187df95f87bca31a5255300d3fcf616c9209cebdd740fa43caf0ec2d43a2063b",
@@ -39837,6 +39841,8 @@
"T3W1_en_thp-test_multiple_hosts.py::test_concurrent_channels": "a57583ba03fc58ce06c970d79ac99c7410c1dcebf69dd77cd3c6068bed2c766a",
"T3W1_en_thp-test_multiple_hosts.py::test_concurrent_handshakes": "90812f196e8c35dc7f35cb5a9a0e553c8fc6a610812d85b9f166e90b3f1c8b59",
"T3W1_en_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "90812f196e8c35dc7f35cb5a9a0e553c8fc6a610812d85b9f166e90b3f1c8b59",
+"T3W1_en_thp-test_multiple_hosts.py::test_preemption_busy": "2933009bfde73eab94a97779bb128f0f693db1c721fbb187af6a379aa76005ef",
+"T3W1_en_thp-test_multiple_hosts.py::test_preemption_wait": "2933009bfde73eab94a97779bb128f0f693db1c721fbb187af6a379aa76005ef",
"T3W1_en_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "599865f2889976594c967c1d7c3621dce019222a501bd2cbe41b306436fd4039",
"T3W1_en_thp-test_pairing.py::test_channel_replacement": "482cefa858cbd525b63e6b54ac8a420b576205dcdf3580b70fb62e034f5c96f0",
"T3W1_en_thp-test_pairing.py::test_connection_confirmation_cancel": "7c04017f454f864bf95538ad0bfd4259924d282baa85fbdfb7f87550f8652ed1",
@@ -41649,6 +41655,8 @@
"T3W1_es_thp-test_multiple_hosts.py::test_concurrent_channels": "d9c35303d31266fd4cb5dae297af2964053a8a4d062b10b7d511588b49874c85",
"T3W1_es_thp-test_multiple_hosts.py::test_concurrent_handshakes": "ddbd1412a9f69aea1c41c375a2ec0647411a0e487ec5c1ee55e24505dfc34e29",
"T3W1_es_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "ddbd1412a9f69aea1c41c375a2ec0647411a0e487ec5c1ee55e24505dfc34e29",
+"T3W1_es_thp-test_multiple_hosts.py::test_preemption_busy": "2691c5562f9919e57364b5b9ed6a4674b65c4d428424c56094ec5e094f5b4081",
+"T3W1_es_thp-test_multiple_hosts.py::test_preemption_wait": "2691c5562f9919e57364b5b9ed6a4674b65c4d428424c56094ec5e094f5b4081",
"T3W1_es_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "04e11d1fa337e01679e807a98e8e024ba03e3d46f392e3930d40e343b9226621",
"T3W1_es_thp-test_pairing.py::test_channel_replacement": "87d6aae972971e4ad2a67b1ae097d54bbbce36596e42743a8ade7d93615becf1",
"T3W1_es_thp-test_pairing.py::test_connection_confirmation_cancel": "413751e567dac7c057c86588cafbcef6a9ca51e78def7d91d8dc831b18651b5b",
@@ -43461,6 +43469,8 @@
"T3W1_fr_thp-test_multiple_hosts.py::test_concurrent_channels": "ad64999bce0563dd0a98149aef9ae4ee5d09cf190859dc27842a5930b6729a29",
"T3W1_fr_thp-test_multiple_hosts.py::test_concurrent_handshakes": "a392ca1dea666647fb2d6edfcd90e4e3cad6ab0e294ca85cda7db478214a6a81",
"T3W1_fr_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "a392ca1dea666647fb2d6edfcd90e4e3cad6ab0e294ca85cda7db478214a6a81",
+"T3W1_fr_thp-test_multiple_hosts.py::test_preemption_busy": "bb0eaa1e09b123b1e3f897643aaa99c3483307d4d9905fd0bed5bebf115e410d",
+"T3W1_fr_thp-test_multiple_hosts.py::test_preemption_wait": "bb0eaa1e09b123b1e3f897643aaa99c3483307d4d9905fd0bed5bebf115e410d",
"T3W1_fr_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "66f65de1dba88896b7d2f3b634be22c53d04e9cdefd08361165ad9aecc134d42",
"T3W1_fr_thp-test_pairing.py::test_channel_replacement": "95ac9e37bfa5db7fcd5ffabf4c888264db5174f3d5e3e33b44137a9078df2122",
"T3W1_fr_thp-test_pairing.py::test_connection_confirmation_cancel": "99b6bcfde0b9b1dc991d4d21eea02d11ec146477049f5f99291de280c107e3aa",
@@ -45278,6 +45288,8 @@
"T3W1_pt_thp-test_multiple_hosts.py::test_concurrent_channels": "0ef560392cf3716bb70ef2a9911c87c63af9d074a9af4b265efe31afa75c653e",
"T3W1_pt_thp-test_multiple_hosts.py::test_concurrent_handshakes": "6fb22dca4b252028ed3f91db1fb746e30d12aece0a74e1bcc4abef605a3a9026",
"T3W1_pt_thp-test_multiple_hosts.py::test_concurrent_handshakes_busy_retries": "6fb22dca4b252028ed3f91db1fb746e30d12aece0a74e1bcc4abef605a3a9026",
+"T3W1_pt_thp-test_multiple_hosts.py::test_preemption_busy": "94cf292edc4975016fbf38031258fdd1f85434d0e824d700f506622a268c9085",
+"T3W1_pt_thp-test_multiple_hosts.py::test_preemption_wait": "94cf292edc4975016fbf38031258fdd1f85434d0e824d700f506622a268c9085",
"T3W1_pt_thp-test_pairing.py::test_autoconnect_credential_request_cancel": "f0c8717c76180ef5c8befd05c5b112940c543eacbb8155f73667fb301efb3aed",
"T3W1_pt_thp-test_pairing.py::test_channel_replacement": "a07ddec3a2d5f94ca60fcf53469ba9c7a2c5c3da78e8f09567976dcc0efe6c55",
"T3W1_pt_thp-test_pairing.py::test_connection_confirmation_cancel": "eb053e9d006b42d06e9649ee434888f00e919b097931d2aca6bbaea513b719d4",
Risk score
Our methodology →Why this scored 37/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.