-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add AES with armv8 crypto extension #6895
Merged
gilles-peskine-arm
merged 24 commits into
Mbed-TLS:development
from
yuhaoth:pr/add-aes-with-armv8-crypto-extension
Feb 28, 2023
Merged
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
2fddfd7
Add AESCE confige options
yuhaoth 4923131
Add empty aesce files
yuhaoth b95c776
Add linux runtime detection
yuhaoth 3f2fb71
Add key expansion for encrypt
yuhaoth e096da1
Add inverse key function
yuhaoth 2bb3d81
Add en(de)crypt routine
yuhaoth e51eddc
disable aesce when ASM not available
yuhaoth 32f977e
Add arm64 tests on travis ci
yuhaoth e908c57
Disable clang tests
yuhaoth b3b85dd
Disable macro conflict check
yuhaoth 837e9cf
fix wrong typo
yuhaoth b2783f6
fix typo issue
yuhaoth 751e76b
Replace `crypto engine` with `crypto extension`
yuhaoth c8bcdc8
fix various issues
yuhaoth 330e6ae
Add document about runtime detection of AESCE
yuhaoth 97b31d8
Revert "Disable clang tests"
yuhaoth baae401
merge setkey_enc* functions
yuhaoth 3304c20
Improve readabilities
yuhaoth fac5a54
fix code style issues
yuhaoth 947bf96
Improve readability of expansion size
yuhaoth ba1e78f
fix code style and comment issues
yuhaoth aa18c4b
Add comments about travis test.
yuhaoth c66deda
Add explanation for aesce limitation
yuhaoth 608e109
Improve comment about conflicts between aesce and sha512-crypto
yuhaoth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ endif() | |
set(src_crypto | ||
aes.c | ||
aesni.c | ||
aesce.c | ||
aria.c | ||
asn1parse.c | ||
asn1write.c | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,6 +78,7 @@ endif | |
OBJS_CRYPTO= \ | ||
aes.o \ | ||
aesni.o \ | ||
aesce.o \ | ||
aria.o \ | ||
asn1parse.o \ | ||
asn1write.o \ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why not verify that if we have
MBEDTLS_AESCE_C
we don't haveMBEDTLS_SHA512_USE_A64_CRYPTO_xxx
? Even if it's temporary, with a/* XXX temporary */
comentThere 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.
They might or might not work depending on the compiler version and command line switches. Since some users might be happy with those options and able to get away with having them both. I think this could justify not enforcing it.
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.
Then shouldn't this text be more explanatory?
But actually, isn't this warning wrong? The issue is with setting compiler flags needed for
MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT
and running the resulting code on a system without the SHA3 crypto extension (because compiler optimisation uses the SHA3 feature set elsewhere) - or am I misinterpreting this warning (which would also suggest that it needs updating!)If the library is built with
MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY
, and the compiler flags set accordingly, that's not a problem, because "only" means "this will definitely be run on a system with the SHA3 crypto extension"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 problem is that if both are present then with -O3 a SHA3 instruction will be compiled into the AES code. If this runs on a platform with crypto extension but no SHA3 extension, the AES detection will think that we have support and then the code chokes on the SHA3 instruction.
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're saying the same thing - probably writing our comments at the same time!
I don't think this warning is the best way to express the potential issue to a user of the library
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.
Agree if
runtime detection
can be disabled at project level.For time being, we do not have an option to disable runtime detection of sha512 and AESCE together. In theory,
MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY
andMBEDTLS_AES_USE_A64_CRYPTO_IF_PRESENT
can be enabled together.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.
That's not what the warning currently says
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.
Unfortunately I can't find a better way of writing this warning than
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 change
some compilers
tocompilers
. The optimization is reasonable if we tell compilersha3
is supported.