Skip to content

simplify flash area api #6492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TychoVrahe merged 2 commits into from
Mar 6, 2026
Merged

simplify flash area api #6492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f8b9c7
refactor(core): simplify flash area api
TychoVrahe Feb 19, 2026
998f4f2
refactor(core): remove quadword usage from secret implementation
TychoVrahe Feb 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 8 additions & 18 deletions core/embed/sec/secret/stm32u5/secret.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +147 to +150
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 1) Inspect flash_area_write_data and its cast patterns ==="
fd -t f "flash_area.c" | head -5

Repository: trezor/trezor-firmware

Length of output: 149

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 2) Look for uint32_t cast patterns in flash_area write path ==="
rg -n 'uint32_t.*\*data|data32|const uint32_t' storage/ core/ --max-count=20

Repository: trezor/trezor-firmware

Length of output: 19458

🏁 Script executed:

#!/bin/bash
#!/bin/bash
set -euo pipefail

echo "=== 3) Examine secret_write implementation and callers ==="
fd -t f "secret.c" core/embed/sec/secret/ | xargs wc -l

Repository: trezor/trezor-firmware

Length of output: 268

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 4) Check lines 147-150 context in secret.c ==="
fd -t f "secret.c" core/embed/sec/secret/stm32u5/ | xargs cat -n | sed -n '140,160p'

Repository: trezor/trezor-firmware

Length of output: 892

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 5) Check lines 431-437 context for BHK regeneration ==="
fd -t f "secret.c" core/embed/sec/secret/stm32u5/ | xargs cat -n | sed -n '425,445p'

Repository: trezor/trezor-firmware

Length of output: 920

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 6) Search for alignment/aliasing build flags ==="
rg -n 'strict-aliasing|UNALIGN|align' . -t c -t h --max-count=15

Repository: trezor/trezor-firmware

Length of output: 26882

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 1) Find SECRET_BHK_LEN or similar constant ==="
rg -n 'SECRET_BHK_LEN|BHK_LEN|#define.*8.*BHK' core/embed/sec/secret/ -A2 -B2

Repository: trezor/trezor-firmware

Length of output: 2061

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 2) Check flash_area.h alignment requirements ==="
fd -t f "flash_area.h" | xargs sed -n '70,95p'

Repository: trezor/trezor-firmware

Length of output: 1529

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== 3) Examine flash_area_write_data_padded signature and callers ==="
rg -n 'flash_area_write_data' core/embed/sec/secret/stm32u5/secret.c -B3 -A1

Repository: trezor/trezor-firmware

Length of output: 653

Confirm alignment expectations in secret_write and improve BHK array size maintainability.

At line 147, secret_write passes const uint8_t *data directly to flash_area_write_data, which casts it to const uint32_t * (line 162 in flash_area.c) and dereferences it. The flash_area.h header documents that data must be aligned to flash block size, but secret_write does not enforce or document this contract to callers—add alignment documentation to its signature or ensure all callers provide properly aligned buffers.

For lines 431–437, the BHK regeneration loop correctly uses ARRAY_LENGTH(val) and sizeof(val), so that pattern is sound. However, val[8] is hardcoded while elsewhere the codebase uses SECRET_BHK_LEN / sizeof(uint32_t) (line 401). Consider defining the array size as uint32_t val[SECRET_BHK_LEN / sizeof(uint32_t)] for consistency and maintainability.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@core/embed/sec/secret/stm32u5/secret.c` around lines 147 - 150, secret_write
currently forwards const uint8_t *data to flash_area_write_data which expects
flash-aligned data (and casts to const uint32_t *), but secret_write neither
enforces nor documents this alignment; update secret_write to either validate
that data is aligned to the flash block size (e.g. check pointer and length
against FLASH_WRITE_ALIGNMENT / flash block size) or add clear documentation to
its function signature/comments that callers must pass an aligned buffer, and
adjust callers if necessary; additionally replace the hardcoded local array
declaration uint32_t val[8] with a maintainable sized declaration using
SECRET_BHK_LEN (e.g. uint32_t val[SECRET_BHK_LEN / sizeof(uint32_t)]) so the BHK
buffer size derives from SECRET_BHK_LEN and stays consistent with the
regeneration loop that uses ARRAY_LENGTH(val) and sizeof(val).

Copy link
Copy Markdown
Contributor Author

@TychoVrahe TychoVrahe Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coderabbit is right, strictly speaking we break some rules here.

HAL function requires that data address is aligned to 32bits, which we incorrectly translate to requirement for block alignment.

The 32 bit requirement is presumably because of unaligned access to the data, but we have that enabled so it works anyway.

As this is also not handled elsewhere in codebase, i suggest we dont deal with tis in this PR, but potentially fix this as separate issue.

what do you think, @cepetr ?

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Copy Markdown
Contributor

@cepetr cepetr Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. Unaligned arrays are handled correctly on cortex-m.

However, I think we should reconsider this practice we agreed on previously, as it could cause significant issues in the future on platforms that do not support it.

}

secbool secret_read(uint8_t *data, uint32_t offset, uint32_t len) {
Expand Down Expand Up @@ -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);

Expand Down
7 changes: 5 additions & 2 deletions core/embed/sys/flash/stm32u5/flash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@

#ifdef KERNEL_MODE

#define FLASH_QUADWORD_WORDS (4)
#define FLASH_QUADWORD_SIZE (FLASH_QUADWORD_WORDS * sizeof(uint32_t))

Copy link
Copy Markdown
Contributor

@cepetr cepetr Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flash_write_quadword() at the end of file may be static now

Copy link
Copy Markdown
Contributor Author

@TychoVrahe TychoVrahe Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 5477623

#ifdef STM32U585xx
#define FLASH_BANK_PAGES 128
#define FLASH_SECTOR_COUNT (FLASH_BANK_PAGES * 2)
Expand Down Expand Up @@ -168,8 +171,8 @@ secbool flash_sector_erase(uint16_t sector) {
return sectrue;
}

secbool flash_write_quadword(uint16_t sector, uint32_t offset,
const uint32_t *data) {
static secbool flash_write_quadword(uint16_t sector, uint32_t offset,
const uint32_t *data) {
uint32_t address =
(uint32_t)flash_get_address(sector, offset, FLASH_QUADWORD_SIZE);
if (address == 0) {
Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/D002/discovery2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ def configure(
"USE_HASH_PROCESSOR=1",
"USE_STORAGE_HWKEY=1",
"USE_TAMPER=1",
"USE_FLASH_BURST=1",
"USE_OEM_KEYS_CHECK=1",
]

Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/T3B1/trezor_t3b1_revB.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ def configure(
("USE_HASH_PROCESSOR", "1"),
("USE_STORAGE_HWKEY", "1"),
("USE_TAMPER", "1"),
("USE_FLASH_BURST", "1"),
("USE_OEM_KEYS_CHECK", "1"),
("USE_PVD", "1"),
]
Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/T3T1/trezor_t3t1_revE.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ def configure(
("USE_HASH_PROCESSOR", "1"),
("USE_STORAGE_HWKEY", "1"),
("USE_TAMPER", "1"),
("USE_FLASH_BURST", "1"),
("USE_OEM_KEYS_CHECK", "1"),
("USE_PVD", "1"),
]
Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/T3W1/trezor_t3w1_revA.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ def configure(
("USE_HASH_PROCESSOR", "1"),
("USE_STORAGE_HWKEY", "1"),
("USE_TAMPER", "1"),
("USE_FLASH_BURST", "1"),
("USE_OEM_KEYS_CHECK", "1"),
]

Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/T3W1/trezor_t3w1_revB.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ def configure(
("USE_HASH_PROCESSOR", "1"),
("USE_STORAGE_HWKEY", "1"),
("USE_TAMPER", "1"),
("USE_FLASH_BURST", "1"),
("USE_OEM_KEYS_CHECK", "1"),
]

Expand Down
1 change: 0 additions & 1 deletion core/site_scons/models/T3W1/trezor_t3w1_revC.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ def configure(
("USE_HASH_PROCESSOR", "1"),
("USE_STORAGE_HWKEY", "1"),
("USE_TAMPER", "1"),
("USE_FLASH_BURST", "1"),
("USE_OEM_KEYS_CHECK", "1"),
]

Expand Down
3 changes: 3 additions & 0 deletions core/site_scons/models/stm32u5_common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ def stm32u5_common_files(env, features_wanted, defines, sources, paths):
defines += [
("STM32_HAL_H", "<stm32u5xx.h>"),
("FLASH_BLOCK_WORDS", "4"),
("USE_FLASH_BURST", "1"),
("FLASH_BURST_WORDS", "32"),
("FLASH_BURST_SIZE", "128"),
("USE_TRUSTZONE", "1"),
]

Expand Down
14 changes: 1 addition & 13 deletions storage/flash_area.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,19 +108,7 @@ secbool flash_area_write_word(const flash_area_t *area, uint32_t offset,
return flash_write_word(sector, sector_offset, data);
}

#else // not defined FLASH_BIT_ACCESS

secbool flash_area_write_quadword(const flash_area_t *area, uint32_t offset,
const uint32_t *data) {
uint16_t sector;
uint32_t sector_offset;
if (get_sector_and_offset(area, offset, &sector, &sector_offset) != sectrue) {
return secfalse;
}
return flash_write_quadword(sector, sector_offset, data);
}

#endif // not defined FLASH_BIT_ACCESS
#endif // FLASH_BIT_ACCESS

#ifdef USE_FLASH_BURST
secbool flash_area_write_burst(const flash_area_t *area, uint32_t offset,
Expand Down
2 changes: 0 additions & 2 deletions storage/flash_area.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ secbool __wur flash_area_write_byte(const flash_area_t *area, uint32_t offset,
secbool __wur flash_area_write_word(const flash_area_t *area, uint32_t offset,
uint32_t data);
#endif
secbool __wur flash_area_write_quadword(const flash_area_t *area,
uint32_t offset, const uint32_t *data);

secbool __wur flash_area_write_burst(const flash_area_t *area, uint32_t offset,
const uint32_t *data);
Expand Down
12 changes: 1 addition & 11 deletions storage/flash_ll.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,6 @@
// 1. Non-uniform sector number on STM32F4
// 2. Uniform page number on STM32U5

#define FLASH_QUADWORD_WORDS (4)
#define FLASH_QUADWORD_SIZE (FLASH_QUADWORD_WORDS * sizeof(uint32_t))

#define FLASH_BURST_WORDS (8 * FLASH_QUADWORD_WORDS)
#define FLASH_BURST_SIZE (FLASH_BURST_WORDS * sizeof(uint32_t))

#define FLASH_BLOCK_SIZE (sizeof(uint32_t) * FLASH_BLOCK_WORDS)

typedef uint32_t flash_block_t[FLASH_BLOCK_WORDS];
Expand All @@ -50,7 +44,7 @@ typedef uint32_t flash_block_t[FLASH_BLOCK_WORDS];
#error Unsupported number of FLASH_BLOCK_WORDS.
#endif

// Returns the size of the a continuous area of sectors
// Returns the size of a continuous area of sectors
// Returns 0 if any of the sectors is out of range
uint32_t flash_sector_size(uint16_t first_sector, uint16_t sector_count);

Expand Down Expand Up @@ -80,10 +74,6 @@ secbool __wur flash_write_word(uint16_t sector, uint32_t offset, uint32_t data);

#endif

// Writes a 16-byte block to specified 'offset' inside a flash 'sector'
secbool __wur flash_write_quadword(uint16_t sector, uint32_t offset,
const uint32_t *data);

// Writes a 128-byte burst to specified 'offset' inside a flash 'sector'
secbool __wur flash_write_burst(uint16_t sector, uint32_t offset,
const uint32_t *data);
Expand Down
5 changes: 0 additions & 5 deletions storage/tests/c/flash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,6 @@ secbool flash_write_word(uint16_t sector, uint32_t offset, uint32_t data) {
return flash_write(sector, offset, (uint8_t *)&data, sizeof(uint32_t));
}

secbool flash_write_quadword(uint16_t sector, uint32_t offset,
const uint32_t *data) {
return flash_write(sector, offset, (uint8_t *)data, 4 * sizeof(uint32_t));
}

secbool flash_write_burst(uint16_t sector, uint32_t offset,
const uint32_t *data) {
return flash_write(sector, offset, (uint8_t *)data, 32 * sizeof(uint32_t));
Expand Down