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

arch/esp32s3_partition: Read data from SPI Flash with decryption #15470

Merged

Conversation

nuttxs
Copy link
Contributor

@nuttxs nuttxs commented Jan 8, 2025

Summary

arch/esp32s3_partition: Read data from SPI Flash at designated address (with decryption)

Impact

esp32s3 add read partition interface with decryption

Testing

nsh> tc_ota_test --read_partion_decrypt
INFO: [label]: otadataxx
OK: get read decryption data from MTD
[CPU0] otadata (0x3ca02a00):
[CPU0] 0000 35 9b f2 07 b4 6d 40 89 28 b4 1e 22 98 7b 4a 36 5....m@.(..".{J6
[CPU0] 0010 ba 89 81 67 77 a3 60 5e 0a e7 51 01 b3 58 c2 f6 ...gw.`^..Q..X..

at designated address (with decryption)
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small labels Jan 8, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 8, 2025

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is missing some key information.

Missing/Needs Improvement:

  • Summary: While the summary provides a title of the change, it lacks crucial details. It needs to explicitly state why this change is necessary. Is it fixing a bug, adding a new feature, or an optimization? What problem does it solve? The summary should also mention the functional part of the code being changed (e.g., the OTA update mechanism).
  • Impact: While it mentions adding a new interface, it lacks specifics.
    • User Impact: Does this change the way users interact with OTA updates? If so, how?
    • Build Impact: Are there any build system changes required (e.g., new Kconfig options)?
    • Hardware Impact: It mentions esp32s3, but is it only for esp32s3? Are any specific peripherals involved?
    • Documentation: Does this require documentation updates? If so, have they been included in the PR? If not, why not?
    • Security: Decryption is mentioned. This is crucial. The PR must explain the security implications thoroughly. What encryption algorithm is used? How are keys managed? What are the potential vulnerabilities?
    • Compatibility: Does this break any existing functionality?
  • Testing: The provided logs only show the "after" scenario. Logs from before the change are needed to demonstrate the difference and prove the fix/feature works. The testing also needs more detail:
    • Build Host: Specify the OS, CPU architecture, and compiler version used for building NuttX.
    • Target: More details are needed. "sim, RISC-V, ARM" isn't sufficient. Specify the specific board and configuration used (e.g., esp32s3-devkitc:default). Was the test performed on real hardware or a simulator (QEMU)?

Example of Improved Summary and Impact Sections:

Summary: This PR adds support for reading encrypted OTA partitions on the esp32s3. Previously, reading OTA partitions with decryption was not possible, preventing secure OTA updates. This change adds a new interface to the MTD subsystem specifically for the esp32s3, enabling secure access to firmware updates. This addresses [NuttX Issue #XXXX] (if applicable).

Impact:

  • New Feature: Adds support for reading encrypted OTA partitions on esp32s3.
  • User Impact: YES. Users can now perform secure OTA updates. They will need to use the new tc_ota_test --read_partion_decrypt command to validate the decrypted data.
  • Build Impact: NO.
  • Hardware Impact: YES. Only affects esp32s3. Uses the SPI flash peripheral and the esp32s3's hardware encryption engine.
  • Documentation Impact: YES. Documentation updates included in this PR explaining the new interface and its usage.
  • Security Impact: YES. This change implements AES-256 decryption for OTA partitions. Keys are stored in efuse and are provisioned during manufacturing. (Expand on key management and potential vulnerabilities).
  • Compatibility Impact: NO. This is a new feature and does not break existing functionality.

By adding this missing information, the PR will be much clearer and easier to review. This will significantly improve the chances of it being merged.

@anchao anchao merged commit cb980cc into apache:master Jan 8, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants