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.

Sign up for GitHub

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

Jump to bottom

Add AES with armv8 crypto extension #6895

Merged
merged 24 commits into from
Feb 28, 2023
Merged

Add AES with armv8 crypto extension #6895

Show file tree
Hide file tree
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 Jan 10, 2023
4923131
Add empty aesce files
yuhaoth Jan 10, 2023
b95c776
Add linux runtime detection
yuhaoth Jan 10, 2023
3f2fb71
Add key expansion for encrypt
yuhaoth Jan 10, 2023
e096da1
Add inverse key function
yuhaoth Jan 10, 2023
2bb3d81
Add en(de)crypt routine
yuhaoth Jan 10, 2023
e51eddc
disable aesce when ASM not available
yuhaoth Jan 11, 2023
32f977e
Add arm64 tests on travis ci
yuhaoth Jan 11, 2023
e908c57
Disable clang tests
yuhaoth Jan 13, 2023
b3b85dd
Disable macro conflict check
yuhaoth Jan 30, 2023
837e9cf
fix wrong typo
yuhaoth Feb 8, 2023
b2783f6
fix typo issue
yuhaoth Feb 13, 2023
751e76b
Replace `crypto engine` with `crypto extension`
yuhaoth Feb 16, 2023
c8bcdc8
fix various issues
yuhaoth Feb 21, 2023
330e6ae
Add document about runtime detection of AESCE
yuhaoth Feb 21, 2023
97b31d8
Revert "Disable clang tests"
yuhaoth Feb 21, 2023
baae401
merge setkey_enc* functions
yuhaoth Feb 21, 2023
3304c20
Improve readabilities
yuhaoth Feb 22, 2023
fac5a54
fix code style issues
yuhaoth Feb 23, 2023
947bf96
Improve readability of expansion size
yuhaoth Feb 23, 2023
ba1e78f
fix code style and comment issues
yuhaoth Feb 24, 2023
aa18c4b
Add comments about travis test.
yuhaoth Feb 24, 2023
c66deda
Add explanation for aesce limitation
yuhaoth Feb 24, 2023
608e109
Improve comment about conflicts between aesce and sha512-crypto
yuhaoth Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,58 @@ jobs:
- sleep 5
- scripts/windows_msbuild.bat v141 # Visual Studio 2017

- name: full configuration on arm64
os: linux
dist: focal
arch: arm64
addons:
apt:
packages:
- gcc
script:
# Do a manual build+test sequence rather than using all.sh, because
# there's no all.sh component that does what we want. We should set
# CFLAGS for arm64 host CC.
- scripts/config.py full
- scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT
- scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY
- scripts/config.py unset MBEDTLS_SHA256_USE_A64_CRYPTO_IF_PRESENT
- scripts/config.py unset MBEDTLS_SHA256_USE_A64_CRYPTO_ONLY
- make generated_files
- make CFLAGS='-march=armv8-a+crypto -O3 -Werror -fsanitize=address,undefined -fno-sanitize-recover=all' LDFLAGS='-Werror -fsanitize=address,undefined -fno-sanitize-recover=all'
- make test
- programs/test/selftest
- tests/scripts/test_psa_constant_names.py
# Modern OpenSSL does not support fixed ECDH or null ciphers.
- tests/compat.sh -p OpenSSL -e 'NULL\|ECDH_'
- tests/scripts/travis-log-failure.sh
- tests/context-info.sh

- name: full configuration(GnuTLS compat tests) on arm64
os: linux
dist: focal
arch: arm64
addons:
apt:
packages:
- clang
- gnutls-bin
script:
# Do a manual build+test sequence rather than using all.sh, because
# there's no all.sh component that does what we want. We should set
# CFLAGS for arm64 host CC.
- scripts/config.py full
- scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT
- scripts/config.py unset MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY
- scripts/config.py unset MBEDTLS_SHA256_USE_A64_CRYPTO_IF_PRESENT
- scripts/config.py unset MBEDTLS_SHA256_USE_A64_CRYPTO_ONLY
- make generated_files
- make CC=clang CFLAGS='-march=armv8-a+crypto -O3 -Werror -fsanitize=address,undefined -fno-sanitize-recover=all' LDFLAGS='-Werror -fsanitize=address,undefined -fno-sanitize-recover=all'
# GnuTLS supports CAMELLIA but compat.sh doesn't properly enable it.
- tests/compat.sh -p GnuTLS -e 'CAMELLIA'
- tests/scripts/travis-log-failure.sh
- tests/context-info.sh

after_failure:
- tests/scripts/travis-log-failure.sh

Expand Down
4 changes: 4 additions & 0 deletions include/mbedtls/check_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@
#error "MBEDTLS_AESNI_C defined, but not all prerequisites"
#endif

#if defined(MBEDTLS_AESCE_C) && !defined(MBEDTLS_HAVE_ASM)
#error "MBEDTLS_AESCE_C defined, but not all prerequisites"
#endif

#if defined(MBEDTLS_CTR_DRBG_C) && !defined(MBEDTLS_AES_C)
#error "MBEDTLS_CTR_DRBG_C defined, but not all prerequisites"
#endif
Expand Down
26 changes: 26 additions & 0 deletions include/mbedtls/mbedtls_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,6 +2065,32 @@
*/
#define MBEDTLS_AESNI_C

/**
* \def MBEDTLS_AESCE_C
*
* Enable AES crypto extension support on Arm64.
*
* Module: library/aesce.c
* Caller: library/aes.c
*
* Requires: MBEDTLS_HAVE_ASM, MBEDTLS_AES_C
*
* \note The code uses Neon intrinsics, so \c CFLAGS must be set to a minimum
* of \c -march=armv8-a+crypto .
*
* \warning `MBEDTLS_SHA512_USE_A64_CRYPTO_*` should be disabled when enabled
Copy link
Contributor

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 have MBEDTLS_SHA512_USE_A64_CRYPTO_xxx? Even if it's temporary, with a /* XXX temporary */ coment

Copy link
Contributor

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.

Copy link
Contributor

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"

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

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 and MBEDTLS_AES_USE_A64_CRYPTO_IF_PRESENT can be enabled together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY and MBEDTLS_AES_USE_A64_CRYPTO_IF_PRESENT can be enabled together

That's not what the warning currently says

Copy link
Contributor

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

 * \warning If the target architecture is set to something that includes the
            SHA3 feature (e.g. `-march=armv8.2-a+sha3`), for example because
            `MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT` is desired, some
            compilers generate code for `MBEDTLS_AESCE_C` that includes
            instructions only present with the (optional) SHA3 feature. This
            will lead to an undefined instruction exception if the code is run
            on a CPU without that feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change some compilers to compilers. The optimization is reasonable if we tell compiler sha3 is supported.

* because unexpected instruction will be generated in AESCE module.
* `MBEDTLS_SHA512_USE_A64_CRYPTO_*` requires \c -march=armv8.2-a+sha3,
* compiler optimizes the code with `eor3` that is part of sha3
* extension and unexpected in AESCE.
*
* \warning Runtime detection only works on linux. For non-linux operation
* system, crypto extension MUST be supported by CPU.
*
* This module adds support for the AES crypto instructions on Arm64
*/
#define MBEDTLS_AESCE_C

/**
* \def MBEDTLS_AES_C
*
Expand Down
1 change: 1 addition & 0 deletions library/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ endif()
set(src_crypto
aes.c
aesni.c
aesce.c
aria.c
asn1parse.c
asn1write.c
Expand Down
1 change: 1 addition & 0 deletions library/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ endif
OBJS_CRYPTO= \
aes.o \
aesni.o \
aesce.o \
aria.o \
asn1parse.o \
asn1write.o \
Expand Down
25 changes: 25 additions & 0 deletions library/aes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
#if defined(MBEDTLS_AESNI_C)
#include "aesni.h"
#endif
#if defined(MBEDTLS_AESCE_C)
#include "aesce.h"
#endif

#include "mbedtls/platform.h"

Expand Down Expand Up @@ -544,6 +547,12 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
}
#endif

#if defined(MBEDTLS_AESCE_C) && defined(MBEDTLS_HAVE_ARM64)
if (mbedtls_aesce_has_support()) {
return mbedtls_aesce_setkey_enc((unsigned char *) RK, key, keybits);
}
#endif

for (i = 0; i < (keybits >> 5); i++) {
RK[i] = MBEDTLS_GET_UINT32_LE(key, i << 2);
}
Expand Down Expand Up @@ -652,6 +661,16 @@ int mbedtls_aes_setkey_dec(mbedtls_aes_context *ctx, const unsigned char *key,
}
#endif

#if defined(MBEDTLS_AESCE_C) && defined(MBEDTLS_HAVE_ARM64)
if (mbedtls_aesce_has_support()) {
mbedtls_aesce_inverse_key(
(unsigned char *) RK,
(const unsigned char *) (cty.buf + cty.rk_offset),
ctx->nr);
goto exit;
}
#endif

SK = cty.buf + cty.rk_offset + cty.nr * 4;

*RK++ = *SK++;
Expand Down Expand Up @@ -944,6 +963,12 @@ int mbedtls_aes_crypt_ecb(mbedtls_aes_context *ctx,
}
#endif

#if defined(MBEDTLS_AESCE_C) && defined(MBEDTLS_HAVE_ARM64)
if (mbedtls_aesce_has_support()) {
return mbedtls_aesce_crypt_ecb(ctx, mode, input, output);
}
#endif

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
if (aes_padlock_ace > 0) {
if (mbedtls_padlock_xcryptecb(ctx, mode, input, output) == 0) {
Expand Down
Loading