-
Notifications
You must be signed in to change notification settings - Fork 809
ESP32-XX: fix hardware flash encryption issue when updating images #2320
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.
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
base: main
Are you sure you want to change the base?
ESP32-XX: fix hardware flash encryption issue when updating images #2320
Conversation
64ec4e9
to
cd51774
Compare
cd51774
to
b3b01c5
Compare
b3b01c5
to
c31bf4e
Compare
Also fix build if hal_espressif is used as IDF code layer (hal) for Espressif Port Signed-off-by: Almir Okato <[email protected]>
Signed-off-by: Almir Okato <[email protected]>
c31bf4e
to
3eb4366
Compare
Signed-off-by: Almir Okato <[email protected]>
Signed-off-by: Almir Okato <[email protected]>
Add cache flush after write/erase operations to avoid getting invalid data when these are followed by read operation. Signed-off-by: Almir Okato <[email protected]>
… encryption is enabled When hardware flash encryption is enabled, force expected erased value (0xFF) into flash when erasing a region, and also always do a real erase before writing data into flash. This is handled on this implementation because MCUboot's state machine relies on erased valued data (0xFF) readed from a previously erased region that was not written yet, however when hardware flash encryption is enabled, the flash read always decrypts whats being read from flash, thus a region that was erased would not be read as what MCUboot expected (0xFF). Signed-off-by: Almir Okato <[email protected]>
3eb4366
to
8ef9137
Compare
@utzig @marekmatej could you please take a look? |
*libhal.a:rtc_time.*(.literal .text .literal.* .text.*) | ||
*libhal.a:secure_boot_signatures_bootloader.*(.literal .text .literal.* .text.*) | ||
*libhal.a:wdt_hal_iram.*(.literal .text .literal.* .text.*) | ||
*libgcc.a:*.*(.literal .text .literal.* .text.*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is libgcc.a
entry really needed?
@@ -218,36 +237,55 @@ int flash_area_read(const struct flash_area *fa, uint32_t off, void *dst, | |||
return 0; | |||
} | |||
|
|||
static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size) | |||
static bool aligned_flash_write(size_t dest_addr, const void *src, size_t size, bool erase) | |||
{ | |||
#ifdef CONFIG_SECURE_FLASH_ENC_ENABLED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esp_flash_encryption_enabled();
will return if encryption is enabled. Why the need of #ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
?
} | ||
} | ||
|
||
if(bytes < sizeof(write_data)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check code-style as there are some missing spaces like in if(bytes < sizeof(write_data)) {
|
||
uint32_t offset = bytes; | ||
|
||
while (bytes_remaining != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the while
loop could start in L333 and grab everything related to the partial read-erase-write.
BOOT_LOG_ERR("%s: Flash erase failed", __func__); | ||
return -1; | ||
} | ||
#if VALIDATE_PROGRAM_OP | ||
|
||
flush_cache(start_addr, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aligned_flash_erase
already flush_cache()
at the end. Is this needed again?
|
||
if (bootloader_flash_erase_range(start_addr, len) != ESP_OK) { | ||
if(!aligned_flash_erase(start_addr, len)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should #ifdef CONFIG_SECURE_FLASH_ENC_ENABLED
be checked before aligned_flash_erase
in here?
When hardware flash encryption is enabled, force expected erased value (0xFF) into flash when erasing a region, and also always do a real erase before writing data into flash.
This is handled on this implementation because MCUboot's state machine relies on erased valued data (0xFF) readed from a previously erased region that was not written yet, however when hardware flash encryption is enabled, the flash read always decrypts whats being read from flash, thus a region that was erased would not be read as what MCUboot expected (0xFF).
This PR also bumps used IDF code version to v5.1.6.
How to test it within Zephyr on ESP32-S3 using a "zeroed" key (DEVELOPMENT AND TEST ONLY, so the device will not be bricked) and updating through Wi-Fi:
Use Zephyr branch from this PR:
zephyrproject-rtos/zephyr#90442
Use hal_espressif branch from this PR:
zephyrproject-rtos/hal_espressif#445
Prepare and enable Flash Encryption on ESP32-S3:
Building the SMPMGR sample application on Zephyr:
<ZEPHYRPROJECT_DIR>/zephyr/samples/subsys/mgmt/mcumgr/smp_svr/boards/esp32s3_devkitm_procpu.overlay
Building and flashing MCUboot:
(MCUBOOT_DIR should be the directory from Zephyr environment:
<ZEPHYRPROJECT_DIR>/bootloader/mcuboot
)<MCUBOOT_DIR>/boot/espressif/port/esp32s3/bootloader.conf
with:Workaround for empty slot1 and scratch regions when testing:
Testing like described, as Flash Encryption would be already enabled, reading an empty region (physical 0xFF values) would result in garbage as the read decryption would decrypt the data unconditionally. So generate a
0xFF
s binary file and flash it to the regions in order to avoid an unexpected behavior:Upload the update image through wifi using
smpmgr
:smpmgr
command. Connect the device to wifi again, fetch the image information and use its hash to confirm the image.