refactor(core): remove special handling THP ACKs from `Reassembler`
What the developer wrote
Authored by Roman Zeyde
refactor(core): remove special handling THP ACKs from `Reassembler`
Once a message is retrieved over a cached channel, the THP buffers
will be owned by this channel until an event loop restart.
Thus, the `if control_byte.is_ack(ctrl_byte)` branch is not helping
to avoid buffer allocation - and should be removed to simplify the flow.
[no changelog]
What changed, and why it matters
This commit removes a small optimization in the Trezor firmware's THP (Trezor Host Protocol) message reassembly code. Previously, small ACK (acknowledgment) packets used a slice of the existing packet buffer instead of allocating a separate buffer. Now all packets, including ACKs, use the same buffer allocation path. The change is described by the developer as a simplification that does not affect security because the buffers are already owned by the channel. There is no direct evidence of a security vulnerability being fixed.
No immediate action required. Treat as a routine refactoring. If auditing, verify that `thp_read_buf.get(self.buffer_len)` always returns a buffer of at least `self.buffer_len` for small ACK-sized payloads and that `_buffer_packet_data` correctly handles the full packet length.
Security signals we found
Removal of a special-case code path for ACK packets
Change to buffer allocation behavior in protocol reassembly
No explicit security claim or CVE reference in commit
No changelog entry ([no changelog])
Evidence from the diff
In core/src/trezor/wire/thp/channel.py, the Reassembler class no longer special-cases THP ACK packets. The removed branch avoided calling self.thp_read_buf.get(self.buffer_len) for ACKs by slicing packet[: self.buffer_len] and setting bytes_read to the full length. After the change, all initial packets allocate from thp_read_buf and copy data via _buffer_packet_data. The commit message states this is safe because THP buffers remain owned by the cached channel until an event loop restart, so the optimization was not actually avoiding allocation. The diff is small (+2/-7) and contains no bounds-checking, parsing, or cryptographic changes.
Changed components
core/src/trezor/wire/thp/channel.pyTrezor Host Protocol (THP) ReassemblerInspect captured patch +2 / −7
diff --git a/core/src/trezor/wire/thp/channel.py b/core/src/trezor/wire/thp/channel.py
index c9515e2a8..52d02e9ce 100644
--- a/core/src/trezor/wire/thp/channel.py
+++ b/core/src/trezor/wire/thp/channel.py
@@ -94,13 +94,8 @@ class Reassembler:
_, _, payload_length = ustruct.unpack(PacketHeader.INIT_FORMAT, packet)
self.buffer_len = payload_length + PacketHeader.INIT_LENGTH
- if control_byte.is_ack(ctrl_byte):
- # don't allocate buffer for ACKs (since they are small)
- buffer = packet[: self.buffer_len]
- self.bytes_read = len(buffer)
- else:
- buffer = self.thp_read_buf.get(self.buffer_len)
- self._buffer_packet_data(buffer, packet, 0)
+ buffer = self.thp_read_buf.get(self.buffer_len)
+ self._buffer_packet_data(buffer, packet, 0)
assert len(buffer) == self.buffer_len
if self.bytes_read < self.buffer_len:
Why this scored 26/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.