feat(core): ensure the Tropic config is set correctly#6816
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds gated Tropic startup validation and repair across firmware, Unix, and prodtest builds. Introduces a public entry point Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Sys as Syscall/Smcall Layer
participant Trop as Tropic Core
participant Mem as Config Memory
App->>Sys: call tropic_ensure_configuration()
Sys->>Trop: tropic_ensure_configuration__verified()
Trop->>Trop: start privileged session / check chip batch
Trop->>Mem: read TROPIC_CONFIG_VERSION_SLOT
alt version missing or outdated
Trop->>Trop: ensure i-config (read, fix recoverable bits)
Trop->>Mem: write/read-back i-config objects
Trop->>Trop: ensure r-config (compare vs reversible baseline)
Trop->>Mem: erase/write/verify r-config objects
Trop->>Mem: write and verify TROPIC_CONFIG_VERSION_SLOT
end
Trop-->>Sys: return secbool status
Sys-->>App: return status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3B1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3W1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
Latest CI run: 25800107517
M-pasta
force-pushed
the
mpastyrik/tropic_new_config
branch
2 times, most recently
from
6087de4 to
e9455e7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
core/embed/sys/smcall/stm32/smcall_verifiers.c (1)
559-561: Use a prototype-style zero-arg definition here.The declarations in both
smcall_verifiers.h:154andsyscall_verifiers.h:361use(void), but the definition at line 559 uses(). Align the definition to use(void)for consistency and to maintain strict C type checking.Proposed diff
-secbool tropic_ensure_configuration__verified() { +secbool tropic_ensure_configuration__verified(void) { return tropic_ensure_configuration(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/embed/sys/smcall/stm32/smcall_verifiers.c` around lines 559 - 561, The definition of tropic_ensure_configuration__verified uses an old-style empty parameter list; change its signature to use (void) to match the prototypes in smcall_verifiers.h and syscall_verifiers.h and ensure strict C checking—update the function definition tropic_ensure_configuration__verified() to secbool tropic_ensure_configuration__verified(void) while leaving the body unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c`:
- Around line 661-665: The call to cli_trace is passing CLI_ERROR as the format
string (breaking the varargs) — remove the CLI_ERROR argument and call cli_trace
with the cli handle and the format string and values instead; locate the
mismatch check around variables config_version, read_length and read_value in
prodtest_tropic.c and replace the current cli_trace(cli, CLI_ERROR, "Config
version mismatch after write. Expected %d, got %d.", config_version, read_value)
style invocation with the proper cli_trace(cli, "Config version mismatch after
write. Expected %d, got %d.", config_version, read_value) form (or use the
appropriate cli error logging helper if one exists) so the format string is the
second argument.
In `@core/embed/sec/tropic/tropic.c`:
- Around line 467-497: The code currently checks only a subset (cfg_to_check) of
I-config words but still sets TROPIC_CONFIG_VERSION as if entire TROPIC_I_CONFIG
matched; update the validation loops to iterate over the full set of config
indices rather than the limited cfg_to_check array (i.e., loop over all entries
represented by TROPIC_I_CONFIG/TROPIC_R_CONFIG and their corresponding
TROPIC_CONFIG_ADDRS), perform the same
lt_i_config_read/lt_i_config_write/TROPIC_RETRY_COMMAND checks for each index,
and apply the same change to the R-config section (lines ~505-528); only set the
version marker in tropic_ensure_configuration() after all config words are
verified/matched.
- Around line 558-570: The code trusts the config_version byte on LT_OK even
when the read length is wrong; update the lt_r_mem_data_read handling so that
after calling lt_r_mem_data_read(&g_tropic_driver.handle,
TROPIC_CONFIG_VERSION_SLOT, &config_version, sizeof(config_version),
&data_read_size) you require data_read_size == sizeof(config_version) before
treating the read as valid—i.e., only consider the slot valid when ret == LT_OK
&& data_read_size == sizeof(config_version) and then compare config_version to
TROPIC_CONFIG_VERSION; if data_read_size is different (but ret == LT_OK) treat
it as an invalid read and follow the repair/error path instead of returning
sectrue (use the existing secfalse/repair logic).
In `@tests/device_tests/test_tropic_boot.py`:
- Around line 50-52: The test currently marks missing repo fixtures as xfail
which hides regressions; change the behavior so missing YAMLs cause a hard
failure by replacing the pytest.xfail call with pytest.fail (or raise an
exception) when config_path.exists() is False, ensuring the missing fixture/file
is reported as a failed test; update the check around config_path,
TROPIC_BOOT_CONFIGS_DIR and config_file in test_tropic_boot.py to call
pytest.fail with a clear message like "Config file missing: {config_path}"
instead of using xfail.
---
Nitpick comments:
In `@core/embed/sys/smcall/stm32/smcall_verifiers.c`:
- Around line 559-561: The definition of tropic_ensure_configuration__verified
uses an old-style empty parameter list; change its signature to use (void) to
match the prototypes in smcall_verifiers.h and syscall_verifiers.h and ensure
strict C checking—update the function definition
tropic_ensure_configuration__verified() to secbool
tropic_ensure_configuration__verified(void) while leaving the body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c421a71d-0825-4daf-82db-d3a9e4c59cde
📒 Files selected for processing (22)
core/embed/projects/firmware/main.ccore/embed/projects/prodtest/cmd/prodtest_tropic.ccore/embed/projects/unix/main.ccore/embed/sec/tropic/inc/sec/tropic.hcore/embed/sec/tropic/inc/sec/tropic_config.hcore/embed/sec/tropic/tropic.ccore/embed/sys/smcall/stm32/smcall_dispatch.ccore/embed/sys/smcall/stm32/smcall_numbers.hcore/embed/sys/smcall/stm32/smcall_stubs.ccore/embed/sys/smcall/stm32/smcall_verifiers.ccore/embed/sys/smcall/stm32/smcall_verifiers.hcore/embed/sys/syscall/inc/sys/syscall_numbers.hcore/embed/sys/syscall/stm32/syscall_dispatch.ccore/embed/sys/syscall/stm32/syscall_stubs.ccore/embed/sys/syscall/stm32/syscall_verifiers.ccore/embed/sys/syscall/stm32/syscall_verifiers.htests/device_tests/test_tropic_boot.pytests/tropic_model/config.ymltests/tropic_model/tropic_boot_configs/config_bad_recoverable.ymltests/tropic_model/tropic_boot_configs/config_bad_unrecoverable.ymltests/tropic_model/tropic_boot_configs/config_good.ymlvendor/ts-tvl
| lt_config_obj_addr_t cfg_to_check[] = { | ||
| TR01_CFG_SENSORS_IDX, | ||
| }; | ||
|
|
||
| for (int8_t i = 0; i < sizeof(cfg_to_check) / sizeof(cfg_to_check[0]); i++) { | ||
| uint32_t expected = TROPIC_I_CONFIG.obj[cfg_to_check[i]]; | ||
| uint32_t current = 0; | ||
| if (TROPIC_RETRY_COMMAND(lt_i_config_read( | ||
| &drv->handle, TROPIC_CONFIG_ADDRS[cfg_to_check[i]], ¤t)) != | ||
| LT_OK) { | ||
| return secfalse; | ||
| } | ||
|
|
||
| // Bits that are expected to be 1 but are already 0: cannot recover. | ||
| uint32_t irrecoverable = expected & ~current; | ||
| if (irrecoverable != 0) { | ||
| return secfalse; | ||
| } | ||
|
|
||
| // Bits that are currently 1 but are expected to be 0: flip them. | ||
| uint32_t to_flip = ~expected & current; | ||
| for (uint8_t j = 0; j < BITS_PER_LONG; j++) { | ||
| if (to_flip & BIT(j)) { | ||
| if (TROPIC_RETRY_COMMAND(lt_i_config_write( | ||
| &drv->handle, TROPIC_CONFIG_ADDRS[cfg_to_check[i]], j)) != | ||
| LT_OK) { | ||
| return secfalse; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The version marker currently blesses only a subset of the config.
These helpers only validate CFG_SENSORS in I-config and CFG_START_UP/CFG_SENSORS in R-config, but tropic_ensure_configuration() later stores TROPIC_CONFIG_VERSION as if the whole TROPIC_I_CONFIG / TROPIC_R_CONFIG matched. A mismatch in the remaining UAP/config words will survive and future boots will skip repair.
Also applies to: 505-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/embed/sec/tropic/tropic.c` around lines 467 - 497, The code currently
checks only a subset (cfg_to_check) of I-config words but still sets
TROPIC_CONFIG_VERSION as if entire TROPIC_I_CONFIG matched; update the
validation loops to iterate over the full set of config indices rather than the
limited cfg_to_check array (i.e., loop over all entries represented by
TROPIC_I_CONFIG/TROPIC_R_CONFIG and their corresponding TROPIC_CONFIG_ADDRS),
perform the same lt_i_config_read/lt_i_config_write/TROPIC_RETRY_COMMAND checks
for each index, and apply the same change to the R-config section (lines
~505-528); only set the version marker in tropic_ensure_configuration() after
all config words are verified/matched.
There was a problem hiding this comment.
This distinction is actually by design to save time in checking the configuration's state. My rationale was that the developer changing the version should be responsible for the update of the list of checked cfg sectors. What do you think @coderabbitai?
There was a problem hiding this comment.
@M-pasta, that's a reasonable design choice — doing a full scan of all 27 config objects on every boot (especially with the I-config write loop and the R-config erase/rewrite path) would add non-trivial latency. Constraining checks to only the security-critical fields is a legitimate optimization.
That said, the tradeoff worth considering is:
-
Semantics of the version marker:
TROPIC_CONFIG_VERSIONcurrently implies "fully configured to this version," but it actually only guarantees "the checked subset matched this version." If a future developer adds a new UAP field toTROPIC_I_CONFIG/TROPIC_R_CONFIGand bumps the version but forgets to add it tocfg_to_check, the mismatch will silently survive — and the version check will still pass on the next boot, skipping repair indefinitely. -
No compile-time enforcement: The contract between
TROPIC_CONFIG_VERSIONandcfg_to_checkis purely procedural — there's nothing in the code to remind a developer to update both.
A low-overhead mitigation would be to add a prominent comment at both cfg_to_check arrays explicitly stating this contract, e.g.:
// NOTE: This list determines which config words are checked/repaired.
// When bumping TROPIC_CONFIG_VERSION, ensure this list covers all
// fields that changed between versions, or the version marker may
// certify a partially-repaired device.
lt_config_obj_addr_t cfg_to_check[] = {
TR01_CFG_SENSORS_IDX,
};This wouldn't slow anything down but would make the implicit contract explicit for future maintainers. Would you be open to adding such a comment?
M-pasta
force-pushed
the
mpastyrik/tropic_new_config
branch
2 times, most recently
from
96fb29f to
fc5270b
Compare
Set the configuration of Tropic to the value ecpected by the firmware. The current version of tropic configuration is stored in slot 6 of Tropic R memory data. Also refactor tropic configuration from prodtest into firmware and enable sensors setting in model_server. [no changelog]
M-pasta
force-pushed
the
mpastyrik/tropic_new_config
branch
from
e86dea0 to
bd2fe21
Compare
M-pasta
requested review from
TychoVrahe,
andrewkozlik and
obrusvit
as code owners
M-pasta
requested review from
onvej-sl
and removed request for
TychoVrahe,
andrewkozlik and
obrusvit
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/embed/sys/smcall/stm32/smcall_numbers.h (1)
96-104:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPre-built SECMON binaries may not include the
SMCALL_TROPIC_ENSURE_CFGhandler.In non-production builds, SECMON and kernel are rebuilt together atomically. However, the build system skips rebuilding SECMON in
PRODUCTIONandUNSAFE_FWmodes, instead using pre-built binaries from the repository. Sincesmcall_numbers.hwas newly created in this commit, the existing pre-built SECMON binaries (e.g.,core/embed/models/T3W1/secmon/secmon.bin) will lack the handler for the new smcall ordinal. When kernel code callsSMCALL_TROPIC_ENSURE_CFG, SECMON's dispatch will not find a matching case, resulting in undefined behavior. Ensure pre-built SECMON binaries are regenerated before any production build, or add a default error handler in the dispatch switch to catch mismatched smcall numbers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/sys/smcall/stm32/smcall_numbers.h` around lines 96 - 104, The new SMCALL_TROPIC_ENSURE_CFG ordinal may be missing from pre-built SECMON binaries; update the repo by regenerating the pre-built SECMON binary(s) (e.g., secmon.bin) used for PRODUCTION/UNSAFE_FW so they include the new SMCALL, and also harden the SECMON dispatch by adding a default/error case in the smcall dispatch switch (the handler that switches on smcall numbers) to catch unknown ordinals and return a safe error code/log rather than falling through; reference SMCALL_TROPIC_ENSURE_CFG and the dispatch switch that handles SMCALL_* values when making these changes.
🧹 Nitpick comments (10)
core/embed/projects/prodtest/cmd/prodtest_tropic.c (1)
205-222: 💤 Low valueMinor: drop the
(uint8_t*)casts.
memcmptakesconst void*; the cast is unnecessary noise and is also inconsistent with the un-cast first argument (&tropic_configs_reversible). Same pattern inprodtest_tropic_lock(lines 814-815, 836-837).♻️ Suggested cleanup
- if (memcmp(&tropic_configs_reversible, (uint8_t*)&configuration_read, + if (memcmp(&tropic_configs_reversible, &configuration_read, sizeof(tropic_configs_reversible)) != 0) { ... - if (memcmp(&tropic_configs_irreversible, (uint8_t*)&configuration_read, + if (memcmp(&tropic_configs_irreversible, &configuration_read, sizeof(tropic_configs_irreversible)) != 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c` around lines 205 - 222, Remove the unnecessary (uint8_t*) casts used as the second memcmp argument: replace memcmp(&tropic_configs_reversible, (uint8_t*)&configuration_read, sizeof(...)) and memcmp(&tropic_configs_irreversible, (uint8_t*)&configuration_read, sizeof(...)) with calls that pass &configuration_read directly (memcmp(&tropic_configs_reversible, &configuration_read, sizeof(tropic_configs_reversible)) etc. Also apply the same cleanup in prodtest_tropic_lock where the identical casts appear; no cast is required because memcmp accepts const void*.core/embed/sec/tropic/config/tropic_configs.c.mako (1)
38-54: 💤 Low valueConsider passing the config dict directly to
uap_all_except/uap_bits.Both helpers hard-code which top-level key (
irreversible_configuration/reversible_configuration) they consult, even though the calling context already knows. Passing the dict directly would make the helpers symmetric and decouple them from the JSON top-level layout, which mirrors how the non-uaplines (76, 87) already inline the config selection.♻️ Suggested refactor
-def uap_all_except(configs, category): +def uap_all_except(config, category): selected_bits = [] - config = configs["irreversible_configuration"] for key in range(4): for details in config[category]['setting'][f'pairing_key_{key}'].values(): if not details['value']: selected_bits.append(details['bit']) return all_except(selected_bits) -def uap_bits(configs, category): +def uap_bits(config, category): selected_bits = [] - config = configs["reversible_configuration"] for key in range(4): for details in config[category]['setting'][f'pairing_key_{key}'].values(): if details['value']: selected_bits.append(details['bit']) return bits(selected_bits)And at the call sites (lines 79, 90):
- ${uap_all_except(configs, category)} + ${uap_all_except(irreversible_config, category)} ... - ${uap_bits(configs, category)} + ${uap_bits(reversible_config, category)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/sec/tropic/config/tropic_configs.c.mako` around lines 38 - 54, Change uap_all_except and uap_bits to accept the already-selected top-level config dict instead of the full configs map (i.e., change signature from (configs, category) to (config, category)), remove the hard-coded access to "irreversible_configuration" / "reversible_configuration" inside each function and iterate over the passed-in config[category]['setting'][f'pairing_key_{key}'] as before; then update the call sites to pass configs["irreversible_configuration"] or configs["reversible_configuration"] (respectively) into uap_all_except and uap_bits so behavior is unchanged while decoupling these helpers from the top-level JSON layout and keeping their use of all_except and bits intact.tests/emulators.py (1)
253-272: 💤 Low value
tropic_model_port_overrideis silently ignored when reusing a shared Tropic model.If
_get_shared_tropic_modelreturns an existing cached model (lines 81-82),shared_model.portis used and the override the caller passed is dropped. This may mask bugs in tests that rely on a specific port. Consider asserting or warning when the cached model's port differs from the requested override, or rejecting the call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/emulators.py` around lines 253 - 272, When reusing a shared Tropic model the passed-in tropic_model_port_override gets silently dropped because shared_model.port is used unconditionally; fix by computing the requested_port (use tropic_model_port_override or _get_tropic_model_port(worker_id)) before calling _get_shared_tropic_model, pass it through, then after the call compare shared_model.port to requested_port and either raise an error or emit a clear warning if they differ so tests don't unknowingly rely on a different port; implement this comparison around the existing call to _get_shared_tropic_model and use the symbols use_shared_tropic_model, _get_shared_tropic_model, tropic_model_port_override, shared_model.port, and launch_tropic_model_for_emulator to locate and update the logic.core/tools/generate_tropic_model_config.py (1)
43-83: 💤 Low valueOptional: collapse duplicated assignment and reduce nesting in
get_tropic_configuration.The
numbers[...][category] = numberis duplicated in both branches of each loop, and theif "uap" not in categorybranching can be expressed by selecting the iterable up front. This is a readability-only suggestion; current behavior is correct as far as I can tell.♻️ Possible simplification
- for category in config["irreversible_configuration"]: - number = 0xFFFFFFFF - if "uap" not in category: - for details in config["irreversible_configuration"][category][ - "setting" - ].values(): - if not details["value"]: - number &= ~(1 << details["bit"]) - numbers["i_config"][category] = number - else: - for i in range(4): - for details in config["irreversible_configuration"][category][ - "setting" - ][f"pairing_key_{i}"].values(): - if not details["value"]: - number &= ~(1 << details["bit"]) - numbers["i_config"][category] = number + for category, body in config["irreversible_configuration"].items(): + number = 0xFFFFFFFF + if "uap" not in category: + groups = [body["setting"]] + else: + groups = [body["setting"][f"pairing_key_{i}"] for i in range(4)] + for group in groups: + for details in group.values(): + if not details["value"]: + number &= ~(1 << details["bit"]) + numbers["i_config"][category] = numberThe
reversible_configurationblock can be simplified analogously (withnumber = 0and|=flipping).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tools/generate_tropic_model_config.py` around lines 43 - 83, The get_tropic_configuration function repeats numbers["i_config"][category] = number and numbers["r_config"][category] = number in both branches; refactor by selecting the iterable of "details" up front (for non-uap: config["irreversible_configuration"][category]["setting"].values(), for uap: loop i in range(4) and aggregate values from config["irreversible_configuration"][category]["setting"][f"pairing_key_{i}"].values()), perform the bit operations into the local variable number (starting at 0xFFFFFFFF for irreversible and 0 for reversible), then assign numbers["i_config"][category] or numbers["r_config"][category] = number once after the loop; apply the same simplification to the reversible_configuration block and keep function name get_tropic_configuration, variables number, category, and i to locate where to change.Makefile (1)
190-196: ⚡ Quick winDeclare generation targets as phony to avoid silent skips.
makemay skip these targets if files namedtropic_config,tropic_config_check,gen, orgen_checkexist in the workspace.Suggested patch
+.PHONY: tropic_config tropic_config_check gen gen_check + tropic_config: ./core/tools/generate_tropic_model_config.py ./core/tools/generate_tropic_config_docs.pyAlso applies to: 204-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 190 - 196, Make the Makefile declare the generation targets as phony so make won't skip them when files with the same names exist; add a .PHONY declaration that includes tropic_config and tropic_config_check (and likewise gen and gen_check mentioned in the comment) and ensure the .PHONY line appears before those target definitions so targets like tropic_config, tropic_config_check, gen, and gen_check are always executed.core/embed/sec/tropic/tropic.c (3)
466-468: ⚡ Quick winDuplicate include of
<sec/tropic_configs.h>.This header is already included at the top of the file (line 26). The include guards prevent breakage, but the duplicate placed mid-file (between functions) is misleading and likely a leftover from refactoring. Please drop it.
♻️ Proposed fix
-#include <sec/tropic_configs.h> - static secbool tropic_ensure_i_config(void) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/sec/tropic/tropic.c` around lines 466 - 468, Remove the redundant mid-file include of <sec/tropic_configs.h> (the duplicate `#include` found between function definitions) — this header is already included at the top of tropic.c; delete the extra include so only the top-of-file inclusion remains and no misleading duplicate remains among the function bodies.
560-599: 💤 Low valueRead-size validation and version-bump flow look correct.
The
LT_OK && data_read_size != sizeof(config_version) → secfalsebranch addresses the previously raised concern: an occupied slot with unexpected length now fails closed (RSOD via the caller) rather than silently re-running the repair path. The erase-then-write ofTROPIC_CONFIG_VERSION_SLOTafter both i-/r-config repairs succeed is the right ordering.One minor consistency note: line 593 uses
sizeof(uint8_t)while line 564 usessizeof(config_version). They're equivalent today but tying both tosizeof(config_version)would make a future widening ofconfig_versionautomatically correct.♻️ Optional consistency tweak
config_version = TROPIC_CONFIG_VERSION; ret = lt_r_mem_data_write(&g_tropic_driver.handle, TROPIC_CONFIG_VERSION_SLOT, - &config_version, sizeof(uint8_t)); + &config_version, sizeof(config_version));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/sec/tropic/tropic.c` around lines 560 - 599, Replace the hardcoded sizeof(uint8_t) in the lt_r_mem_data_write call with sizeof(config_version) so both the read and write use the same size expression; update the write call that writes to TROPIC_CONFIG_VERSION_SLOT (the lt_r_mem_data_write invocation after setting config_version = TROPIC_CONFIG_VERSION) to pass sizeof(config_version) to ensure future widening of config_version is handled consistently.
469-505: 💤 Low valueApproval on the i-config recovery direction; small loop-type nit.
The recoverable/irrecoverable bit-mask split (
expected & ~currentvs~expected & current) is the right shape: an expected-1 bit that's already 0 in OTP is genuinely unrecoverable on i-config and correctly aborts, while expected-0 bits that are still 1 are flipped one bit at a time. Returningsecfalseon any retriedlt_i_config_*failure is appropriate.Nit:
int8_t i(and the same pattern intropic_ensure_r_configat line 517) is compared againstsizeof(...)/sizeof(...[0])which has typesize_t. This produces a signed/unsigned comparison and is also fragile if the array ever grows beyond 127.size_t iwould be a more conventional choice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/embed/sec/tropic/tropic.c` around lines 469 - 505, The loop index variable type is signed (int8_t) while it's compared to sizeof(...)/sizeof(...[0]) which is size_t and also limits the max array size; in tropic_ensure_i_config (and similarly in tropic_ensure_r_config) change the outer loop index declaration from int8_t i to size_t i so the signed/unsigned comparison is avoided and the loop can handle arrays >127 entries; update any corresponding casts/usages if necessary.core/tools/generate_tropic_config_docs.py (1)
92-99: ⚡ Quick winHandle missing target file in
--checkmode.If
MD_FILEdoesn't exist (first run, fresh checkout, or it was accidentally deleted),MD_FILE.read_text()raises a bareFileNotFoundErrorinstead of the user-friendly "out of date" message. Treat a missing target the same as a stale one.♻️ Suggested change
if check: with tempfile.NamedTemporaryFile() as f: temp = Path(f.name) json_to_markdown(JSON_FILE, temp) new_content = temp.read_text() - original_content = MD_FILE.read_text() + original_content = MD_FILE.read_text() if MD_FILE.exists() else "" if original_content != new_content: raise click.ClickException(f"{MD_FILE.name} file is out of date.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tools/generate_tropic_config_docs.py` around lines 92 - 99, When running the --check branch the code uses MD_FILE.read_text() which will raise FileNotFoundError if the target doesn't exist; update the check block (the code that creates temp via tempfile.NamedTemporaryFile(), calls json_to_markdown(JSON_FILE, temp), then reads temp.read_text()) to handle a missing MD_FILE by catching FileNotFoundError around MD_FILE.read_text() (or testing MD_FILE.exists()) and treating a missing file as stale (e.g., set original_content to "" or otherwise force the inequality) so the existing raise click.ClickException(f"{MD_FILE.name} file is out of date.") is executed for missing targets.tests/device_tests/test_tropic_boot.py (1)
72-76: 💤 Low valueFree-port acquisition is racy.
Sockets are bound with
port=0to obtain free ports, then closed before the emulator/tropic-model server bind. Between close and the actual bind, another process (or another xdist worker doing the same trick) can grab the same port, causing flaky failures. Consider keeping the sockets bound and passing them down (or usingSO_REUSEPORT/an established lock-file allocator), or accept a wider, deterministic worker-id-based range guarded so the session emulator doesn't already own the port.This is a low-priority polish — the existing pattern is widely used and usually works — but worth keeping in mind if these tests turn flaky in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/device_tests/test_tropic_boot.py` around lines 72 - 76, The current free-port acquisition using temporary sockets s1/s2 and reading s1.getsockname()[1]/s2.getsockname()[1] is racy because sockets are closed before the emulator/tropic server bind; fix by keeping the sockets bound and handing the open socket objects into the code that starts the emulator/tropic-model server (or bind using SO_REUSEPORT if the platform supports it), or alternatively compute ports deterministically from the xdist worker id into a guarded range to avoid collisions; update test_tropic_boot.py to stop closing s1/s2 before server start (or switch to the worker-id-based range approach) so tropic_port and emulator_port remain reserved until bind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/sec/tropic/tropic.c`:
- Around line 545-558: The flag batch_to_fix in tropic_ensure_configuration() is
incorrectly declared static so its value persists across calls; change it to a
normal local variable (e.g., bool batch_to_fix = false;) inside
tropic_ensure_configuration() so it is reinitialized on every invocation,
leaving the rest of the memcmp loop and return logic unchanged.
- Around line 378-411: The arrays cfg_to_check (used at lines where
tropic_configs_irreversible.obj[cfg_to_check[i]] and
TROPIC_CONFIG_ADDRS[cfg_to_check[i]] are indexed) are declared as
lt_config_obj_addr_t but actually hold TR01_CFG_*_IDX index values; change their
type to an integer/index type (e.g., size_t, unsigned, or the dedicated index
enum if available) instead of lt_config_obj_addr_t so they are used as indices,
not addresses, and update any related declarations/usages of cfg_to_check
accordingly.
In `@core/embed/sys/smcall/stm32/smcall_stubs.c`:
- Around line 394-396: The function definition for tropic_ensure_configuration
currently uses an empty parameter list; change its signature to use (void) to
match the rest of the file and the prototype in sec/tropic.h (i.e., update
"secbool tropic_ensure_configuration()" to "secbool
tropic_ensure_configuration(void)") and keep the body calling
smcall_invoke0(SMCALL_TROPIC_ENSURE_CFG) unchanged so the stub and declaration
are consistent.
In `@core/tools/generate_tropic_model_config.py`:
- Around line 37-40: The HEADER string constant contains a typo "direcotry" that
will be written into generated configs; update the HEADER definition (the
f-string assigned to HEADER that references Path(__file__).relative_to(ROOT)) to
correct "direcotry" to "directory" so generated headers read "root directory"
(keep the rest of the string and formatting unchanged).
In `@docs/core/misc/tropic_configs.md`:
- Around line 10-418: Tables in generated Tropic docs violate markdownlint MD058
because table blocks (e.g., the sections under headings like "## CFG_SENSORS
(0x08)", "## CFG_UAP_PAIRING_KEY_WRITE (0x20)", etc.) are not surrounded by
blank lines; update the docs generator to insert a blank line before and after
every rendered table block (ensure when emitting a heading followed by a table
you write a trailing blank line after the heading and a blank line after the
table), and add a unit/regression test that verifies each generated table is
preceded and followed by an empty line to prevent MD058 regressions.
In `@tests/tropic_model/config.yml`:
- Line 3: Fix the typo in the autogenerated header by updating the generator
script core/tools/generate_tropic_model_config.py: find the template or string
literal that emits the comment "# `make tropic_config` in the root direcotry."
and change "direcotry" to "directory", then re-run the generator to regenerate
tests/tropic_model/config.yml; ensure any unit tests or fixtures expecting the
old misspelling are updated if necessary.
In `@tests/tropic_model/tropic_boot_configs/config_bad_unrecoverable.yml`:
- Around line 1-3: Fix the typo in the auto-generated config header by updating
the template in core/tools/generate_tropic_model_config.py: locate the
header/template string (e.g., the variable or function that builds the comment
block used to generate config files like config_bad_unrecoverable.yml) and
change "direcotry" to "directory" so all generated files (config_good.yml,
config_bad_recoverable.yml, config_bad_unrecoverable.yml) emit the corrected
header.
---
Outside diff comments:
In `@core/embed/sys/smcall/stm32/smcall_numbers.h`:
- Around line 96-104: The new SMCALL_TROPIC_ENSURE_CFG ordinal may be missing
from pre-built SECMON binaries; update the repo by regenerating the pre-built
SECMON binary(s) (e.g., secmon.bin) used for PRODUCTION/UNSAFE_FW so they
include the new SMCALL, and also harden the SECMON dispatch by adding a
default/error case in the smcall dispatch switch (the handler that switches on
smcall numbers) to catch unknown ordinals and return a safe error code/log
rather than falling through; reference SMCALL_TROPIC_ENSURE_CFG and the dispatch
switch that handles SMCALL_* values when making these changes.
---
Nitpick comments:
In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c`:
- Around line 205-222: Remove the unnecessary (uint8_t*) casts used as the
second memcmp argument: replace memcmp(&tropic_configs_reversible,
(uint8_t*)&configuration_read, sizeof(...)) and
memcmp(&tropic_configs_irreversible, (uint8_t*)&configuration_read, sizeof(...))
with calls that pass &configuration_read directly
(memcmp(&tropic_configs_reversible, &configuration_read,
sizeof(tropic_configs_reversible)) etc. Also apply the same cleanup in
prodtest_tropic_lock where the identical casts appear; no cast is required
because memcmp accepts const void*.
In `@core/embed/sec/tropic/config/tropic_configs.c.mako`:
- Around line 38-54: Change uap_all_except and uap_bits to accept the
already-selected top-level config dict instead of the full configs map (i.e.,
change signature from (configs, category) to (config, category)), remove the
hard-coded access to "irreversible_configuration" / "reversible_configuration"
inside each function and iterate over the passed-in
config[category]['setting'][f'pairing_key_{key}'] as before; then update the
call sites to pass configs["irreversible_configuration"] or
configs["reversible_configuration"] (respectively) into uap_all_except and
uap_bits so behavior is unchanged while decoupling these helpers from the
top-level JSON layout and keeping their use of all_except and bits intact.
In `@core/embed/sec/tropic/tropic.c`:
- Around line 466-468: Remove the redundant mid-file include of
<sec/tropic_configs.h> (the duplicate `#include` found between function
definitions) — this header is already included at the top of tropic.c; delete
the extra include so only the top-of-file inclusion remains and no misleading
duplicate remains among the function bodies.
- Around line 560-599: Replace the hardcoded sizeof(uint8_t) in the
lt_r_mem_data_write call with sizeof(config_version) so both the read and write
use the same size expression; update the write call that writes to
TROPIC_CONFIG_VERSION_SLOT (the lt_r_mem_data_write invocation after setting
config_version = TROPIC_CONFIG_VERSION) to pass sizeof(config_version) to ensure
future widening of config_version is handled consistently.
- Around line 469-505: The loop index variable type is signed (int8_t) while
it's compared to sizeof(...)/sizeof(...[0]) which is size_t and also limits the
max array size; in tropic_ensure_i_config (and similarly in
tropic_ensure_r_config) change the outer loop index declaration from int8_t i to
size_t i so the signed/unsigned comparison is avoided and the loop can handle
arrays >127 entries; update any corresponding casts/usages if necessary.
In `@core/tools/generate_tropic_config_docs.py`:
- Around line 92-99: When running the --check branch the code uses
MD_FILE.read_text() which will raise FileNotFoundError if the target doesn't
exist; update the check block (the code that creates temp via
tempfile.NamedTemporaryFile(), calls json_to_markdown(JSON_FILE, temp), then
reads temp.read_text()) to handle a missing MD_FILE by catching
FileNotFoundError around MD_FILE.read_text() (or testing MD_FILE.exists()) and
treating a missing file as stale (e.g., set original_content to "" or otherwise
force the inequality) so the existing raise
click.ClickException(f"{MD_FILE.name} file is out of date.") is executed for
missing targets.
In `@core/tools/generate_tropic_model_config.py`:
- Around line 43-83: The get_tropic_configuration function repeats
numbers["i_config"][category] = number and numbers["r_config"][category] =
number in both branches; refactor by selecting the iterable of "details" up
front (for non-uap:
config["irreversible_configuration"][category]["setting"].values(), for uap:
loop i in range(4) and aggregate values from
config["irreversible_configuration"][category]["setting"][f"pairing_key_{i}"].values()),
perform the bit operations into the local variable number (starting at
0xFFFFFFFF for irreversible and 0 for reversible), then assign
numbers["i_config"][category] or numbers["r_config"][category] = number once
after the loop; apply the same simplification to the reversible_configuration
block and keep function name get_tropic_configuration, variables number,
category, and i to locate where to change.
In `@Makefile`:
- Around line 190-196: Make the Makefile declare the generation targets as phony
so make won't skip them when files with the same names exist; add a .PHONY
declaration that includes tropic_config and tropic_config_check (and likewise
gen and gen_check mentioned in the comment) and ensure the .PHONY line appears
before those target definitions so targets like tropic_config,
tropic_config_check, gen, and gen_check are always executed.
In `@tests/device_tests/test_tropic_boot.py`:
- Around line 72-76: The current free-port acquisition using temporary sockets
s1/s2 and reading s1.getsockname()[1]/s2.getsockname()[1] is racy because
sockets are closed before the emulator/tropic server bind; fix by keeping the
sockets bound and handing the open socket objects into the code that starts the
emulator/tropic-model server (or bind using SO_REUSEPORT if the platform
supports it), or alternatively compute ports deterministically from the xdist
worker id into a guarded range to avoid collisions; update test_tropic_boot.py
to stop closing s1/s2 before server start (or switch to the worker-id-based
range approach) so tropic_port and emulator_port remain reserved until bind.
In `@tests/emulators.py`:
- Around line 253-272: When reusing a shared Tropic model the passed-in
tropic_model_port_override gets silently dropped because shared_model.port is
used unconditionally; fix by computing the requested_port (use
tropic_model_port_override or _get_tropic_model_port(worker_id)) before calling
_get_shared_tropic_model, pass it through, then after the call compare
shared_model.port to requested_port and either raise an error or emit a clear
warning if they differ so tests don't unknowingly rely on a different port;
implement this comparison around the existing call to _get_shared_tropic_model
and use the symbols use_shared_tropic_model, _get_shared_tropic_model,
tropic_model_port_override, shared_model.port, and
launch_tropic_model_for_emulator to locate and update the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d3f8f64-aaea-4b00-b659-06c77cdd01d6
📒 Files selected for processing (37)
Makefilecore/embed/projects/firmware/main.ccore/embed/projects/prodtest/cmd/prodtest_tropic.ccore/embed/projects/unix/main.ccore/embed/sec/tropic/config/tropic_configs.ccore/embed/sec/tropic/config/tropic_configs.c.makocore/embed/sec/tropic/config/tropic_configs.jsoncore/embed/sec/tropic/inc/sec/tropic.hcore/embed/sec/tropic/inc/sec/tropic_configs.hcore/embed/sec/tropic/tropic.ccore/embed/sys/smcall/stm32/smcall_dispatch.ccore/embed/sys/smcall/stm32/smcall_numbers.hcore/embed/sys/smcall/stm32/smcall_stubs.ccore/embed/sys/smcall/stm32/smcall_verifiers.ccore/embed/sys/smcall/stm32/smcall_verifiers.hcore/embed/sys/syscall/inc/sys/syscall_numbers.hcore/embed/sys/syscall/stm32/syscall_dispatch.ccore/embed/sys/syscall/stm32/syscall_stubs.ccore/embed/sys/syscall/stm32/syscall_verifiers.ccore/embed/sys/syscall/stm32/syscall_verifiers.hcore/site_scons/models/T3W1/emulator.pycore/site_scons/models/T3W1/trezor_t3w1_revA.pycore/site_scons/models/T3W1/trezor_t3w1_revB.pycore/site_scons/models/T3W1/trezor_t3w1_revC.pycore/tools/generate_tropic_config_docs.pycore/tools/generate_tropic_model_config.pydocs/SUMMARY.mddocs/core/misc/index.mddocs/core/misc/tropic_configs.mdstorage/storage.ctests/device_tests/test_tropic_boot.pytests/emulators.pytests/tropic_model/config.ymltests/tropic_model/config_old.ymltests/tropic_model/tropic_boot_configs/config_bad_recoverable.ymltests/tropic_model/tropic_boot_configs/config_bad_unrecoverable.ymltests/tropic_model/tropic_boot_configs/config_good.yml
| // clang-format off | ||
| // Address table for all 27 I/R-config objects, ordered to match lt_config_t.obj[]. | ||
| // Using a local const table (flash/.rodata) instead of cfg_desc_table from | ||
| // libtropic which is a non-const global (RAM/.data). | ||
| static const enum lt_config_obj_addr_t TROPIC_CONFIG_ADDRS[LT_CONFIG_OBJ_CNT] = { | ||
| TR01_CFG_START_UP_ADDR, | ||
| TR01_CFG_SENSORS_ADDR, | ||
| TR01_CFG_DEBUG_ADDR, | ||
| TR01_CFG_GPO_ADDR, | ||
| TR01_CFG_SLEEP_MODE_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_WRITE_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_READ_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_INVALIDATE_ADDR, | ||
| TR01_CFG_UAP_R_CONFIG_WRITE_ERASE_ADDR, | ||
| TR01_CFG_UAP_R_CONFIG_READ_ADDR, | ||
| TR01_CFG_UAP_I_CONFIG_WRITE_ADDR, | ||
| TR01_CFG_UAP_I_CONFIG_READ_ADDR, | ||
| TR01_CFG_UAP_PING_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_WRITE_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_READ_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_ERASE_ADDR, | ||
| TR01_CFG_UAP_RANDOM_VALUE_GET_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_GENERATE_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_STORE_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_READ_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_ERASE_ADDR, | ||
| TR01_CFG_UAP_ECDSA_SIGN_ADDR, | ||
| TR01_CFG_UAP_EDDSA_SIGN_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_INIT_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_GET_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_UPDATE_ADDR, | ||
| TR01_CFG_UAP_MAC_AND_DESTROY_ADDR, | ||
| }; | ||
| // clang-format on |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the libtropic definition of lt_config_obj_addr_t and confirm whether
# it carries an enum tag named `lt_config_obj_addr_t`.
fd -t f -e h -e c | xargs rg -nP -C2 'lt_config_obj_addr_t' 2>/dev/null | head -200
# Show the typedef form specifically.
rg -nP -U '(?s)typedef\s+enum[^{]*\{[^}]*\}\s*lt_config_obj_addr_t\s*;' -g '*.h' -g '*.c'
rg -nP 'enum\s+lt_config_obj_addr_t\b' -g '*.h' -g '*.c'
# Confirm whether TR01_CFG_*_IDX symbols are members of lt_config_obj_addr_t
# (i.e. share the enum's underlying type).
rg -nP 'TR01_CFG_[A-Z_]+_IDX\b' -g '*.h' -C2 | head -80Repository: trezor/trezor-firmware
Length of output: 1243
🏁 Script executed:
# Find where lt_config_obj_addr_t is defined (likely in header files)
rg -nP 'typedef\s+(enum\s+)?lt_config_obj_addr_t' -g '*.h' -g '*.c' -A5
# Also search for just the definition in headers
fd -t f -e h | xargs rg -nP 'lt_config_obj_addr_t' -A3 -B3 2>/dev/null | head -100
# Look for libtropic includes or headers in the repo
fd -t f -name '*libtropic*' -o -name '*lt_*' 2>/dev/null
# Search for TR01_CFG definitions (especially the *_IDX variants)
rg -nP 'define\s+TR01_CFG_[A-Z_]+_(ADDR|IDX)' -g '*.h' | head -50Repository: trezor/trezor-firmware
Length of output: 48
🏁 Script executed:
# Check what libtropic includes are referenced in tropic.c
head -50 core/embed/sec/tropic/tropic.c | grep -E '#include|#define'
# Search for all enum definitions in the file
rg -nP 'enum\s+\w+\s*\{' core/embed/sec/tropic/tropic.c -A20
# Look for TR01_CFG definitions anywhere in the codebase
rg -n 'TR01_CFG_' core/embed/sec/tropic/tropic.c | head -30
# Check for libtropic library in vendor or external
find . -name '*libtropic*' -o -name '*lt_config*' 2>/dev/null | head -20
# Look in the entire file for where these values come from
rg -n 'TR01_CFG_START_UP' core/embed/sec/tropic/ -B5 -A5Repository: trezor/trezor-firmware
Length of output: 3375
🏁 Script executed:
# Look for libtropic headers and the typedef
find ./core/vendor/libtropic ./vendor/libtropic -name '*.h' 2>/dev/null | head -20
# Search for lt_config_obj_addr_t definition in libtropic
rg -n 'lt_config_obj_addr_t' ./core/vendor/libtropic ./vendor/libtropic 2>/dev/null | head -50
# Search for TR01_CFG definitions in libtropic
rg -n 'TR01_CFG_' ./core/vendor/libtropic ./vendor/libtropic 2>/dev/null | head -100
# Look for the typedef enum form specifically
rg -n 'typedef.*enum.*lt_config_obj_addr_t' ./core/vendor/libtropic ./vendor/libtropic 2>/dev/nullRepository: trezor/trezor-firmware
Length of output: 48
🏁 Script executed:
# Check project configuration files
find . -maxdepth 3 -name 'CMakeLists.txt' -o -name 'setup.py' -o -name 'package.json' 2>/dev/null | head -10
# Look for any documentation or comments about libtropic
rg -n 'libtropic' . -g '*.md' -g '*.txt' -g '*.cmake' 2>/dev/null | head -30
# Check if there's a system installation path or include hints
grep -r 'libtropic' . --include='*.c' --include='*.h' -l 2>/dev/null | head -10Repository: trezor/trezor-firmware
Length of output: 1126
🏁 Script executed:
# Check tropic_configs.h for TR01_CFG definitions
cat core/embed/sec/tropic/inc/sec/tropic_configs.h
# Check if TR01_CFG symbols are defined there
rg -n 'TR01_CFG_' core/embed/sec/tropic/inc/sec/tropic_configs.h
# Also check tropic.h
rg -n 'lt_config_obj_addr_t' core/embed/sec/tropic/inc/sec/tropic.h -B3 -A3
# Check the config file
rg -n 'TR01_CFG_' core/embed/sec/tropic/config/tropic_configs.c | head -50Repository: trezor/trezor-firmware
Length of output: 1016
🌐 Web query:
libtropic 3.0.0 lt_config_obj_addr_t typedef enum documentation
💡 Result:
No documentation or source code explicitly defining or documenting 'lt_config_obj_addr_t' as a typedef enum in libtropic 3.0.0 was found across multiple searches targeting the official GitHub repository (tropicsquare/libtropic) [1][2][3], release notes [2], and general web results. Libtropic is the C SDK for TROPIC01 secure element by Tropic Square, with releases up to v3.1.0 noted, but no matching type appears in available snippets or docs. Unrelated results from LibreTiny (lt_config.h macros) [4][5] surfaced due to similar naming, but do not match. The type may be internal/private, from a specific platform HAL/example, or not publicly indexed. Check the full libtropic source code directly at https://github.com/tropicsquare/libtropic for version 3.0.0 headers like lt_config.h or similar.
Citations:
- 1: https://github.com/tropicsquare/libtropic
- 2: https://newreleases.io/project/github/tropicsquare/libtropic/release/v3.1.0
- 3: https://github.com/tropicsquare/libtropic/tree/master
- 4: https://docs.libretiny.eu/ltapi/lt__config_8h
- 5: https://github.com/libretiny-eu/libretiny/blob/c6b06d4b/cores/common/base/lt_config.h
🏁 Script executed:
# Search for all uses of enum lt_config_obj_addr_t to see if it's a pattern
rg -n 'enum lt_config_obj_addr_t' .
# Search for typedef or enum definitions that might hint at the structure
rg -n 'lt_config_obj_addr_t\|TR01_CFG_.*_IDX\|TR01_CFG_.*_ADDR' core/embed/sec/tropic/ --type c --type h -C1 | head -150
# Check compilation flags or any type-related warnings
find . -name '*tropic*' -type f \( -name '*.mk' -o -name 'CMakeLists.txt' -o -name 'compile_flags.txt' \) 2>/dev/null
# Look for any struct or typedef definitions that include lt_config_obj_addr_t
rg -n 'lt_config_obj_addr_t' core/embed/sec/tropic/ -B2 -A2 --type c --type hRepository: trezor/trezor-firmware
Length of output: 1245
🏁 Script executed:
# Find the functions where cfg_to_check is used and see how it's accessed
rg -n 'cfg_to_check' core/embed/sec/tropic/tropic.c -B5 -A20
# Check if there are any loops or indexing operations on these arrays
rg -n 'cfg_to_check\[' core/embed/sec/tropic/tropic.c -B2 -A2
# Look for LT_CONFIG_OBJ_CNT and related definitions
rg -n 'LT_CONFIG_OBJ_CNT' core/embed/sec/tropic/tropic.c -B2 -A2Repository: trezor/trezor-firmware
Length of output: 4371
Use an appropriate index type for cfg_to_check arrays instead of lt_config_obj_addr_t.
The cfg_to_check[] arrays at lines 472 and 510 are declared with type lt_config_obj_addr_t but contain TR01_CFG_*_IDX index values. These arrays are then used strictly as indices (e.g., tropic_configs_irreversible.obj[cfg_to_check[i]] and TROPIC_CONFIG_ADDRS[cfg_to_check[i]]). The address type should only hold address values (*_ADDR), not indices. Consider switching to an appropriate integer or index type for the cfg_to_check arrays.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/embed/sec/tropic/tropic.c` around lines 378 - 411, The arrays
cfg_to_check (used at lines where
tropic_configs_irreversible.obj[cfg_to_check[i]] and
TROPIC_CONFIG_ADDRS[cfg_to_check[i]] are indexed) are declared as
lt_config_obj_addr_t but actually hold TR01_CFG_*_IDX index values; change their
type to an integer/index type (e.g., size_t, unsigned, or the dedicated index
enum if available) instead of lt_config_obj_addr_t so they are used as indices,
not addresses, and update any related declarations/usages of cfg_to_check
accordingly.
| // Address table for all 27 I/R-config objects, ordered to match lt_config_t.obj[]. | ||
| // Using a local const table (flash/.rodata) instead of cfg_desc_table from | ||
| // libtropic which is a non-const global (RAM/.data). | ||
| static const enum lt_config_obj_addr_t TROPIC_CONFIG_ADDRS[LT_CONFIG_OBJ_CNT] = { | ||
| TR01_CFG_START_UP_ADDR, | ||
| TR01_CFG_SENSORS_ADDR, | ||
| TR01_CFG_DEBUG_ADDR, | ||
| TR01_CFG_GPO_ADDR, | ||
| TR01_CFG_SLEEP_MODE_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_WRITE_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_READ_ADDR, | ||
| TR01_CFG_UAP_PAIRING_KEY_INVALIDATE_ADDR, | ||
| TR01_CFG_UAP_R_CONFIG_WRITE_ERASE_ADDR, | ||
| TR01_CFG_UAP_R_CONFIG_READ_ADDR, | ||
| TR01_CFG_UAP_I_CONFIG_WRITE_ADDR, | ||
| TR01_CFG_UAP_I_CONFIG_READ_ADDR, | ||
| TR01_CFG_UAP_PING_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_WRITE_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_READ_ADDR, | ||
| TR01_CFG_UAP_R_MEM_DATA_ERASE_ADDR, | ||
| TR01_CFG_UAP_RANDOM_VALUE_GET_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_GENERATE_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_STORE_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_READ_ADDR, | ||
| TR01_CFG_UAP_ECC_KEY_ERASE_ADDR, | ||
| TR01_CFG_UAP_ECDSA_SIGN_ADDR, | ||
| TR01_CFG_UAP_EDDSA_SIGN_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_INIT_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_GET_ADDR, | ||
| TR01_CFG_UAP_MCOUNTER_UPDATE_ADDR, | ||
| TR01_CFG_UAP_MAC_AND_DESTROY_ADDR, | ||
| }; |
There was a problem hiding this comment.
I don’t like duplicating this table because you have to keep it synchronized with the upstream. Is the purpose to save space in RAM? I would rather ask the tropic team to
- split it into two parallel tables, one for descriptions and one for addresses and
- make it
static const.
There was a problem hiding this comment.
Yes, it is because of RAM. The table was currently not included in the build. When my functions started using it, the RAM overflew its limit and firmware would not build.
I will ask the Tropic team to make it static const
There was a problem hiding this comment.
Okay, let’s keep it as it is until Tropic makes it static const. Just move it to the beginning of the file, where the other static variables are defined, and add a comment that we can remove it once it’s fixed.
There was a problem hiding this comment.
And please add a comment saying that we can remove it once it’s fixed or a link to this conversation.
| tropic_driver_t *drv = &g_tropic_driver; | ||
|
|
||
| lt_config_obj_addr_t cfg_to_check[] = { | ||
| TR01_CFG_SENSORS_IDX, |
There was a problem hiding this comment.
Why is not TR01_CFG_START_UP_IDX here?
There was a problem hiding this comment.
The start up cfg in I-config did not change. The change was only in R-config
| #ifdef USE_TROPIC | ||
| ensure(tropic_ensure_configuration(), "Tropic configuration check failed"); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
To be honest, I’m not sure whether this belongs here or in secmon’s main, where tropic_init() is. I’m actually surprised it compiles, because I thought tropic_ensure_configuration() wasn’t accessible from firmware.
There was a problem hiding this comment.
I’m actually surprised it compiles, because I thought tropic_ensure_configuration() wasn’t accessible from firmware.
I understand it now. You created a syscall and smcall for it.
| #ifdef USE_TROPIC | ||
| ensure(tropic_ensure_configuration(), "Tropic configuration check failed"); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
To be honest, I’m not sure whether this belongs here or in main_main.c where tropic_init() is.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/sec/tropic/tropic.c`:
- Around line 490-497: The loop that flips bits (using to_flip, BIT(j),
TROPIC_RETRY_COMMAND and lt_i_config_write on
TROPIC_CONFIG_ADDRS[cfg_to_check[i]] with drv->handle) must not return secfalse
immediately on a write transport error; instead, after a failed
lt_i_config_write call perform a read-back of that same I-config bit (use the
corresponding lt_i_config_read or the existing R-config read path) and verify
the bit actually flipped by comparing against expected/current; only return
secfalse if the read-back confirms the bit remains unset (i.e., not flipped) or
the read itself fails definitively. Ensure the logic mirrors the R-config
handling: attempt a read-retry if transient, and only treat as irrecoverable if
read shows the bit is still incorrect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9187bc2-6ca2-4611-beeb-b7b87ddf0968
📒 Files selected for processing (1)
core/embed/sec/tropic/tropic.c
2 participants
During firmware boot checks the tropic configuration against the expected values. If there is a mismatch, tries to set it correctly. If the set cannot be done, throws RSOD.