feat(core): Optimize and generalize tropic_data_multi_read().
What the developer wrote
Authored by Andrew Kozlik
feat(core): Optimize and generalize tropic_data_multi_read().
What changed, and why it matters
This commit rewrites a function that reads data stored across multiple secure memory slots on Trezor's Tropic secure element. The rewrite reduces memory usage and avoids reading unused slots, but it also changes how the code checks slot boundaries and how it validates that the declared data length actually fits within the requested slot range. A subtle bug in the new code could allow reading more slots than intended or mishandling short terminal slots, though the commit appears aimed at hardening rather than introducing a vulnerability.
Treat as a code-quality and potential security fix requiring review. Verify whether the bare 'false;' line is a typo and should be 'return false;'. Add tests covering edge cases: slot_count overflow, out_length larger than max_data_length, occupied_slot_count larger than slot_count, terminal slot padding, and partial terminal slots. Consider running static analysis to catch the ineffective statement.
Security signals we found
Bounds-checking logic rewritten for multi-slot secure-element read
New slot_count overflow check using R_MEM_DATA_SLOT_MAX + 1 - first_slot
New validation that declared out_length fits in max_data_length
New validation that occupied_slot_count does not exceed slot_count
Likely ineffective statement 'false;' instead of 'return false;' after first max_data_length check
Reduced stack memory footprint by reading one slot at a time
Non-terminal slots now required to be fully utilized
Evidence from the diff
The patch refactors tropic_data_multi_read() in core/embed/sec/tropic/tropic.c. Previously it allocated a buffer for all slots, read every slot up to slot_count, then parsed a 2-byte length prefix and copied that many bytes. The new version reads only the first slot, derives the expected total length and occupied_slot_count from the prefix, then reads only the remaining occupied slots. It adds checks: first_slot <= R_MEM_DATA_SLOT_MAX, slot_count <= R_MEM_DATA_SLOT_MAX + 1 - first_slot, out_length <= max_data_length, occupied_slot_count <= slot_count, and requires non-terminal slots to be full. However, there is a likely typo where a bare ‘false;’ statement appears instead of ‘return false;’ after the first max_data_length check, meaning the check has no effect. Also, the loop’s terminal-slot copy uses min(slot_length, out_length - out_pos) without re-checking max_data_length for the non-first slots, relying instead on the earlier out_length <= max_data_length check. The change is plausibly a security/robustness improvement, but the partial nature and the suspicious ‘false;’ line lower confidence that the patch is fully correct.
Changed components
core/embed/sec/tropic/tropic.ctropic_data_multi_read()Tropic secure-element data-slot readingInspect captured patch +47 / −20
diff --git a/core/embed/sec/tropic/tropic.c b/core/embed/sec/tropic/tropic.c
index 94eddd725..4ca1d6540 100644
--- a/core/embed/sec/tropic/tropic.c
+++ b/core/embed/sec/tropic/tropic.c
@@ -206,45 +206,72 @@ bool tropic_data_multi_size(uint16_t first_slot, size_t *data_length) {
return true;
}
+static size_t min(size_t x, size_t y) { return (x < y) ? x : y; }
+
bool tropic_data_multi_read(uint16_t first_slot, uint16_t slot_count,
uint8_t *data, size_t max_data_length,
size_t *data_length) {
- const uint16_t last_data_slot = first_slot + slot_count - 1;
- if (slot_count == 0 || last_data_slot > R_MEM_DATA_SLOT_MAX) {
+ if (first_slot > R_MEM_DATA_SLOT_MAX || slot_count == 0 ||
+ slot_count > R_MEM_DATA_SLOT_MAX + 1 - first_slot) {
return false;
}
- // The following code can be further optimized:
- // * It uses unnecessary amount of memory.
- // * It reads from a data slot even if there is no data to be read.
-
- const size_t total_slots_length = R_MEM_DATA_SIZE_MAX * slot_count;
- uint8_t prefixed_data[total_slots_length];
- size_t position = 0;
uint16_t slot = first_slot;
+ uint8_t slot_buffer[R_MEM_DATA_SIZE_MAX] = {0};
+ uint16_t slot_length = 0;
+ if (!tropic_data_read(slot, slot_buffer, &slot_length)) {
+ return false;
+ }
- while (slot <= last_data_slot) {
- uint16_t slot_length = 0;
- if (!tropic_data_read(slot, prefixed_data + position, &slot_length)) {
- return false;
- }
+ const size_t prefix_length = 2;
+ if (slot_length < prefix_length) {
+ return false;
+ }
+
+ size_t out_length = slot_buffer[0] << 8 | slot_buffer[1];
+ uint16_t occupied_slot_count =
+ (out_length + prefix_length + R_MEM_DATA_SIZE_MAX - 1) /
+ R_MEM_DATA_SIZE_MAX;
+ if (out_length > max_data_length || occupied_slot_count > slot_count) {
+ return false;
+ }
+
+ size_t out_pos = 0;
+ // Terminal slots may be padded. Make sure not to copy beyond the actual data
+ // length.
+ size_t copy_length = min(slot_length - prefix_length, out_length - out_pos);
+ if (out_pos + copy_length > max_data_length) {
+ false;
+ }
+ memcpy(&data[out_pos], &slot_buffer[prefix_length], copy_length);
+ out_pos += copy_length;
+ uint16_t last_data_slot = first_slot + occupied_slot_count - 1;
+ while (slot < last_data_slot) {
+ // Non-terminal slots must be used to their full capacity.
if (slot_length != R_MEM_DATA_SIZE_MAX) {
return false;
}
- position += R_MEM_DATA_SIZE_MAX;
+ // Read next slot.
slot += 1;
+ if (!tropic_data_read(slot, slot_buffer, &slot_length)) {
+ return false;
+ }
+
+ // Terminal slots may be padded. Make sure not to copy beyond the actual
+ // data length.
+ copy_length = min(slot_length, out_length - out_pos);
+ memcpy(&data[out_pos], slot_buffer, copy_length);
+ out_pos += copy_length;
}
- const size_t prefix_length = 2;
- size_t length = prefixed_data[0] << 8 | prefixed_data[1];
- if (length > max_data_length || length + prefix_length > total_slots_length) {
+ if (out_pos != out_length) {
+ // The terminal slot had less data than expected.
return false;
}
- *data_length = length;
- memcpy(data, prefixed_data + prefix_length, length);
+ *data_length = out_length;
return true;
}
Why this scored 41/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.