-
Notifications
You must be signed in to change notification settings - Fork 834
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
Espressif HW Improvements #6624
Conversation
de3a5e4
to
3bc952a
Compare
I'm converting this to draft while I investigate the PRB failure after merging with upstream conflict, as well as an observed TLS client/server problem that may or may not be related. |
3bc952a
to
33dd5b7
Compare
wolfcrypt/src/sha.c
Outdated
@@ -879,7 +904,7 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash) | |||
|
|||
XMEMCPY(hash, (byte *)&sha->digest[0], WC_SHA_DIGEST_SIZE); | |||
|
|||
(void)InitSha(sha); /* reset state */ | |||
ret = InitSha(sha); /* reset state */ |
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.
Could this be stepping on another error code? Not sure this is the right thing to do. Might let ShaFinal() return a success code, where in fact it failed.
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.
oh, that's excellent! thank you! that at least solved a different problem I was working on regarding TLS client/server. Indeed the return code was being stomped on when the HW was busy and needed to fall back to SW.
Not sure it will fix the current PRB problem that was previously passing. The only change I made was to AES here after @dgarske added the !defined(WOLFSSL_SILABS_SE_ACCEL)
upstream.
The aes.c
logic I had earlier may have been wrong and had been updated in this PR squash. @dgarske please confirm your SILABS is still working as desired with my changes.
33dd5b7
to
25d1d50
Compare
Really looking forward to this update. Thanks for all the hard work @gojimmypi & all. |
25d1d50
to
c18fb50
Compare
Clarification: This PR includes SW support for AES-192 on the ESP32-S3 which has AES acceleration, just not for AES-192. When defining To completely disable all AES HW acceleration on the ESP32, define To completely disable all HW and all SW AES, define |
|
||
/* exit codes to be used in tfm.c, sp_int.c, integer.c, etc. */ | ||
/* TODO what numbers do we really want to use? */ | ||
#define MP_HW_ERROR (-106) /* hardware error, consider SW fallback */ |
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.
can WC_HW_WAIT_E and WC_HW_E be used?
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.
oh, good suggestion. The WC_HW_E
is fairly generic, whereas the MP_HW_ERROR
is specific to the math acceleration.
I'm ok with either. Which do you prefer?
This is related to the above code review comment and #6565
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.
I think we should be using the error-crypt.h errors over making a new section for errors that need to be maintained and searched out when comparing return values. Additional errors could be appended to that list if needed. For more verbose error messages though please use the WOLFSSL_MSG() debug macros. For example we have a lot of sections of code that can trigger a BAD_FUNC_ARG error or MEMORY_E, which is pretty generic, and then the issue can be debugged farther in debug mode with additional WOLFSSL_MSG() logs.
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.
The ESP specific fallback error messages can stay here in esp32-crypt.h. Thanks for looking for which error messages could be reused from error-crypt.h.
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.
I've taken your suggestion for WC_HW_E
instead of MP_HW_ERROR
and WC_HW_WAIT_E
instead of MP_HW_BUSY
.
I'll keep MP_HW_FALLBACK
that is more of a state hint than an error (e.g. "all is ok, but you need to fall back to SW").
I'll also keep MP_HW_VALIDATION_ACTIVE
that is only useful during debugging. This too is an indication that a SW calc is explicitly needed to compare to a prior HW calc for comparison.
Both will remain in error-crypt.h
with addition comments regarding usage.
/* Compare MATH_INT_T A to MATH_INT_T B | ||
* During debug, the strings name_A and name_B can help | ||
* identify variable name. */ | ||
int esp_mp_cmp(char* name_A, MATH_INT_T* A, char* name_B, MATH_INT_T* B); |
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.
we should control the visibility of these API, if used only in the wolfSSL library than please use WOLFSSL_LOCAL
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.
hm. Interesting. Good idea. Do you think this is something an end user would ever want to use, or is it just for our own internal diagnostics and testing?
I think I will make it WOLFSSL_LOCAL
until someone requests it, eh?
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.
Better to error on the side of WOLFSSL_LOCAL. Please do this though for all functions in the .h file that are only used locally. Keeping a clear boundary of what are public API and private API is good.
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.
Agreed. I suppose in theory someone may want to call some function such as a stand-alone esp_mp_mul()
operation, but I think that would be rare, if ever. We could address that in a future request. I've flagged everything in esp32-crypt.h
as WOLFSSL_LOCAL
since the intention is really for acceleration to be used only in wolfcrypt.
This will be included when I push my pending changes.
wolfcrypt/src/aes.c
Outdated
@@ -2367,8 +2414,14 @@ static WARN_UNUSED_RESULT WC_INLINE word32 PreFetchTd4(void) | |||
#endif | |||
|
|||
/* Software AES - ECB Decrypt */ | |||
#ifdef NEED_AES_HW_FALLBACK |
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.
can we follow the pattern of AESNI and FREESCALE_LTC here where only the existing NEED_AES_TABLES macro is needed
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.
I'm not sure I understand. Do you mean to use NEED_AES_TABLES
instead of NEED_AES_HW_FALLBACK
?
Let me clarify my intentions. The issue here is only with the ESP32-S3, and not the classic ESP32.
The classic ESP32 has all AES algos implemented in hardware. I see how that could follow the FREESCALE_LTC pattern.
However, the ESP32-S3 only has partial AES hardware acceleration implementation. It is missing AES-192. See #6375
Thus for the ESP32, it is all or nothing: either we need AES tables for SW, or not when we use HW.
For the ESP32-S3, we may need tables. So depending on the specific AES (e.g. AES192 enabled), we need both a hardware and software algo enabled. The NEED_AES_HW_FALLBACK
is used to indicate this.
For example, here we check NEED_AES_HW_FALLBACK
and if enabled, we check if aes
parameter is a supported key length for hardware, otherwise fall back to software:
#ifdef NEED_AES_HW_FALLBACK
if (wc_esp32AesSupportedKeyLen(aes)) {
ret = wc_esp32AesDecrypt(aes, inBlock, outBlock);
}
else {
ret = wc_AesDecrypt_SW(aes, inBlock, outBlock);
}
#else
/* if we don't need fallback, always use HW */
ret = wc_esp32AesDecrypt(aes, inBlock, outBlock);
#endif
If that was not clear, perhaps I could use a more intuitive name?
It could be I completely misunderstood. :) If so, please clarify. Thanks
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.
Am hoping we can get away from having the ifdef around the function name. Either by having the esp function calls instead be in the wc_AesDecrypt() function or by following the wc_AesGcmDecrypt of calling a AES_GCM_decrypt_C function like with AESNI.
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.
ok, I've pushed some of the requested changes, but the AES update was a bit more complicated.
My WIP is over on my dev branch. I did get rid of the ifdef
around the function name, for example here:
#if defined(WOLFSSL_ESPIDF) && defined(NEED_AESCBC_HW_FALLBACK)
if (wc_esp32AesSupportedKeyLen(aes)) {
ESP_LOGV(TAG, "wc_AesCbcEncrypt calling wc_esp32AesCbcEncrypt");
return wc_esp32AesCbcEncrypt(aes, out, in, sz);
}
else {
ESP_LOGW(TAG, "wc_AesCbcEncrypt unsupported keylen = %d, "
"falling back to SW.", aes->keylen);
}
#endif
software algo follows...
It needs some more polishing and testing. I expect to be able to update this PR in the coming week. cc: @PaulMartinsen
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.
Hi @JacobBarthelmeh, I pushed the updates that should address the last of the code review comments. In particular I'm no longer renaming function for software fallback.
Let me know how things are looking. Thank you.
|
||
ESP_EM__PRE_DPORT_WRITE; | ||
DPORT_INTERRUPT_DISABLE(); | ||
for (int i=0; i < wordSz; i++) { |
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.
Don't declare int i
in for. Please put declaration at top of function.
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.
Good catch. Done.
#ifdef DEBUG_WOLFSSL | ||
MATH_INT_T rinv2[1]; | ||
MATH_INT_T M2[1]; | ||
mp_copy(M, M2); /* copy (src = M) to (dst = M2) */ |
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.
Mixed declaration and code. Please put all variable declarations at top.
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.
moved to top.
/* Y (left-extend) | ||
* Accelerator supports large-number multiplication with only | ||
* four operand lengths of N ∈ {512, 1024, 1536, 2048} */ | ||
int left_pad_offset = maxWords_sz << 2; |
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.
Mixed declaration.
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.
done
name_A, name_B); | ||
} | ||
else { | ||
// esp_show_mp(name_A, A); |
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.
Remove or use #if 0
or C style comment /* */
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.
wrapped in DEBUG_WOLFSSL
. This condition is unlikely to occur unless there's a math problem; will be handy for future HW work, e.g. RISC-V.
wolfcrypt/src/tfm.c
Outdated
@@ -2158,6 +2251,17 @@ static int _fp_exptmod_ct(fp_int * G, fp_int * X, int digits, fp_int * P, | |||
#ifdef WOLFSSL_SMALL_STACK | |||
XFREE(R, NULL, DYNAMIC_TYPE_BIGINT); | |||
#endif | |||
|
|||
#if defined(DEBUG_WOLFSSL) && defined(WOLFSSL_ESPIDF) | |||
/* TODO consider moving / removing this */ |
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.
Perhaps remove this? Not adding much value.
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.
Agreed. I address this now in the esp_clean_result().
Hi @dgarske thanks for the code review; I've applied the suggested changes, addressed a few similar instances, and applied other minor polishing changes. Tested on ESP32, ESP32-S3 and ESP32-C6 for ESP-IDF v5.1 Note there's unresolved problem, unrelated to this PR, with RISC-V timers (e.g. ESP32-C3/ESP32-C6) for the benchmark app. For some as-yet unknown reason, the compiler cannot seem to find the file for |
retest this please |
@@ -343,7 +356,7 @@ int ShowExtendedSystemInfo(void) | |||
return 0; | |||
} | |||
|
|||
int esp_ShowExtendedSystemInfo() | |||
WOLFSSL_LOCAL int esp_ShowExtendedSystemInfo() |
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.
WOLFSSL_LOCAL in the .c does nothing. WOLFSSL_LOCAL sets a hidden visibility but should only be used in the header. If you don't want the function to be used outside the .c then make it static.
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.
Also please use (void) in the declaration.
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.
Ok. I added those only as a reminder that the WOLFSSL_LOCAL
is in the header for the respective function.
I will remove them in future PR's and add a comment reminder instead about the visibility decorator tag.
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.
It doesn't harm anything and we aren't 100% elsewhere so I went ahead and accepted it as-is.
Description
This update improves multi-chip support of Espressif ESP32 devices.
There are significant improvements to the hardware acceleration for the ESP32 and ESP32-S3. Provides more graceful fallback to SW for other chipsets.
Adds ESP32-S3 conditional SW fallback for AES192 (not supported in HW), fixing #6375.
Fixes #6028 regarding negative big num values with OpenSSL.
Most importantly: Fixes #6205 and renders #6286 moot as this PR now properly supports all key lengths.
The root cause of the RSA signature problem was HW math errors. I have new math tests coming up in a subsequent PR. The tests would cause the wolfcrypt test to fail with current code in production on the ESP32.
Part of the math problem was the lack of errata mitigation for the ESP32. See esp32_errata_en.pdf.
There was also a problem with TFM handling of Espressif HW results as related to zero padding and lengths. TFM is not changed, but the Espressif results are cleaned up to address #6382 and #6380.
This PR also refactors and cleans up the code contributed from #5950.
There's full support for ESP-IDF v5.0 as noted in #5909.
New debugging code has been added to more readily observe operand overwriting as noted in #6359. This is an expected condition in most (or all?) situation. For example for
Z = X x Y
:Z = X x Z
. (the pointer toY
is the same asZ
). Comments added to be aware when falling back to SW after HW function called.The
fp
bit num references were all updated tomp
in preparation for using HW acceleration outside of TFM.The Montgomery Math initialization for HW was cleaned up and implemented to be multi-chip aware.
The
SINGLE_THREADED
or multi-threaded environment has been improved for HW math.Math HW acceleration code in
esp32_mp.c
was refactored to have fewer in-functionreturn
statements.A new "hard bottom" for some math functions on the ESP32 classic was implemented. This is turned on by default. See
ESP_PROHIBIT_SMALL_X
. Although unlikely to be used in the real world cryptography, some small operands (e.g. <= 8 bits) were observed to not compute properly in HW. There's a conditional hard bottom for Y operands, disabled by default.RSA Key sizes up to 4096 bits are now supported. HW multiplication math falls back to software for keys larger than over 2048 bits.
Fixed the used operand allowed lengths for the ESP32, limited to four operand lengths of N ∈ {512, 1024, 1536, 2048}.
Individual math HW acceleration can now be turned on and off with
NO_WOLFSSL_ESP32_CRYPT_RSA_PRI_MP_MUL
',NO_WOLFSSL_ESP32_CRYPT_RSA_PRI_EXPTMOD
, andNO_WOLFSSL_ESP32_CRYPT_RSA_PRI_MULMOD
. Seeesp32-crypt.h
header comments.Fixed proper handling of negative numbers when
WOLFSSL_SP_INT_NEGATIVE
orUSE_FAST_MATH
is detected.The
HW_MATH_ENABLED
can be used to determine is any of the hardware math functions have been enabled.Hardware usage metrics can optionally be observed by turning on
WOLFSSL_HW_METRICS
.A variety of other cleanup and improvement updates were made.
This is certainly not the final-and-done version, but is a good release candidate and foundation for future improvements.
See #6234 for Espressif Roadmap and related updates.
Fixes zd# n/a
Testing
How did you test?
Tested on a dozen different ESP32, ESP32-S3, ESP32-C2, ESP32-C3 devices. The wolfcrypt tests were modified to run continuously in a loop.
Of interest to @PaulMartinsen and others interested in key generation and signing is the key test app. The ESP32 generates a certificate, and the inspect_test.sh script signs and inspects it for validity. See the various key lengths in user_settings.h.
Run the ESP32
key_test
app, and copy the output to the output/test_request.crt file. See inspect_test.sh#L3Note that for key sizes larger than
2048
theESP_HW_RSAMAX_BIT
andESP_HW_MULTI_RSAMAX_BITS
values will need to be adjusted. (and possiblyESP_RSA_TIMEOUT_CNT
and PSRAM settings).Most of the time seems to be finding a big prime number. Once found, the math is computed rather quickly, even when falling back to wolfcrypt SW math for multiplication over 64 words long.
Note that the HW acclerator only accepts math multiplication operands up to 64 words, so key sizes over 2048 will not fully benefit from from all the RSA HW accleration.
When successful, the output should look something like this:
Also tested with:
See #6234 for Espressif Roadmap and related updates.
Checklist