Skip to content
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

esp_flash_write_encrypted is broken (IDFGH-4493) #6322

Closed
rojer opened this issue Dec 26, 2020 · 11 comments
Closed

esp_flash_write_encrypted is broken (IDFGH-4493) #6322

rojer opened this issue Dec 26, 2020 · 11 comments
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@rojer
Copy link
Contributor

rojer commented Dec 26, 2020

Environment

  • Development Kit: [ESP32-Wrover-Kit|ESP32-DevKitC|ESP32-PICO-Kit|ESP32-LyraT|ESP32-LyraTD-MSC|none]
  • Kit version (for WroverKit/PicoKit/DevKitC): [v1|v2|v3|v4]
  • Module or chip used: ESP32-WROOM-32
  • IDF version 4.2
  • Build System: Make
  • Compiler version 8.4.0-2020r3
  • Operating System: Linux
  • Using an IDE?: No
  • Power Supply: USB

Problem Description

esp_partition_write() does not work on encrypted partitions.

Expected Behavior

esp_partition_write() should be able to write data to encrypted partitions

Actual Behavior

new API falls back to ROM API (spi_flash_write_encrypted) that doesn't work.
gets stuck unable to set WEL bit properly (more likely, unable to read status register properly).

Steps to reproduce

ESP32D0WDQ6 with GigaDevice flash in 80 MHz QIO mode

[Dec 26 17:35:24.714] I (34) qio_mode: Enabling default flash chip QIO
[Dec 26 17:35:24.714] I (39) boot.esp32: SPI Speed      : 80MHz
[Dec 26 17:35:24.736] I (44) boot.esp32: SPI Mode       : QIO
[Dec 26 17:35:24.736] I (48) boot.esp32: SPI Flash Size : 4MB
...
[Dec 26 17:35:25.451] I (766) spi_flash: detected chip: gd
[Dec 26 17:35:25.451] I (767) spi_flash: flash io: qio

Code to reproduce this issue

enable flash encryption and try to write to an encrypted partition.

works fine if new API is disabled (CONFIG_SPI_FLASH_USE_LEGACY_IMPL=y)

@github-actions github-actions bot changed the title esp_flash_write_encrypted is broken esp_flash_write_encrypted is broken (IDFGH-4493) Dec 26, 2020
@Alvin1Zhang
Copy link
Collaborator

Thanks for reporting, we will look into.

@igrr
Copy link
Member

igrr commented Dec 28, 2020

@rojer if you switch from QIO to DIO mode, does this issue still happen?

@rojer
Copy link
Contributor Author

rojer commented Dec 29, 2020

@igrr set CONFIG_ESPTOOLPY_FLASHMODE_DIO=y CONFIG_ESPTOOLPY_FLASHMODE_QIO=n - nope, doesn't seem to help.

from memory when i was debugging this - after new flash driver initializes, reading status register via rom function starts returning incorrect result. so yes, there is some configuration mismatch but it's not QIO / DIO, it seems.

@igrr
Copy link
Member

igrr commented Dec 29, 2020

Thanks for checking @rojer. We do have an automated test for flash encryption functionality (based on the examples/security/flash_encryption example), and that seems to be passing both on master and on release/v4.2 branch. So we need to find what is different between your setup and ours. Will look into it more.

If you get a chance, could you please try running examples/security/flash_encryption example with its default configuration on your hardware?

@rojer
Copy link
Contributor Author

rojer commented Jan 2, 2021

@igrr investigated further and found the root cause: frequency mismatch between rom settings and the driver.
if memspi_host_config_t.speed and the clock configuration in image parameters (set based on CONFIG_ESPTOOLPY_FLASHFREQ_* variable) get out of sync with reality (and in our case it does), things go awry.
the reason for things to go out of sync is that we sometimes rewrite the image header of the image we write based on command line settings.
i think flash driver should query current ROM settings when initializing to ensure actual config is in sync as opposed to compile-time.

@rojer
Copy link
Contributor Author

rojer commented Jan 2, 2021

alternatively (and preferably), esp_flash driver should sync ROM's settings to the one it uses.

rojer added a commit to cesanta/mongoose-os that referenced this issue Jan 2, 2021
Because modern API falls back to ROM API for encrypted writes,
the flash frequency used by ROM APIs must match the frequency (but curiously not necessarily I/o mode)
set by the modern API. mos tool has been setting frerquenct to 80 MHz since forever (actually since mongoose-os/mos@74448ba)
so we set compile-time default to 80 MHz as well.

This issue has been reported upstream in espressif/esp-idf#6322
@rojer
Copy link
Contributor Author

rojer commented Jan 2, 2021

ok, for now the workaround is pretty simple - set default to 80 mhz, to match what mos tool sets by default (cesanta/mongoose-os@23e8064) but proper fix wshould be made to ensure speed settings agree between ROM and modern APIs or that modern API does not require ROM API for encrypted writes anymore.

@rojer
Copy link
Contributor Author

rojer commented Jan 17, 2021

another twist on this issue is the SPIRAM clock - if it doesn't match the flash settings, ROM access to flash is also broken.

rojer added a commit to cesanta/mongoose-os that referenced this issue Jan 17, 2021
SPIRAM frequency, if enabled, must also match the flash frequency.

espressif/esp-idf#6322
@mythbuster5
Copy link
Collaborator

Sorry for the long wait, I have fixed the issue(flash_encryption failed with 80M flash and 40M psram) with the patch below, could you please have a try? Thanks!

esp32_patch_for_flash_encryption_with_40Mram_80Mflash.txt

@rojer
Copy link
Contributor Author

rojer commented Jan 30, 2021

@mythbuster5 yes, that works, thank you!

projectgus pushed a commit that referenced this issue Feb 10, 2021
0xFEEDC0DE64 added a commit to 0xFEEDC0DE64/esp-idf that referenced this issue Feb 18, 2021
0xFEEDC0DE64 added a commit to 0xFEEDC0DE64/esp-idf that referenced this issue Feb 18, 2021
espressif-bot pushed a commit that referenced this issue Mar 20, 2021
espressif-bot pushed a commit that referenced this issue Mar 24, 2021
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally labels Apr 26, 2021
@Alvin1Zhang
Copy link
Collaborator

Thanks for reporting and sharing updates, feel free to reopen.

espressif-bot pushed a commit that referenced this issue Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

5 participants