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

AES: Add accelerator only mode #7384

Merged
merged 56 commits into from
Aug 18, 2023
Merged

AES: Add accelerator only mode #7384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
0d4f4e5
Add option to disable built-in aes implementation.
yuhaoth Mar 31, 2023
d767cc4
Add accelerator only tests.
yuhaoth Mar 31, 2023
2f26a59
Add std output information for AESCE in gcm
yuhaoth Mar 31, 2023
315fd30
Rename plain c disable option
yuhaoth Apr 18, 2023
4d030f3
Add check for no aes implementation provided
yuhaoth Apr 18, 2023
1b3ab36
Update comments
yuhaoth Apr 18, 2023
3fcf2b5
Rename HAS_NO_PLAIN_C to DONT_USE_SOFTWARE_CRYPTO
yuhaoth Apr 18, 2023
8840a8c
fix wrong checks
yuhaoth Apr 19, 2023
3660623
Rename plain c option and update comments
yuhaoth Apr 19, 2023
d76ded0
fix various issues
yuhaoth Apr 19, 2023
4dfbb2e
add changelog entry
yuhaoth Apr 23, 2023
02b1519
move accelerator checks to `aes.c`
yuhaoth Apr 23, 2023
9e3e3dd
Fix code-style too-long line fail
yuhaoth Apr 24, 2023
e77c4d9
Mention the crash risk without runtime detection
yuhaoth Apr 24, 2023
6943681
Improve error message and documents
yuhaoth Apr 25, 2023
1414029
improve document about hardware only
yuhaoth Aug 1, 2023
69dd441
Remove test_aes_*
yuhaoth Aug 2, 2023
1221a31
Run aes tests only for test_aesni
yuhaoth Aug 3, 2023
17a9d2e
Add MBEDTLS_AES_USE_HADWARE_ONLY for test_aesni
yuhaoth Aug 3, 2023
8a599c0
Add aesni only test
yuhaoth Aug 3, 2023
193cbc0
Add aesce build test
yuhaoth Aug 3, 2023
c935aa6
Add via padlock build test
yuhaoth Aug 3, 2023
2700ef6
Add aesce test string filter
yuhaoth Aug 3, 2023
29c91ba
fix unreachable code warnings
yuhaoth Aug 4, 2023
b241db3
remove padlock only mode
yuhaoth Aug 4, 2023
fce351d
improve platform relative check
yuhaoth Aug 4, 2023
9c0b7d1
Remove unnecessary name check tag
yuhaoth Aug 4, 2023
7802f65
Add negative test for aesni only
yuhaoth Aug 7, 2023
5fcdd6a
remove unnecessary definition
yuhaoth Aug 7, 2023
c4508c0
improve error message and config check for padlock
yuhaoth Aug 8, 2023
a7de78d
improve test
yuhaoth Aug 8, 2023
76a51b9
replace strings command with grep
yuhaoth Aug 8, 2023
ba42b07
Remove asm check for aarch64 aesce
yuhaoth Aug 10, 2023
13696bb
improve check config option for i386
yuhaoth Aug 10, 2023
8189f32
improve aesni check for x86_64
yuhaoth Aug 10, 2023
240bb11
Add gnu check for aseni assembly code
yuhaoth Aug 11, 2023
e62ff09
Restore aesni for i386
yuhaoth Aug 16, 2023
cc068ae
fix `-Werror=return-type` when runtime detection enabled and plain c …
yuhaoth Aug 16, 2023
c628486
enable runtime detection when padlock enabled and plain c disabled
yuhaoth Aug 16, 2023
b6d39c2
Add aesni test for i386
yuhaoth Aug 16, 2023
506759f
fix build fail for via padlock test
yuhaoth Aug 16, 2023
3ce0398
Add compiler cflags error message
yuhaoth Aug 16, 2023
516cf27
fix msvc build fail on i386 target
yuhaoth Aug 16, 2023
bdd96b9
disable aesni for componets without cpu modifiers
yuhaoth Aug 16, 2023
35b59d7
exclude arm64ec mode for aesni
yuhaoth Aug 17, 2023
2319af0
Change the order of runtime detection
yuhaoth Aug 17, 2023
9e62862
Add via padlock detection macro
yuhaoth Aug 17, 2023
1b4c7ed
add hardware only check for padlock
yuhaoth Aug 17, 2023
f258d17
remove aesni + padlock - plain c tests
yuhaoth Aug 17, 2023
e9c6b53
remove return-type when runtime detection enabled without plain c
yuhaoth Aug 17, 2023
6c6b9f6
Change document to match real status
yuhaoth Aug 17, 2023
3a0f044
improve readability
yuhaoth Aug 17, 2023
9608447
replace padlock_c with padlock_have_code
yuhaoth Aug 17, 2023
372f7a0
Add missing check
yuhaoth Aug 18, 2023
61fc5ed
improve readability of error message
yuhaoth Aug 18, 2023
0a6272d
revert padlock from aesni module
yuhaoth Aug 18, 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
24 changes: 24 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,30 @@ jobs:
- tests/scripts/travis-log-failure.sh
- tests/context-info.sh

- name: Arm64 accelerators tests on arm64 host
os: linux
dist: focal
arch: arm64
addons:
apt:
packages:
- gcc
script:
# Do a manual build+test sequence rather than using all.sh.
#
# This is arm64 host only test for no runtime detection case. Internal
# and Open CI do not include Arm64 host, and they check if components
# are be tested. As result, it will always fail on `pre-test-check` in
# them.
- scripts/config.py unset MBEDTLS_AESNI_C
- scripts/config.py unset MBEDTLS_PADLOCK_C
- scripts/config.py set MBEDTLS_AESCE_C
- scripts/config.py set MBEDTLS_AES_USE_HARDWARE_ONLY
- make generated_files
- make
- programs/test/selftest aes | grep "using AESCE"
- tests/context-info.sh

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

Expand Down
6 changes: 6 additions & 0 deletions ChangeLog.d/add-aes-hardware-only-option.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Features
* New configuration option MBEDTLS_AES_USE_HARDWARE_ONLY introduced. When
using CPU-accelerated AES (e.g., Arm Crypto Extensions), this option
disables the plain C implementation and the run-time detection for the
CPU feature, which reduces code size and avoids the vulnerability of the
plain C implementation.
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If the ChangeLog is information for library users, this doesn't really tell me whether I need it or not. Should we add something like

Enable if - but only if - you know the library will only be used on systems with CPU-accelerated AES

And I wonder if a sentence like that might be useful in the config file too?

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 the documentation in mbedtls_config is pretty clear now (it says it will crash at runtime if you don't have CPU support). Anyone who uses this feature will read that for the full details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with Dave. The information should be put at document of option.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just that, reading both the ChangeLog entry and the config file entry after being away from it for a few days, neither gives me a sense of when I might want to enable it. The ChangeLog entry says what happens, but doesn't give reasons why enabling it might be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely "reduces code size and avoids the vulnerability" tells users why they might want it?

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

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

#if defined(MBEDTLS_PEM_PARSE_C) && !defined(MBEDTLS_BASE64_C)
#error "MBEDTLS_PEM_PARSE_C defined, but not all prerequisites"
#endif
Expand Down
14 changes: 14 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 @@ -4006,4 +4006,18 @@
*/
//#define MBEDTLS_ECP_WITH_MPI_UINT

/*
* Disable plain C implementation for AES.
*
* When the plain C implementation is enabled, and an implementation using a
* special CPU feature (such as MBEDTLS_AESCE_C) is also enabled, runtime
* detection will be used to select between them.
*
* If only one implementation is present, runtime detection will not be used.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify that attempts to use AES will invoke the accelerator code, which will cause an instruction fault if the code is running on a platform that lacks the acceleration instructions.

* This configuration will crash at runtime if running on a CPU without the
* necessary features. It will not build unless at least one of MBEDTLS_AESCE_C
* and/or MBEDTLS_AESNI_C is enabled & present in the build.
*/
daverodgman marked this conversation as resolved.
Show resolved Hide resolved
//#define MBEDTLS_AES_USE_HARDWARE_ONLY

daverodgman marked this conversation as resolved.
Show resolved Hide resolved
/** \} name SECTION: Module configuration options */
86 changes: 64 additions & 22 deletions library/aes.c
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to auto-enable MBEDTLS_AES_[EN|DE]CRYPT_ALT to remove the plain C implementations when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. MBEDTLS_AES_[EN|DE]CRYPT_ALT are for user provided functions. If it is provided, MBEDTLS_AES_USE_HARDWARE_ONLY will not work.

Copy link
Contributor

Choose a reason for hiding this comment

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

When MBEDTLS_AES_[EN|DE]CRYPT_ALT are enabled, user need to provide their own implementations for mbedtls_internal_aes_decrypt and mbedtls_internal_aes_encrypt, so MBEDTLS_AES_USE_HARDWARE_ONLY would not be affected.

What I want to ask is, do we need to eliminate the plain C implementation when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled so that both symbols won't be built into 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.

What I want to ask is, do we need to eliminate the plain C implementation when MBEDTLS_AES_USE_HARDWARE_ONLY is enabled so that both symbols won't be built into the library.

Yes.

The first name of MBEDTLS_AES_USE_HARDWARE_ONLY is MBEDTL_AES_DISABLE_PLAIN_C . :) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I think it should just remove plain C . MBEDTLS_AES_[EN|DE]CRYPT_ALT will remove both hardware and software built-in aes

Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,36 @@
#include "mbedtls/platform.h"
#include "mbedtls/platform_util.h"
#include "mbedtls/error.h"

#if defined(__aarch64__)
#if !defined(MBEDTLS_AESCE_C) && defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
#error "MBEDTLS_AES_USE_HARDWARE_ONLY defined, but not all prerequisites"
#endif
lpy4105 marked this conversation as resolved.
Show resolved Hide resolved
#endif

#if defined(__amd64__) || defined(__x86_64__) || \
((defined(_M_X64) || defined(_M_AMD64)) && !defined(_M_ARM64EC))
#if !defined(MBEDTLS_AESNI_C) && defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
#error "MBEDTLS_AES_USE_HARDWARE_ONLY defined, but not all prerequisites"
#endif
#endif

#if defined(__i386__) || defined(_M_IX86)
#if defined(MBEDTLS_AES_USE_HARDWARE_ONLY) && !defined(MBEDTLS_AESNI_C)
#error "MBEDTLS_AES_USE_HARDWARE_ONLY defined, but not all prerequisites"
daverodgman marked this conversation as resolved.
Show resolved Hide resolved
#endif

#if defined(MBEDTLS_PADLOCK_C)
#if !defined(MBEDTLS_HAVE_ASM)
#error "MBEDTLS_PADLOCK_C defined, but not all prerequisites"
#endif
#if defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
#error "MBEDTLS_AES_USE_HARDWARE_ONLY cannot be defined when " \
"MBEDTLS_PADLOCK_C is set"
#endif
#endif
#endif

#if defined(MBEDTLS_PADLOCK_C)
#include "padlock.h"
#endif
Expand All @@ -47,7 +77,7 @@

#if !defined(MBEDTLS_AES_ALT)

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)
static int aes_padlock_ace = -1;
#endif

Expand Down Expand Up @@ -542,7 +572,7 @@ void mbedtls_aes_xts_free(mbedtls_aes_xts_context *ctx)
* Note that the offset is in units of elements of buf, i.e. 32-bit words,
* i.e. an offset of 1 means 4 bytes and so on.
*/
#if (defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)) || \
#if (defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)) || \
(defined(MBEDTLS_AESNI_C) && MBEDTLS_AESNI_HAVE_CODE == 2)
#define MAY_NEED_TO_ALIGN
#endif
Expand All @@ -554,7 +584,7 @@ static unsigned mbedtls_aes_rk_offset(uint32_t *buf)
#if defined(MAY_NEED_TO_ALIGN)
int align_16_bytes = 0;

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)
if (aes_padlock_ace == -1) {
aes_padlock_ace = mbedtls_padlock_has_support(MBEDTLS_PADLOCK_ACE);
}
Expand Down Expand Up @@ -595,7 +625,6 @@ static unsigned mbedtls_aes_rk_offset(uint32_t *buf)
int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits)
{
unsigned int i;
uint32_t *RK;

switch (keybits) {
Expand Down Expand Up @@ -629,14 +658,15 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
}
#endif

for (i = 0; i < (keybits >> 5); i++) {
#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
for (unsigned int i = 0; i < (keybits >> 5); i++) {
RK[i] = MBEDTLS_GET_UINT32_LE(key, i << 2);
}

switch (ctx->nr) {
case 10:

for (i = 0; i < 10; i++, RK += 4) {
for (unsigned int i = 0; i < 10; i++, RK += 4) {
RK[4] = RK[0] ^ RCON[i] ^
((uint32_t) FSb[MBEDTLS_BYTE_1(RK[3])]) ^
((uint32_t) FSb[MBEDTLS_BYTE_2(RK[3])] << 8) ^
Expand All @@ -652,7 +682,7 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
#if !defined(MBEDTLS_AES_ONLY_128_BIT_KEY_LENGTH)
case 12:

for (i = 0; i < 8; i++, RK += 6) {
for (unsigned int i = 0; i < 8; i++, RK += 6) {
RK[6] = RK[0] ^ RCON[i] ^
((uint32_t) FSb[MBEDTLS_BYTE_1(RK[5])]) ^
((uint32_t) FSb[MBEDTLS_BYTE_2(RK[5])] << 8) ^
Expand All @@ -669,7 +699,7 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,

case 14:

for (i = 0; i < 7; i++, RK += 8) {
for (unsigned int i = 0; i < 7; i++, RK += 8) {
RK[8] = RK[0] ^ RCON[i] ^
((uint32_t) FSb[MBEDTLS_BYTE_1(RK[7])]) ^
((uint32_t) FSb[MBEDTLS_BYTE_2(RK[7])] << 8) ^
Expand All @@ -695,6 +725,7 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
}

return 0;
#endif /* !MBEDTLS_AES_USE_HARDWARE_ONLY */
}
#endif /* !MBEDTLS_AES_SETKEY_ENC_ALT */

Expand All @@ -705,10 +736,13 @@ int mbedtls_aes_setkey_enc(mbedtls_aes_context *ctx, const unsigned char *key,
int mbedtls_aes_setkey_dec(mbedtls_aes_context *ctx, const unsigned char *key,
unsigned int keybits)
{
int i, j, ret;
#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
uint32_t *SK;
#endif
int ret;
mbedtls_aes_context cty;
uint32_t *RK;
uint32_t *SK;


mbedtls_aes_init(&cty);

Expand Down Expand Up @@ -740,15 +774,16 @@ int mbedtls_aes_setkey_dec(mbedtls_aes_context *ctx, const unsigned char *key,
}
#endif

#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
SK = cty.buf + cty.rk_offset + cty.nr * 4;

*RK++ = *SK++;
*RK++ = *SK++;
*RK++ = *SK++;
*RK++ = *SK++;

for (i = ctx->nr - 1, SK -= 8; i > 0; i--, SK -= 8) {
for (j = 0; j < 4; j++, SK++) {
SK -= 8;
for (int i = ctx->nr - 1; i > 0; i--, SK -= 8) {
for (int j = 0; j < 4; j++, SK++) {
*RK++ = AES_RT0(FSb[MBEDTLS_BYTE_0(*SK)]) ^
AES_RT1(FSb[MBEDTLS_BYTE_1(*SK)]) ^
AES_RT2(FSb[MBEDTLS_BYTE_2(*SK)]) ^
Expand All @@ -760,7 +795,7 @@ int mbedtls_aes_setkey_dec(mbedtls_aes_context *ctx, const unsigned char *key,
*RK++ = *SK++;
*RK++ = *SK++;
*RK++ = *SK++;

#endif /* !MBEDTLS_AES_USE_HARDWARE_ONLY */
exit:
mbedtls_aes_free(&cty);

Expand Down Expand Up @@ -1062,17 +1097,20 @@ int mbedtls_aes_crypt_ecb(mbedtls_aes_context *ctx,
}
#endif

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)
if (aes_padlock_ace > 0) {
return mbedtls_padlock_xcryptecb(ctx, mode, input, output);
}
#endif

#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
if (mode == MBEDTLS_AES_ENCRYPT) {
return mbedtls_internal_aes_encrypt(ctx, input, output);
} else {
return mbedtls_internal_aes_decrypt(ctx, input, output);
}
#endif

}

#if defined(MBEDTLS_CIPHER_MODE_CBC)
Expand All @@ -1098,7 +1136,7 @@ int mbedtls_aes_crypt_cbc(mbedtls_aes_context *ctx,
return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH;
}

#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
#if defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)
if (aes_padlock_ace > 0) {
if (mbedtls_padlock_xcryptcbc(ctx, mode, length, iv, input, output) == 0) {
return 0;
Expand Down Expand Up @@ -1850,11 +1888,6 @@ int mbedtls_aes_self_test(int verbose)
#if defined(MBEDTLS_AES_ALT)
mbedtls_printf(" AES note: alternative implementation.\n");
#else /* MBEDTLS_AES_ALT */
#if defined(MBEDTLS_PADLOCK_C) && defined(MBEDTLS_HAVE_X86)
if (mbedtls_padlock_has_support(MBEDTLS_PADLOCK_ACE)) {
mbedtls_printf(" AES note: using VIA Padlock.\n");
} else
#endif
#if defined(MBEDTLS_AESNI_HAVE_CODE)
#if MBEDTLS_AESNI_HAVE_CODE == 1
mbedtls_printf(" AES note: AESNI code present (assembly implementation).\n");
Expand All @@ -1867,12 +1900,21 @@ int mbedtls_aes_self_test(int verbose)
mbedtls_printf(" AES note: using AESNI.\n");
} else
#endif
#if defined(MBEDTLS_VIA_PADLOCK_HAVE_CODE)
if (mbedtls_padlock_has_support(MBEDTLS_PADLOCK_ACE)) {
mbedtls_printf(" AES note: using VIA Padlock.\n");
} else
#endif
#if defined(MBEDTLS_AESCE_C) && defined(MBEDTLS_HAVE_ARM64)
if (mbedtls_aesce_has_support()) {
mbedtls_printf(" AES note: using AESCE.\n");
} else
#endif
mbedtls_printf(" AES note: built-in implementation.\n");
{
#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
mbedtls_printf(" AES note: built-in implementation.\n");
#endif
}
#endif /* MBEDTLS_AES_ALT */
}

Expand Down
2 changes: 2 additions & 0 deletions library/aesce.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@
#include <sys/auxv.h>
#endif

#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
/*
* AES instruction support detection routine
*/
Expand All @@ -113,6 +114,7 @@ int mbedtls_aesce_has_support(void)
return 1;
#endif
}
#endif

/* Single round of AESCE encryption */
#define AESCE_ENCRYPT_ROUND \
Expand Down
5 changes: 5 additions & 0 deletions library/aesce.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ extern "C" {
*
* \return 1 if CPU has support for the feature, 0 otherwise
*/
#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
int mbedtls_aesce_has_support(void);
#else
#define mbedtls_aesce_has_support() 1
#endif


/**
* \brief Internal AES-ECB block encryption and decryption
Expand Down
2 changes: 2 additions & 0 deletions library/aesni.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include <immintrin.h>
#endif

#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
/*
* AES-NI support detection routine
*/
Expand Down Expand Up @@ -68,6 +69,7 @@ int mbedtls_aesni_has_support(unsigned int what)

return (c & what) != 0;
}
#endif /* !MBEDTLS_AES_USE_HARDWARE_ONLY */

#if MBEDTLS_AESNI_HAVE_CODE == 2

Expand Down
26 changes: 21 additions & 5 deletions library/aesni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,20 @@
/* Can we do AESNI with inline assembly?
* (Only implemented with gas syntax, only for 64-bit.)
*/
#if defined(MBEDTLS_HAVE_ASM) && defined(__GNUC__) && \
daverodgman marked this conversation as resolved.
Show resolved Hide resolved
(defined(__amd64__) || defined(__x86_64__)) && \
!defined(MBEDTLS_HAVE_X86_64)
#if !defined(MBEDTLS_HAVE_X86_64) && \
(defined(__amd64__) || defined(__x86_64__) || \
defined(_M_X64) || defined(_M_AMD64)) && \
!defined(_M_ARM64EC)
#define MBEDTLS_HAVE_X86_64
#endif

#if defined(MBEDTLS_AESNI_C)
#if !defined(MBEDTLS_HAVE_X86) && \
(defined(__i386__) || defined(_M_IX86))
#define MBEDTLS_HAVE_X86
#endif

#if defined(MBEDTLS_AESNI_C) && \
(defined(MBEDTLS_HAVE_X86_64) || defined(MBEDTLS_HAVE_X86))

/* Can we do AESNI with intrinsics?
* (Only implemented with certain compilers, only for certain targets.)
Expand All @@ -67,8 +74,13 @@
* In the long run, we will likely remove the assembly implementation. */
#if defined(MBEDTLS_AESNI_HAVE_INTRINSICS)
#define MBEDTLS_AESNI_HAVE_CODE 2 // via intrinsics
#elif defined(MBEDTLS_HAVE_X86_64)
#elif defined(MBEDTLS_HAVE_ASM) && \
defined(__GNUC__) && defined(MBEDTLS_HAVE_X86_64)
#define MBEDTLS_AESNI_HAVE_CODE 1 // via assembly
#elif defined(__GNUC__)
# error "Must use `-mpclmul -msse2 -maes` for MBEDTLS_AESNI_C"
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 prefer add #pragma GCC target("pclmul,sse2,aes") in next PR to keep consistent with aesce.c,sha*.c. This should check compiler compatible, I do not think we should do that in this PR.

These checks should be moved to C file like other modules. And I do not think the module should be disabled silently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we still need assembly code ? With pragma, we can not cover assembly code in CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

We were planning to maybe remove the assembly code. It's still useful for older runtimes that don't have the intrinsics. The question is, do we still care about these old runtimes?

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also the assembly still gets used when compiling without the -m flags that enable them. So it does get tested on the CI, in every x86_64 test job except component_test_aesni and those with assembly disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With #pragma GCC target("pclmul,sse2,aes") , command options(-mno-ase -mno-sse -mno-pclmul) will be overwrite. in this case, assembly code won't be used.

It's still useful for older runtimes that don't have the intrinsics. The question is, do we still care about these old runtimes?

Yes. But my question is how old is it? I think we should check compilers and make the decision. I am looking for old compilers. I can not get gcc <4.6 binary and I am not sure old clang can work on my environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

gcc 4.6.1 is from 2011, 12 years ago. It is reasonable for development to require a compiler from the last 10 years.

We certainly don't need to go back that far for clang - in 2014 clang 3.5 couldn't build 95% of the Debian archive, and it wasn't until 2018 that it was used to build Firefox for Windows. I have a Docker environment for clang 6.0.1-14 on x86_64, which seems a reasonable oldest to support, although I would be happy with requiring an even more recent minimum version if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you share the environment?

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It was built from my base 20.04 Ubuntu aarch64 (runs in virtual environment on M1 Mac) environment with just

RUN apt-get update && \
    apt-get install -y clang-6.0

RUN echo "PATH=/usr/lib/llvm-6.0/bin:\$PATH" >> /etc/environ && \
    echo "PS1='clang-6 \$ '" >> /etc/environ

which makes me think it should be do-able on any Ubuntu 20.04

The system parts of my 20.04 setup are

FROM ubuntu:20.04

ENV DEBIAN_FRONTEND=noninteractive

RUN \
    apt-get update && \
    apt-get upgrade -y && \
    apt-get install -y ksh vim bsdmainutils htop && \
    apt-get install -y bc curl iputils-ping ssh-client sshfs sudo && \
    apt-get install -y apt-utils software-properties-common tzdata && \
    apt-get install -y --no-install-recommends \
        ca-certificates \
        mosquitto-clients \
        autoconf automake cmake dpkg-dev file git make patch \
        dirmngr gnupg2 lbzip2 wget xz-utils libtinfo5        \
        python3-pip python3-jinja2 python3-jsonschema

#else
#error "MBEDTLS_AESNI_C defined, but neither intrinsics nor assembly available"
#endif

#if defined(MBEDTLS_AESNI_HAVE_CODE)
Expand All @@ -88,7 +100,11 @@ extern "C" {
*
* \return 1 if CPU has support for the feature, 0 otherwise
*/
#if !defined(MBEDTLS_AES_USE_HARDWARE_ONLY)
int mbedtls_aesni_has_support(unsigned int what);
#else
#define mbedtls_aesni_has_support(what) 1
#endif

/**
* \brief Internal AES-NI AES-ECB block encryption and decryption
Expand Down
Loading