Skip to content

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

Merged
merged 3 commits into from
Feb 23, 2017
Merged

Improvements in EspClass #222

merged 3 commits into from
Feb 23, 2017

Conversation

arcao
Copy link
Contributor

@arcao arcao commented Feb 18, 2017

  • fixed not working functions for flash chip size, speed and mode
  • added function to retrieve chip revision from eFuse
  • flashRead / flashWrite supports encrypted flash (reverted, see @igrr review)

- fixed not working functions for flash chip size, speed and mode
- added function to retrieve chip revision from eFuse
- flashRead / flashWrite supports encrypted flash
@@ -55,6 +57,7 @@ class EspClass
~EspClass() {}
void restart();
uint32_t getFreeHeap();
uint8_t getCpuRevision();
Copy link
Member

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.

@arcao
Copy link
Contributor Author

arcao commented Feb 18, 2017

OK, renamed.

@@ -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;
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

@igrr igrr Feb 18, 2017

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).

}

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;
Copy link
Member

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.

@me-no-dev
Copy link
Member

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.
@me-no-dev me-no-dev merged commit 00c1a65 into espressif:master Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants