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

Esp32s3 support #5950

Closed
wants to merge 9 commits into from
Closed

Conversation

PaulMartinsen
Copy link

@PaulMartinsen PaulMartinsen commented Jan 4, 2023

Description

Added basic support for ESP32s3 hardware acceleration. Addresses part of #5215 . Note: ESP32s3 doesn't support hardware acceleration of AES-192, so this is now disabled in user_settings.h.

Testing

  • Ran wolf_test_task(); on both ESP32s3 and ESP32. All tests passing.
  • Tests for ED25519 fail on both ESP32 (existing code prior to this PR) and ESP32s3 for me, so HAVE_ED25519 is commented out in user_settings.h.
  • only options in standard user_settings.h have been tested plus WOLFSSL_KEY_GEN, WOLFSSL_CERT_GEN, WOLFSSL_CERT_REQ.
  • only ESPWROOM32 hardware target ESP32 and ESP32s3 tested as I don't have the other hardware.

Checklist

  • [na] added tests
  • [na] updated/added doxygen
  • [y] updated appropriate READMEs
  • [y] Updated manual and documentation

Paul added 5 commits December 21, 2022 17:25
… spaces.

Avoid confusing file/directory prompt for default_config_h file that doesn't have an extension.
* SHA acceleration: added guarded implementation, builds, passes some tests
* AES acceleration: added guarded implementation, builds, untested
* MP acceleration: added guarded implementation, builds, untested
…3 (and ESP32).

* esp32-crypt.h - removed ets_sys.h include; not required for ESP32 and breaks ESP32s3 builds.
* esp32_sha.c - update wc_esp_digest_state to skip reading digest for first block and correctly decode hash for SHA2_512.
* esp32_mp.c fix mul, mulmod, exptmod to pass tests ESP32s3
* random.c - added missing include required to build on Espressif devices when not using hardware acceleration.
* aes.c - return error code when setting Aes key if specific algorithms have been disabled (otherwise tests fail).
* user_settings.h
** disable ED25519 as it fails test for ESP32 & ESP32s3
** disable AES-192 as it is not supported on ESP32s3.
@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@PaulMartinsen PaulMartinsen marked this pull request as ready for review January 4, 2023 21:19
@PaulMartinsen
Copy link
Author

When Stack Smashing Protection Mode is set to normal, the ES25519 test also passes. See #5948 for more info.

@gojimmypi
Copy link
Contributor

Hi @PaulMartinsen I've confirmed your Contributor Agreement has been completed and accepted.

I'll be reviewing this PR and #5964 soon. There's a minor conflict with random.c: could you please refresh from upstream and resolve? Thank you.

@gojimmypi
Copy link
Contributor

Hi @PaulMartinsen I've taken a look at this PR. Several of the wolfssl tests are failing.

Some are known to be bad even on the classic ESP32. These can be addressed by disabling SHA512/224 and SHA512/256 and not enabling SHA384:

#define WOLFSSL_NOSHA512_224
#define WOLFSSL_NOSHA512_256
// #define WOLFSSL_SHA384

There's also a problem with #include "esp_timer.h" when using the ESP-IDF v5 in esp32_util.c.

I believe the above problems are fixed in my ED25519_SHA2_fix branch.

However, I'm seeing other AES192 errors on the ESP32-S3 with HW enabled:

AES      test passed!
E (2482) wolf_hw_aes:   unsupported mode
AES192   test failed!
 error L=10901 code=-173 (Bad function argument)
 [fiducial line numbers: 7515 22991 33351 45478]
I (2492) wolfcrypt_test: Exiting main with return code: -1

Your help is definitely appreciated. Let me know if you'll be able to fix up the above items, otherwise I can address them in my ED25519_SHA2_fix branch and include in my upcoming PR.

For reference:

git clone https://github.com/PaulMartinsen/wolfssl.git pm5950
cd pm5950
git checkout ESP32s3-support
git remote add upstream https://github.com/wolfSSL/wolfssl.git
git fetch upstream
git pull upstream master
cd /mnt/c/SysGCC/esp32/esp-idf/v5.0
 . ./export.sh
cd /mnt/c/temp/pm5950/IDE/Espressif/ESP-IDF/examples/wolfssl_test
idf.py  set-target esp32s3
idf.py -p /dev/ttyS16 build flash monitor

Paul added 2 commits March 8, 2023 08:54
Resolved conflict with esp_random import.
Exclude Espressif backup files from git.
@PaulMartinsen
Copy link
Author

Thanks @gojimmypi . I was calling the test routine from a custom program and didn't quite get the user_settings.h right. Now we have your beautiful build-in place it is much easier now. Thanks!

  • I disabled the SHA512/244, SHA512/256 and SHA384 as recommended.
  • the esp_timer.h issue seems only debug related and hasn't affected me. I suggest we keep the resolution of that one to your branch (or at least a separate step).
  • AES192 isn't supported in hardware for the ESP32s3 & I don't know how to make it use a software fallback, so I have conditionally disabled it in user_settings.h. That could be already resolved in ED25519_SHA2_fix too?

I think this is working now on this branch (that build-in-place really is nice!):

idf.py  set-target esp32s3
idf.py -p COM5 build flash monitor

@PaulMartinsen
Copy link
Author

With ED25519_SHA2_fix having a number of changes relevant to this PR, would it be better for me to do a PR On that repository so the changes can be combined and incorporated together?

@gojimmypi
Copy link
Contributor

Hi @PaulMartinsen - Glad to hear you are making good progress!

Yes, I think it might be best if we incorporate your changes into the ED25519_SHA2_fix branch. As you noted there are quite a few changes there that fix many things including SHA512/244, SHA512/256 and SHA384. I'm happy to manually merge those changes if you'd like, or a PR works too.

I'll take a look at AES as a separate exercise if it is not important for your project. In the case of SHA, more specifically the SHA512/224 that is not supported in hardware acceleration on the ESP32, I have an example of forcing it to software ctx->mode at initialization time. We could add some gating there there for other chipsets that do have the hardware acceleration.

For reference, from docs.espressif:

Espressif_chipset_hardware_encryption_comparison

@gojimmypi gojimmypi marked this pull request as draft March 8, 2023 02:18
@gojimmypi
Copy link
Contributor

Hi @PaulMartinsen you have some very good changes in this PR! Thanks very much for all the time your spent on that.

I've merged your changes into my ED25519_SHA2_fix branch. Some things like your aes and mp updates were trivial to add, as I did not have any changes to those. The esp32_sha.c updates were a bit more challenging, as my version is very different from the wolfSSL master branch at this point.

I do believe I have everything working. I need to do a little bit of homework, as the SHA384 HW test was failing. For now I have it hard-coded to fall back to SW for the ESP32-S3. According to the screen snip (above) the SHA-384 should be implemented in the silicon.

Note that I've updated my user_list.h to now have an #include "sdkconfig.h", and near the bottom I added:

#if defined(CONFIG_IDF_TARGET_ESP32S3) && !defined(NO_WOLFSSL_ESP32WROOM32_CRYPT_AES)
    /* AES192 is not supported on the ESP32-S3 HW at this time */
    #define NO_AES_192
#endif

At some point, I'll need to revisit that and fall back to software, rather than just completely disable AES192.

Heads up there are also some ESP32-S3 changes in @kareem-wolfssl 's #6146.

@gojimmypi
Copy link
Contributor

Hi @PaulMartinsen -

I think most (all?) of these changes have been implemented and are included in the latest release.

Would yo please review and let me know if there's anything outstanding, or if this PR can be closed? Thanks.

@PaulMartinsen
Copy link
Author

Hi @gojimmypi , I think there's lots of water under the bridge from this one so I think we'll call it complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants