Various Espressif HW crypto, SHA2, AES, MP updates. #6287

gojimmypi · 2023-04-11T13:50:08Z

Description

This PR includes the bulk of the Espressif-specific wolfcrypt updates as related to HW acceleration, separated out into this PR from my ED25519_SHA2_fix branch.

Code is considerably more multi-thread RTOS friendly with a improved robust fallback to software when the HW engine is busy. There's a new breadcrumb initializer attribute for keeping track of the address of the ctx that initialized a given instance: this is quite handy for detecting an unexpected copy of a ctx object and in particular ensuring only one instance can have active hardware-accelerated encryption active at a given time.

This is not the final update, but the as the changes have been accumulating for literally months, this is a good working version.

See #6234 for Espressif Roadmap and related updates.

This PR resolves:

Fixes zd# n/a

Testing

How did you test?

Run Espressif benchmark and wolfssl test apps.

optional linux test:

git checkout Espressif_HW_Updates
./autogen.sh
./configure CC=clang --enable-all && make && ./wolfcrypt/test/testwolfcrypt
./wolfcrypt/benchmark/benchmark

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

dgarske

Wow a ton of great work here. Please do a couple cleanups and then it should be ready to merge.

dgarske · 2023-04-11T16:51:33Z

wolfcrypt/benchmark/benchmark.c

-    #include <xtensa/hal.h> /* reminder Espressif RISC-V not yet implemented */
+    #if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)
+        #include "driver/gptimer.h"
+        gptimer_handle_t esp_gptimer = NULL;


Please add static. These don't need to be available outside benchmark.

Got it. Done.

dgarske · 2023-04-11T16:52:13Z

wolfcrypt/benchmark/benchmark.c

@@ -1006,8 +1020,13 @@ static const char* bench_desc_words[][15] = {
        /* reminder: unsigned long long max = 18,446,744,073,709,551,615 */

        /* the currently observed clock counter value */
+#if defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6)
+        word64 thisVal = 0;;


Might consider uint64_t to match gptimer_get_raw_count. Does it need ;;? Just one ; please.

Ah yes, good catch.

dgarske · 2023-04-11T16:53:49Z

wolfcrypt/src/port/Espressif/esp32_aes.c

-
+#if CONFIG_IDF_TARGET_ESP32S3
+    if (mode_ == 1 || mode_ == 5 || mode_ == 7) {
+       ESP_LOGE(TAG, "  unsupported mode");


Check indents here.

ah yes, I neglected to format the code copied from #5950

I've cleaned up this, and others too.

dgarske · 2023-04-11T16:54:08Z

wolfcrypt/src/port/Espressif/esp32_aes.c

@@ -164,7 +185,27 @@ static void esp_aes_bk(const byte* in, byte* out)
 #endif

    ESP_LOGV(TAG, "enter esp_aes_bk");
+#if CONFIG_IDF_TARGET_ESP32S3
+    /* See esp32 - s3 technical reference manual �19.4.3 Operation process


Copy paste has unicode here before 19.4.3. Please correct.

unicode char removed

dgarske · 2023-04-11T16:56:27Z

wolfcrypt/src/port/Espressif/esp32_mp.c

+    DPORT_REG_WRITE(RSA_INTERRUPT_REG, 0); // 0 => no interrupt; 1 => interrupt on completion.
+
+    /* 3. Write (N_result_bits/32 - 1) to the RSA_MODE_REG. */
+    uint32_t uOperandBits = max(max(Xs, Ys), Ms);


Same variable declarations at top.

done. also renamed to simply OperandBits

dgarske · 2023-04-11T16:57:23Z

wolfcrypt/src/port/Espressif/esp32_sha.c

+    if (src->ctx.mode == ESP32_SHA_HW) {
+        /* Get a copy of the HW digest, but don't process it. */
+        ESP_LOGI(TAG, "esp_sha384_ctx_copy esp_sha512_digest_process");
+        ret = esp_sha512_digest_process(dst, 0);


Should below occur even if esp_sha512_digest_process returns a failure?

esp_sha512_digest_process is currently hard-coded to return success, and remains so in this PR.

So, no: code should abort. This should be checked & I will revisit in the next update.

@dgarske on this topic, which flow model to you prefer:

mid-function return <code>

goto out

There are plenty of examples of each. I'm generally not a big fan of goto, but in the this case of exit, it makes sense.

if (something) { goto out; } else { return err; } ...more code.... out: return ret;

Neither. The ideal is one exit and no goto's. Like this:

int rc; rc = do_func() if (rc == 0) rc = do_func2() if (rc == 0) rc = do_func3() return rc;

ok, might as well completely resolve this topic: What about in the case of bad parameters?

What are your thoughts on return BAD_FUNC_ARG early on? Good practice for this extra exit, eh?

int coolthing(int* param) { int rc; if (param == NULL) { return BAD_FUNC_ARG; } rc = do_func() if (rc == 0) rc = do_func2() if (rc == 0) rc = do_func3() return rc; }

Or would you prefer to wrap the entire function in if (all good params) { } ?

dgarske · 2023-04-11T16:58:06Z

wolfcrypt/src/port/Espressif/esp32_sha.c

    wc_esp_wait_until_idle();
+#if CONFIG_IDF_TARGET_ESP32S3
+    uint32_t *pMessageSource = (uint32_t *)data;


Variables declared at top please.

dgarske · 2023-04-11T16:58:14Z

wolfcrypt/src/port/Espressif/esp32_sha.c

+    wc_esp_wait_until_idle();
+
+    /* read hash result into buffer & flip endiness */
+    uint32_t* pHashDestination = (uint32_t*)hash;


gojimmypi · 2023-04-12T08:45:46Z

Wow a ton of great work here.

Thank you! I cannot take all of the credit, as @PaulMartinsen contributed some excellent ESP32-S3 Xtensa support in #5950. As I was not yet ready to create a PR to wolfssl, I manually merged most of his changes from that PR into my branch so that we could move forward on his project.

This PR does not fully support ESP32-C3 yet. While making the changes here, I was reminded that although there are some changes for the RISC-V chipsets (ESP32-C3 and ESP32-C6), not all of the essential updates are here for optimal user experience. There are other interdependent changes in my ED25519_SHA2_fix branch, specifically the new CMakeLists.txt. PR for that coming soon.

Please do a couple cleanups and then it should be ready to merge.

I've completed some cleanups & will push updates soon after more testing.

…essif_HW_Updates

gojimmypi · 2023-04-16T15:05:45Z

I've applied the code review suggestions, but build is still failing:

compiled without BUILD_INTROSPECTION.
skipping WOLFSSL_EXTERNAL_TEST because -UHAVE_ECC configuration of build is incompatible.
SKIP scripts/external.test (exit status: 77)

FAIL: scripts/openssl
=====================

and

# ./examples/server/server -p 59482 -g -v d -x -i -a  -l ALL "" "" ""
wolfSSL server started successfully on port 59482
waiting for OpenSSL_RSA ready...
OpenSSL_RSA ready!
waiting for wolfSSL_RSA ready...
SSL_accept error -341, record layer length error
wolfSSL error: SSL_accept failed
Continuing server execution...

and

SSL_accept error -501, can't match cipher suite
wolfSSL error: SSL_accept failed
140419066090816:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../ssl/record/rec_layer_s3.c:1543:SSL alert number 40
Continuing server execution...

I've looked over my update & cannot seem to find anything related to OpenSSL and HAVE_ECC . Any suggestions would be appreciated.

gojimmypi · 2023-04-18T09:23:25Z

Jenkins retest this please.

dgarske · 2023-04-18T15:31:05Z

wolfcrypt/src/sha.c

 int wc_InitSha_ex(wc_Sha* sha, void* heap, int devId)
 {
    int ret = 0;
-
-    if (sha == NULL)
+    (void)devId;


There is already a (void)devId; below at line 588

Removed duplicate

Changes should be less intrusive by default. Please revert yours and leave the original else logic, which should cover 100% of the cases.

wolfcrypt/src/sha.c

dgarske · 2023-04-18T16:18:38Z

wolfcrypt/src/sha.c

@@ -812,6 +864,9 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
    else {
        ret = esp_sha_digest_process(sha, 1);
    }
+#elif defined(WOLFSSL_USE_ESP32C3_CRYPT_HASH_HW)


This and the #else case are the same. Please combine and just document with a comment.

Yes, they were the same on purpose as a placeholder for the -C3, but I agree that's probably not a great idea. Combined and comment added as requested.

dgarske · 2023-04-18T16:19:26Z

wolfcrypt/src/sha.c

+#ifdef WOLFSSL_SMALL_STACK
+    wc_Sha* tmpSha;
+#else
+    wc_Sha  tmpSha[sizeof(wc_Sha)];


This is not valid. Please use wc_Sha tmpSha[1]; and do not duplicate code below.

eek. I don't know what I was thinking that day! We certainly don't need a massive array of those, eh? Good catch, thank you. Fixed.

wolfcrypt/src/sha.c

dgarske · 2023-04-18T16:30:38Z

wolfcrypt/src/sha512.c

@@ -1426,10 +1456,19 @@ int wc_InitSha384_ex(wc_Sha384* sha384, void* heap, int devId)
    sha384->devId = devId;
    sha384->devCtx = NULL;
 #endif
+#if defined(WOLFSSL_USE_ESP32WROOM32_CRYPT_HASH_HW)
+        if (sha384->ctx.mode != ESP32_SHA_INIT) {


Check indent here

Ah yes, there was no reason for that indent. Fixed.

dgarske · 2023-04-18T21:23:52Z

wolfcrypt/src/sha.c

            if (sha->ctx.mode == ESP32_SHA_INIT) {
+                ESP_LOGV("sha", "wc_ShaUpdate try hardware");


Should you have a log string declared at top and use it?

#ifdef WOLFSSL_ESPIDF /* Define the ESP_LOGx(TAG, "" value for output messages here. ** ** Beware of possible conflict in test.c (that one now named TEST_TAG) */ static const char* TAG = "wc_sha"; #endif

yes. updated.

dgarske · 2023-04-18T21:25:26Z

wolfcrypt/src/sha512.c

@@ -1426,10 +1456,19 @@ int wc_InitSha384_ex(wc_Sha384* sha384, void* heap, int devId)
    sha384->devId = devId;
    sha384->devCtx = NULL;
 #endif
+#if defined(WOLFSSL_USE_ESP32WROOM32_CRYPT_HASH_HW)
+    if (sha384->ctx.mode != ESP32_SHA_INIT) {
+        ESP_LOGV("SHA384", "Set ctx mode from prior value: "


Same should there be a logging string? wc_sha512 in all cases. SHA2-384 is SHA2-512 truncated.

Yes, updated.

This was the only ESP_LOG in the entire file. It was only an "interesting" case for SHA384 during debugging and only displays during verbose Espressif logging.

But you are correct: it should have been declared as a static const char* TAG

dgarske · 2023-04-18T21:26:08Z

wolfcrypt/src/sha.c

 int wc_InitSha_ex(wc_Sha* sha, void* heap, int devId)
 {
    int ret = 0;
-
-    if (sha == NULL)
+    (void)devId;


Changes should be less intrusive by default. Please revert yours and leave the original else logic, which should cover 100% of the cases.

wolfssl/wolfcrypt/port/Espressif/esp32-crypt.h

dgarske · 2023-04-18T21:27:33Z

wolfssl/wolfcrypt/port/Espressif/esp32-crypt.h

+
+        /* we'll keep track of our own locks.
+        ** actual enable/disable only occurs for ref_counts[periph] == 0 */
+        byte isfirstblock; /* 0 is not first block; 1 = is first block   */


Consider bit-field byte isfirstblock:1;.

Changing only isfirstblock:1 would not have much benefit alone, but I also changed lockDepth:7, reducing the struct size from 20 to 16 bytes.

There's an expected slight performance penalty: 19.946 MiB/s for SHA-512 using bit-length bytes, vs 20.020 MiB/s using integers:

Bit-sized:

SHA 21 MiB took 1.000 seconds, 21.484 MiB/s Cycles per byte = 7.10 SHA-256 21 MiB took 1.000 seconds, 20.825 MiB/s Cycles per byte = 7.32 SHA-512 20 MiB took 1.000 seconds, 19.946 MiB/s Cycles per byte = 7.65

Integers:

SHA 21 MiB took 1.000 seconds, 21.411 MiB/s Cycles per byte = 7.12 SHA-256 21 MiB took 1.000 seconds, 20.728 MiB/s Cycles per byte = 7.36 SHA-512 20 MiB took 1.000 seconds, 20.020 MiB/s Cycles per byte = 7.62

Do you think it's worthwhile to have a gate like WOLFSSL_SP_SMALL to make these optional, and of so... which would you use?

* various Espressif HW crypto, SHA2, AES, MP updates. * code review updates & cleanup * clean trailing whitespace * cleanup per code review * removed additional unused WOLFSSL_USE_ESP32C3_CRYPT_HASH_HW * Code review updates; pack & order WC_ESP32SHA * clean up TAG text for Espressif ESP_LOG()

various Espressif HW crypto, SHA2, AES, MP updates.

002bef6

gojimmypi mentioned this pull request Apr 11, 2023

ESP32 Espressif Improvements - Roadmap Summary #6234

Open

72 tasks

gojimmypi self-assigned this Apr 11, 2023

gojimmypi added enhancement New Chipset labels Apr 11, 2023

gojimmypi requested review from dgarske, kareem-wolfssl and JacobBarthelmeh April 11, 2023 15:25

gojimmypi assigned dgarske Apr 11, 2023

gojimmypi requested a review from bandi13 April 11, 2023 15:26

dgarske requested changes Apr 11, 2023

View reviewed changes

dgarske removed their assignment Apr 11, 2023

gojimmypi added 3 commits April 15, 2023 21:30

Merge branch 'master' of https://github.com/wolfSSL/wolfssl into Espr…

521956c

…essif_HW_Updates

code review updates & cleanup

e1e3be6

clean trailing whitespace

eedd195

gojimmypi requested a review from dgarske April 18, 2023 10:43

gojimmypi assigned dgarske Apr 18, 2023

dgarske requested changes Apr 18, 2023

View reviewed changes

dgarske removed their assignment Apr 18, 2023

gojimmypi added 2 commits April 18, 2023 21:52

cleanup per code review

d2affec

removed additional unused WOLFSSL_USE_ESP32C3_CRYPT_HASH_HW

1b7ef72

gojimmypi requested a review from dgarske April 18, 2023 20:11

gojimmypi assigned dgarske Apr 18, 2023

dgarske requested changes Apr 18, 2023

View reviewed changes

dgarske removed their assignment Apr 18, 2023

gojimmypi added 2 commits April 19, 2023 12:04

Code review updates; pack & order WC_ESP32SHA

976cb80

clean up TAG text for Espressif ESP_LOG()

25e0918

gojimmypi requested a review from dgarske April 19, 2023 13:03

gojimmypi assigned dgarske Apr 19, 2023

dgarske approved these changes Apr 19, 2023

View reviewed changes

dgarske merged commit 5100380 into wolfSSL:master Apr 19, 2023

		if (sha->ctx.mode == ESP32_SHA_INIT) {
		ESP_LOGV("sha", "wc_ShaUpdate try hardware");

Various Espressif HW crypto, SHA2, AES, MP updates. #6287

Various Espressif HW crypto, SHA2, AES, MP updates. #6287

Conversation

gojimmypi commented Apr 11, 2023 • edited Loading

Description

Testing

Checklist

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gojimmypi Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gojimmypi commented Apr 12, 2023

gojimmypi commented Apr 16, 2023

gojimmypi commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gojimmypi commented Apr 11, 2023 •

edited

Loading

gojimmypi Apr 12, 2023 •

edited

Loading