-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Improvements in EspClass #222
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
Conversation
- fixed not working functions for flash chip size, speed and mode - added function to retrieve chip revision from eFuse - flashRead / flashWrite supports encrypted flash
cores/esp32/Esp.h
Outdated
@@ -55,6 +57,7 @@ class EspClass | |||
~EspClass() {} | |||
void restart(); | |||
uint32_t getFreeHeap(); | |||
uint8_t getCpuRevision(); |
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.
Suggest getChipRevision
, as this is not really the CPU revision.
OK, renamed. |
cores/esp32/Esp.cpp
Outdated
@@ -199,10 +204,10 @@ bool EspClass::flashEraseSector(uint32_t sector) | |||
|
|||
bool EspClass::flashWrite(uint32_t offset, uint32_t *data, size_t size) | |||
{ | |||
return spi_flash_write(offset, (uint32_t*) data, size) == ESP_OK; | |||
return spi_flash_write_encrypted(offset, (uint32_t*) data, size) == ESP_OK; |
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.
This will unconditionally encrypt and write to flash, with the restriction that writes must be 16-byte aligned. This may not be always desired, for example a file system like SPIFFS will try to write smaller chunks of data and fail due to this restriction. Also NAND way of writing (i.e. flipping 1s to 0s) will not work with spi_flash_write_encrypted.
I suggest adding APIs which specifically do encrypted writes (or even better, adding a library for this purpose).
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.
Thanks for info. I'll revert this back and add note that this doesn't work with encrypted flash.
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.
That makes sense. I suppose that the other things which is missing in Arduino for flash encryption support are:
- some kind of way to select whether the bootloader needs to perform flash encryption
- some place to store the unique per-device flash encryption key on the PC (and make it available to different Arduino installations on the same computer?)
- a way to say "I want to enable flash encryption for this board", which will cause the key to be generated and stored to the aforementioned location. This part may be implemented using an IDE plugin.
- some diagnostics when a user tries to upload sketch into an ESP which has flash encryption enabled, without having a valid encryption key for this ESP.
Overall, this may be a lot of work. I'm not 100% sure that Arduino users need flash encryption in the first place (which is not even very useful without enabling Secure Boot).
cores/esp32/Esp.cpp
Outdated
} | ||
|
||
bool EspClass::flashRead(uint32_t offset, uint32_t *data, size_t size) | ||
{ | ||
return spi_flash_read(offset, (uint32_t*) data, size) == ESP_OK; | ||
return spi_flash_read_encrypted(offset, (uint32_t*) data, size) == ESP_OK; |
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.
Same here, with flash encryption enabled, this will always try to decrypt data, even if it wasn't encrypted in the first place. This isn't a problem if data is always encrypted, but as I have mentioned above, unconditionally encrypting data may be problematic due to alignment restrictions and bit flipping not being possible.
I dont think we should venture in flash encryption in arduino yet. I prefer getting the system up unencrypted first then see what will work encrypted and what will need to be adapted. I would like to really spend time with encryption and test its ins and outs first. |
Reading and writing to encrypted flash has to be aligned to 16-bytes. Also NAND way of writing (i.e. flipping 1s to 0s) will not work with spi_flash_write_encrypted. Note: spi_flash_read_encrypted will always try to decrypt data, even if it wasn't encrypted in the first place.
(reverted, see @igrr review)flashRead
/flashWrite
supports encrypted flash