fix bootloader_ci#6996
Open
TychoVrahe wants to merge 3 commits into
Open
Conversation
[no changelog]
|
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 a public secret_reset() API and implements it for STM32F4, STM32U5, and Unix. The bootloader conditionally includes the secret header, calls secret_reset() after TrustZone init, optionally verifies a Secmon header to compute a code offset, calls secret_prepare_fw(...) when enabled, computes the vector-table address including the Secmon offset, and jumps to the next stage using startup_args_export() instead of NULL. Sequence DiagramsequenceDiagram
participant Bootloader as Bootloader/main.c
participant Secret as Secret/subsystem
participant Secmon as Secmon/header
participant NextStage as Next Stage
Bootloader->>Secret: secret_reset() after TrustZone init
activate Secret
Secret-->>Bootloader: (reset complete)
deactivate Secret
Bootloader->>Secmon: Read and verify Secmon header
activate Secmon
Secmon->>Secmon: Validate model, signature, monotonic protection, contents
Secmon-->>Bootloader: secmon_code_offset
deactivate Secmon
Bootloader->>Secret: secret_prepare_fw(startup_args_export())
activate Secret
Secret-->>Bootloader: (prepared)
deactivate Secret
Bootloader->>Bootloader: compute vectbl_addr (include secmon_code_offset)
Bootloader->>NextStage: jump_to_next_stage(vectbl_addr, startup_args_export())
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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 26499961321
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/embed/sec/secret/inc/sec/secret.h (1)
85-90: ⚡ Quick winNarrow the
secret_reset()contract.This comment currently promises that the API clears “all secrets and keys,” but none of the backends in this PR actually erase persistent secret storage; STM32U5 only drops cached state / reboots, and STM32F4/Unix are no-ops. Tighten the docstring to the real runtime-reset semantics so callers do not rely on guarantees this API does not provide.
🤖 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/secret/inc/sec/secret.h` around lines 85 - 90, Update the docstring for secret_reset() to narrow its contract to in-memory/runtime state only: state that the function clears cached secrets and prepares the runtime for a new firmware run but does not erase or zero persistent/flash-backed secret storage; mention that current backends implement this as dropping cached state/reboot on STM32U5 and as a no-op on STM32F4 and Unix so callers must not rely on persistent erasure. Ensure the description replaces the existing “clears all secrets and keys” language and refers to the secret_reset() API name so reviewers can locate it.
🤖 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/projects/bootloader_ci/main.c`:
- Around line 283-285: The call to secret_prepare_fw(sectrue, sectrue) must be
moved so it runs after all Secmon validation checks (the check_secmon_* ensure()
calls) and immediately before jump_to_next_stage(), not before them; update
main.c by removing the current early invocation of secret_prepare_fw and
inserting it just before jump_to_next_stage() so that any failed ensure() paths
can call secret_reset() on the next boot without consuming the BHK, ensuring
secret_prepare_fw only locks secrets after image verification succeeds (also
adjust the nearby block covering the region corresponding to the original
289-312 area).
---
Nitpick comments:
In `@core/embed/sec/secret/inc/sec/secret.h`:
- Around line 85-90: Update the docstring for secret_reset() to narrow its
contract to in-memory/runtime state only: state that the function clears cached
secrets and prepares the runtime for a new firmware run but does not erase or
zero persistent/flash-backed secret storage; mention that current backends
implement this as dropping cached state/reboot on STM32U5 and as a no-op on
STM32F4 and Unix so callers must not rely on persistent erasure. Ensure the
description replaces the existing “clears all secrets and keys” language and
refers to the secret_reset() API name so reviewers can locate it.
🪄 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: ca2ea943-dfb6-40d1-ad27-91a9e1abdad9
📒 Files selected for processing (5)
core/embed/projects/bootloader_ci/main.ccore/embed/sec/secret/inc/sec/secret.hcore/embed/sec/secret/stm32f4/secret.ccore/embed/sec/secret/stm32u5/secret.ccore/embed/sec/secret/unix/secret.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes CI Bootloader, which was unable to function properly on U5 models: