feat(core): check tropic configuration on change_pin#6392
feat(core): check tropic configuration on change_pin#6392
change_pin#6392Conversation
When changing PIN and Tropic is available check that the Tropic configurations are set correctly - according to the expected configurations. Move Tropic configurations specification from `prodtest_tropic.c` to `sec/tropic/` so that `storage` can access it. Add a `tropic` function in `core` to check whether the set configurations are aligned with the expecetd configurations. If the configurations are not aligned with the expectations, make one try to rewrite them to the expected values. Before changing PIN in `storage_change_pin()`, check the Tropic configurations. [no changelog]
|
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:
WalkthroughThis change introduces a Tropic sensor validation feature by externalizing configuration management from compile-time static definitions to generated configuration data. It adds two new public API functions to validate Tropic sensors and track validation timing, integrates these into the PIN derivation flow in storage.c, and updates build systems to compile the new configuration source. Additionally, it adds code generation scripts for configuration and documentation, expands test configuration data, and creates auto-generated documentation for Tropic configurations. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 22140292861
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/embed/sec/tropic/tropic.c`:
- Around line 1000-1014: The R-config write block should erase the R region
before writing to avoid write failures; update the block that compares r_config
vs g_reversible_configuration to call lt_r_config_erase(tropic_handle)
immediately before lt_write_whole_R_config(tropic_handle,
&g_reversible_configuration), check its return value for LT_OK, then proceed
with lt_write_whole_R_config and the subsequent lt_read_whole_R_config/memcmp
verification using the existing variables r_config, g_reversible_configuration
and tropic_handle.
- Around line 989-993: In tropic_validate_configs add a Tropic session around
the read/write ops: after obtaining tropic_get_handle() call
tropic_session_start() and verify it succeeded before calling
lt_read_whole_R_config / lt_write_whole_I_config, and ensure you call
tropic_session_stop() (or clean up) before returning; update error paths to stop
the session when appropriate so the function mirrors the session handling used
by tropic_pin_stretch and tropic_pin_set.
🧹 Nitpick comments (3)
core/embed/sec/tropic/inc/sec/tropic_configs.h (2)
27-232: Consider usingextern constdeclaration to avoid code duplication.Declaring
static constin a header file means each translation unit that includes this header will have its own copy of the configuration data. Since this file is included by bothprodtest_tropic.candtropic.c, these large structures (~100+ uint32_t values each) will be duplicated in the binary.Consider using
extern constin the header with the definition in a single.cfile to reduce binary size:// In tropic_configs.h extern const struct lt_config_t g_irreversible_configuration; extern const struct lt_config_t g_reversible_configuration; // In tropic_configs.c (new file) const struct lt_config_t g_irreversible_configuration = { ... }; const struct lt_config_t g_reversible_configuration = { ... };This is a recommended refactor for embedded systems where flash space is often limited.
24-25: TODO comment requires follow-up before production deployment.The TODO indicates the configuration needs to match the revision of provisioned Tropic chips. This should be tracked to ensure it's addressed before shipping to production.
Would you like me to open a tracking issue for this TODO?
core/embed/sec/tropic/tropic.c (1)
996-1035: Consider adding retry logic for robustness.Other Tropic operations in this file use
TROPIC_RETRY_COMMAND()macro to handle transient errors likeLT_L1_CHIP_ALARM_MODE,LT_L1_SPI_ERROR, etc. (see lines 73-106). The validation function performs multiple read/write operations that could benefit from the same retry mechanism.♻️ Example usage of retry macro
- ret = lt_read_whole_R_config(tropic_handle, &r_config); + ret = TROPIC_RETRY_COMMAND(lt_read_whole_R_config(tropic_handle, &r_config));Apply similarly to all
lt_read_whole_*andlt_write_whole_*calls in this function.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/tools/generate_tropic_model_config.py`:
- Around line 40-56: Add one more blank line before the top-level function
definition get_tropic_configuration so there are two blank lines separating it
from the previous top-level code (satisfies flake8 E302); locate the def
get_tropic_configuration(...) block and insert a single additional newline above
it to produce the required spacing.
- Line 17: CFG_SPEC_DIR is set to a non-existent path and should point to the
actual config file; update the CFG_SPEC_DIR assignment (the constant defined
alongside ROOT in generate_tropic_model_config.py) to reference
"core/embed/sec/tropic/tropic_configs.json" instead of the bogus nested
"inc/sec/tropic_configs_manual.json" so the default --config-path uses the real
file; keep using the ROOT / ... Path construction and only change the final path
components to match the correct filename and directory.
🧹 Nitpick comments (4)
core/embed/sec/tropic/tropic_configs.c.mako (2)
38-43: Fix typo inbitshelper parameter name.
Small readability improvement and avoids propagating the misspelling.✏️ Suggested rename
-def bits(selceted_bits: list[int], counter: Counter) -> str: +def bits(selected_bits: list[int], counter: Counter) -> str: counter.r += 1 - if len(selceted_bits) == 0: + if len(selected_bits) == 0: return "0," else: - return " | ".join(f"BIT({b})" for b in selceted_bits) + "," + return " | ".join(f"BIT({b})" for b in selected_bits) + ","
56-58: Track the revision-specific Tropic config TODO.
Consider linking this TODO to a tracked issue or wiring revision selection so it doesn’t ship unresolved.If you want, I can draft the issue or help sketch the revision switch.
Also applies to: 266-268
core/embed/sec/tropic/tropic_configs.json (1)
2-439: Make config ordering explicit to avoid accidental re-mapping.
The generator currently relies on JSON key insertion order to map config words; any reformatting that reorders keys could silently change register assignments. Consider adding explicit order arrays or offsets and validating them in the generator.Please confirm the generator’s ordering assumptions and that no tooling reorders these keys.
core/tools/generate_tropic_model_config.py (1)
44-55: Consider using explicit validation instead of assertions.Assertions (lines 45, 51) are stripped when Python runs with optimization (
-Oflag). While this is a developer tool, explicit validation would provide clearer error messages if the JSON structure is malformed.♻️ Suggested improvement
for key in config['irreversible_configuration']: - assert list(config['irreversible_configuration'][key].keys()) == ['all_except'], config['irreversible_configuration'][key].keys() + if list(config['irreversible_configuration'][key].keys()) != ['all_except']: + raise ValueError(f"Expected 'all_except' key for irreversible config '{key}', got: {config['irreversible_configuration'][key].keys()}") number = 0xFFFFFFFF for exclude in config['irreversible_configuration'][key]['all_except']: number &= ~exclude numbers['i_config'][key] = number for key in config['reversible_configuration']: - assert list(config['reversible_configuration'][key].keys()) == ['bits'], config['reversible_configuration'][key].keys() + if list(config['reversible_configuration'][key].keys()) != ['bits']: + raise ValueError(f"Expected 'bits' key for reversible config '{key}', got: {config['reversible_configuration'][key].keys()}") number = 0 for include in config['reversible_configuration'][key]['bits']: number |= include
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/tools/generate_tropic_model_config.py`:
- Around line 1-4: The file generate_tropic_model_config.py is failing CI
formatting; re-run the project's formatter on this module (apply the repo's
standard formatting tools such as the configured black/isort/pre-commit hooks)
so the shebang and import block (hashlib, json) follow the repo style, then
commit the reformatted file; ensure the file passes the formatter CI before
merging.
core/embed/sec/tropic/tropic.c
Outdated
| lt_r_mem_data_read_time(time_ms); | ||
| } | ||
|
|
||
| bool tropic_validate_sensors(void) { |
There was a problem hiding this comment.
I think this function should return secbool, because the result should be protected against glitching attacks.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/embed/sec/tropic/tropic_configs.yaml`:
- Around line 266-315: The YAML defines duplicate keys named
cfg_uap_mac_and_destroy which causes parse/lint errors; remove the redundant
entries and keep a single cfg_uap_mac_and_destroy mapping (or convert them into
a list under a single key if multiple variants are intended), ensuring the
remaining block preserves the intended comment/docstring and the {'all_except':
[0,1,8,9,16,24]} value referenced by cfg_uap_mac_and_destroy.
In `@core/tools/generate_tropic_model_config.py`:
- Around line 45-55: The assertion messages in the validation of
config["irreversible_configuration"] and config["reversible_configuration"]
contain typos ("i_congig" and "r_congig"); update the f-strings in those two
assert statements to the correct identifiers ("i_config" and "r_config") so
error output accurately reflects the schema being validated (locate the asserts
that reference config["irreversible_configuration"][key].keys() and
config["reversible_configuration"][key].keys() and correct the typo in the
f-string message).
🧹 Nitpick comments (1)
core/embed/sec/tropic/tropic_configs.c.mako (1)
38-43: Renameselceted_bitsparameter for clarity.This typo is minor but makes the template harder to read.
♻️ Proposed fix
-def bits(selceted_bits: list[int], counter: Counter) -> str: +def bits(selected_bits: list[int], counter: Counter) -> str: counter.r += 1 - if len(selceted_bits) == 0: + if len(selected_bits) == 0: return "0," else: - return " | ".join(f"BIT({b})" for b in selceted_bits) + "," + return " | ".join(f"BIT({b})" for b in selected_bits) + ","
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/embed/sec/tropic/tropic.c`:
- Line 1015: Fix the typo in the comment string "In development builds the
Tropic configuration might not bet set." by changing "bet" to "be" so it reads
"In development builds the Tropic configuration might not be set."; locate that
comment in tropic.c (the line containing the exact incorrect sentence) and
update the text accordingly, keeping the comment punctuation and formatting
unchanged.
- Around line 1025-1037: Wrap the two configuration reads with the existing
retry macro to avoid transient SPI/CRC failures causing spurious validation
failures: replace the direct calls to lt_r_config_read(tropic_handle,
TR01_CFG_SENSORS_ADDR, &sensors_config) and lt_i_config_read(tropic_handle,
TR01_CFG_SENSORS_ADDR, &sensors_config) by invoking TROPIC_RETRY_COMMAND(...)
around each call (same pattern used in lt_erase_and_write_R_config), preserving
the existing error handling (goto cleanup) when the retry macro reports failure;
ensure you reference the same sensors_config variable and TR01_CFG_SENSORS_ADDR
constant.
🧹 Nitpick comments (1)
core/embed/sec/tropic/tropic.c (1)
1047-1047: Cleanup label indentation inconsistency.The
cleanup:label here has leading spaces, while all othercleanup:labels in this file (e.g., Lines 753, 797, 883) are at column 0.✏️ Fix
- cleanup: +cleanup:
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/embed/sec/tropic/tropic.c`:
- Around line 998-1001: tropic_validate_sensors_time() currently only sums
lt_r_config_read_time() and lt_i_config_read_time() but omits the potential cost
of tropic_session_start() invoked by tropic_validate_sensors(); update
tropic_validate_sensors_time() to include the session-start cost by
conditionally adding the value from tropic_session_start_time() (or checking the
same session-active flag used by tropic_session_start and adding ~210ms if not
active) before adding lt_r_config_read_time() and lt_i_config_read_time(),
mirroring the pattern used by tropic_session_start_time() and other *_time
helpers.
In `@docs/core/misc/tropic_configs.md`:
- Around line 175-178: Irreversible config keys use EDDSA_ECCKEY_SLOT_* (e.g.,
EDDSA_ECCKEY_SLOT_0_7) while reversible ones use EDDSA_ECCKEY_* (e.g.,
EDDSA_ECCKEY_0_7); update the source JSON that defines these configs so both
configurations use the consistent key naming with the _SLOT suffix (change
eddsa_ecckey_* to eddsa_ecckey_slot_* for the reversible entries) and
regenerate/update the docs/tables to reflect EDDSA_ECCKEY_SLOT_0_7,
EDDSA_ECCKEY_SLOT_8_15, etc., everywhere.
🧹 Nitpick comments (6)
core/tools/generate_tropic_model_config.py (1)
43-90: Logic looks correct but consider a minor clarity improvement.The
get_tropic_configurationfunction correctly computes bitmasks for both irreversible (clear bits from0xFFFFFFFF) and reversible (set bits from0) configurations, with special "uap" handling that aggregates across all 4 pairing keys.One minor observation: the
number = 0xFFFFFFFFassignment on Line 48 is dead code whencategory == "uap", since it's immediately overridden at Line 60. Not a bug, but slightly misleading.♻️ Optional: move initial assignment into the non-uap branch
for category in config["irreversible_configuration"]: - number = 0xFFFFFFFF if category != "uap": + number = 0xFFFFFFFF for details in config["irreversible_configuration"][category][core/embed/sec/tropic/tropic.c (1)
1013-1013: Nit: extra blank line.There's a redundant blank line between the session start check and the handle retrieval.
Proposed fix
if (!tropic_session_start()) { goto cleanup; } - lt_handle_t *tropic_handle = tropic_get_handle();core/tools/generate_tropic_config_docs.py (2)
19-28:print_value_linesassumes all rows have equal column counts.If any row has fewer elements than
col_count, Line 21 will raise anIndexError. This is safe given current callers always produce uniform-width rows, but a defensive check or padding would make the helper more robust.
68-80: The--checkmode overwrites the file before comparing.If the process is interrupted between Line 74 (regeneration) and Line 79 (restore), the original file content is lost. Consider comparing in-memory instead:
♻️ Proposed: use a temporary file or StringIO for comparison
+import tempfile + `@click.command`() `@click.option`("--check", is_flag=True) def generate_tropic_config_docs(check: bool) -> None: if check: - original_content = MD_FILE.read_text() - - json_to_markdown(JSON_FILE, MD_FILE) - - if check: - new_content = MD_FILE.read_text() - if original_content != new_content: - MD_FILE.write_text(original_content) + with tempfile.NamedTemporaryFile(mode='w', suffix='.md', delete=False) as tmp: + tmp_path = Path(tmp.name) + json_to_markdown(JSON_FILE, tmp_path) + new_content = tmp_path.read_text() + tmp_path.unlink() + if not MD_FILE.exists() or MD_FILE.read_text() != new_content: raise click.ClickException(f"{MD_FILE.name} file is out of date.") + else: + json_to_markdown(JSON_FILE, MD_FILE)core/embed/sec/tropic/config/tropic_configs.c.mako (1)
62-73: Fragile assumption:uapmust be the last key in the JSON.
i_names[:-1](Line 68) andr_names[:-1](Line 79) skip the last key, assuming it is"uap". If the JSON key order ever changes (e.g., a new category is added after"uap", or the dict is reordered), this template would silently produce incorrect output — either processing"uap"as a normal category or skipping a non-uap category.Consider filtering explicitly:
♻️ Proposed: use explicit key filtering instead of index slicing
-% for category in i_names[:-1]: +% for category in [n for n in i_names if n != "uap"]: ${all_except([c['bit'] for c in irreversible_config[category]["setting"].values() if not(c['value'])])} % endforApply the same change for
r_names[:-1]on Line 79.docs/core/misc/tropic_configs.md (1)
1-417: Fix markdown formatting in the documentation generator.The static analysis tool reports 54 instances of missing blank lines around tables (MD058). Since this file is auto-generated, the fix should be applied to
core/tools/generate_tropic_config_docs.py.The generator script should add blank lines before and after each Markdown table. Would you like me to help update the generation script to fix these formatting issues?
There was a problem hiding this comment.
This file should also have a comment that it is auto-generated.
3 participants
When changing PIN and Tropic is available check that the Tropic configurations are set correctly - according to the expected configurations.
Move Tropic configurations specification from
prodtest_tropic.ctosec/tropic/so thatstoragecan access it.Add a
tropicfunction incoreto check whether the set configurations are aligned with the expecetd configurations. If the configurations are not aligned with the expectations, make one try to rewrite them to the expected values.Before changing PIN in
storage_change_pin(), check the Tropic configurations.