refactor(core): remove quadword usage from secret implementation
What the developer wrote
Authored by tychovrahe
refactor(core): remove quadword usage from secret implementation
[no changelog]
What changed, and why it matters
This commit refactors how Trezor's STM32U5 hardware wallet writes sensitive secrets (such as the Board Hardware Key) to protected flash memory. It replaces a fixed 16-byte 'quadword' write loop with a generic data-write helper. The change is described by the developer as a non-functional cleanup, but it removes an alignment/length assumption that previously guaranteed writes happened in exactly 16-byte chunks. If the new helper behaves differently under edge cases, secret storage could be misaligned, partially written, or handled with different atomicity. There is no direct evidence of an exploitable bug in the diff itself.
Review the implementation of `flash_area_write_data()` and compare it to the removed `flash_area_write_quadword()` to confirm that alignment, length, write granularity, and error semantics are preserved for secret writes. If the new helper allows unaligned or partial writes, add explicit preconditions or assertions in `secret_write()` and `secret_bhk_regenerate()` to enforce 16-byte/32-byte alignment and length. Consider adding tests or a changelog entry documenting the behavioral contract for secret storage writes.
Security signals we found
Removal of fixed 16-byte (quadword) write granularity for secret storage
Replacement of explicit per-quadword error handling with a single helper call
Change in BHK regeneration from two 16-byte RNG quadwords to one 32-byte write
No changelog entry and commit framed as refactor, reducing visibility of security implications
Insufficient context in diff to verify alignment/atomicity guarantees of new helper
Evidence from the diff
The patch changes core/embed/sec/secret/stm32u5/secret.c. secret_write() previously looped over len / 16 iterations calling flash_area_write_quadword() with 16-byte offsets. It now calls flash_area_write_data() once with the full offset, data, and len. Similarly, secret_bhk_regenerate() previously wrote two separate 16-byte quadwords (each filled from rng_get()) and now fills one 32-byte buffer and writes it via flash_area_write_data(). The commit message labels this a ‘refactor’ with ‘[no changelog]’. No security relevance, CVE, or researcher attribution is stated in the commit or supplied references. The security question is whether flash_area_write_data() preserves the same alignment, length, and atomicity guarantees that the quadword API enforced. Without the implementation of flash_area_write_data() and the removed flash_area_write_quadword(), we cannot confirm functional equivalence. The change is therefore a potential hardening/assumption shift rather than a demonstrated vulnerability.
Changed components
core/embed/sec/secret/stm32u5/secret.csecret_write()secret_bhk_regenerate()Board Hardware Key (BHK) generation and storageSTM32U5 secret flash area write pathInspect captured patch +8 / −18
diff --git a/core/embed/sec/secret/stm32u5/secret.c b/core/embed/sec/secret/stm32u5/secret.c
index 80e3b591..ebba4ff3 100644
--- a/core/embed/sec/secret/stm32u5/secret.c
+++ b/core/embed/sec/secret/stm32u5/secret.c
@@ -144,17 +144,10 @@ static secbool secret_ensure_initialized(void) {
secbool secret_write(const uint8_t *data, uint32_t offset, uint32_t len) {
mpu_mode_t mpu_mode = mpu_reconfig(MPU_MODE_SECRET);
ensure(flash_unlock_write(), "secret write");
- for (int i = 0; i < len / 16; i++) {
- if (sectrue != flash_area_write_quadword(&SECRET_AREA, offset + (i * 16),
- (uint32_t *)&data[(i * 16)])) {
- ensure(flash_lock_write(), "secret write");
- mpu_restore(mpu_mode);
- return secfalse;
- }
- }
+ secbool result = flash_area_write_data(&SECRET_AREA, offset, data, len);
ensure(flash_lock_write(), "secret write");
mpu_restore(mpu_mode);
- return sectrue;
+ return result;
}
secbool secret_read(uint8_t *data, uint32_t offset, uint32_t len) {
@@ -435,16 +428,13 @@ void secret_bhk_regenerate(void) {
ensure(flash_area_erase(&BHK_AREA, NULL), "Failed regenerating BHK");
ensure(flash_unlock_write(), "Failed regenerating BHK");
- for (int i = 0; i < 2; i++) {
- uint32_t val[4] = {0};
- for (int j = 0; j < 4; j++) {
- val[j] = rng_get();
- }
- secbool res =
- flash_area_write_quadword(&BHK_AREA, i * 4 * sizeof(uint32_t), val);
- memzero(val, sizeof(val));
- ensure(res, "Failed regenerating BHK");
+ uint32_t val[8] = {0};
+ for (int j = 0; j < ARRAY_LENGTH(val); j++) {
+ val[j] = rng_get();
}
+ secbool res = flash_area_write_data(&BHK_AREA, 0, val, sizeof(val));
+ memzero(val, sizeof(val));
+ ensure(res, "Failed regenerating BHK");
mpu_restore(mpu_mode);
Why this scored 23/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.