AES: Add accelerator only mode #7384

yuhaoth · 2023-03-31T07:11:16Z

Description

Add new option(MBEDTLS_AES_USE_HARDWARE_ONLY ) to disable built-in AES implementation.

For time being, there are only two implementations in each known architecture. This PR just disable runtime detection to meet the requirement. When runtime detection was disabled and accelerator is enabled, built-in implementation become dead code, compiler will remove those dead code.

Fixes #7141

Gatekeeper checklist

changelog present
backport no, not in 2.28
tests provided

Notes for the submitter

Please refer to the contributing guidelines, especially the
checklist for PR contributors.

daverodgman · 2023-04-17T17:28:37Z

include/mbedtls/mbedtls_config.h

+/**
+ * Platform independent implementation for crypto algorithms.
+ */
+//#define MBEDTLS_AES_HAS_NO_BUILTIN /* Uncomment to disable built-in platform independent code of AES */


I find the name of this option confusing. Maybe MBEDTLS_AES_CPU_ACCELERATION_NO_RUNTIME_DETECTION would be more clear. It's not a great suggestion though, maybe a second reviewer has a better idea.

This name looks confuse you. This means no Pure-C code. ( platform independent code).
How about MBEDTLS_AES_HAS_NO_IND_ACCEL?

I rename it to MBEDTLS_AES_HAS_NO_PLAIN_C , is it better?

The problem with HAS_ is that it sounds like a statement (of fact) rather than an imperative (instruction).

In SHA-2, we've used MBEDTLS_SHAxxx_USE_A64_CRYPTO_ONLY, so we could use something like MBEDTLS_AES_USE_HARDWARE_CRYPTO_ONLY or MBEDTLS_AES_DONT_USE_SOFTWARE_CRYPTO

Renamed to MBEDTLS_AES_DONT_USE_SOFTWARE_CRYPTO .

Having positive variables is easier for users. But it introduces a problem with backward compatibility: if someone has an existing mbedtls_config.h with just

#define MBEDTLS_AES_C #define MBEDTLS_AESNI_C

then they should get the fallback. We can fiddle with options in build_info.h, but that means we'd have to automaticaly enable MBEDTLS_AES_PLAIN_C is MBEDTLS_AES_C is enabled. Then the user would have to #undef MBEDTLS_AES_PLAIN_C in their mbedtls_config.h if they don't want fallback. We currently have no pre-enabled configuration options, and I think it would be more confusing to start having one than it would be to have a negative option “no software AES”.

On naming: the suffix _C in Mbed TLS compile-time options means “a module providing a feature”. Please avoid option names where it doesn't mean that. Given the precedent of MBEDTLS_SHA256_USE_A64_CRYPTO_ONLY and MBEDTLS_SHA512_USE_A64_CRYPTO_ONLY, we should use similar names for AES. I suggest

MBEDTLS_AES_USE_HARDWARE_ONLY

(and really I'd suggest something similar for the SHA) since it makes perfect sense to enable both AESNI and AESCE if you want to deploy both on high-end x86 and high-end arm.

I like @gilles-peskine-arm 's suggestion. And it should be negative option to avoid backward compatibility issue.

Agreed with @gilles-peskine-arm that we really have to keep it a negative option. But I don't really like "HARDWARE" in the name, because this is a software implementation (that happens to use some special CPU instructions).

So I'm back to MBEDTLS_AES_NO_PLAINC as my suggestion. @gilles-peskine-arm suggested (in slack) MBEDTLS_AES_USE_ACCELERATION_ONLY. I'm OK with either. @tom-cosgrove-arm WDYT?

Sorry, but after thinking about this for a while, I think it really is HARDWARE.

A software implementation of AES is where code (C, assembler, Rust, JavaScript, whatever) does the calculations using registers and memory on a general-purpose CPU.

Hardware-accelerated AES, like AES-NI and the AArch64 crypto extensions, is where hardware does those calculations - and the CPU is hardware. Yes, software has to set up the input data, but it has to do that when handing off to a separate crypto accelerator. i.e. the algorithm is implemented within the hardware of the CPU, not the software of the code running on the CPU.

Therefore I think few people would be confused if we used the term HARDWARE here, but I think some might get confused if we said NO_PLAINC (we're using intrinsics, called by C, so there's a lot riding on the interpretation of "PLAIN" here)

Ah, the problem is that the check is in aesce.c:

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

but... aes.c has the following:

#if defined(MBEDTLS_AESCE_C) #include "aesce.h" #endif

so the error will never trigger. Presumably the checks should move into aes.c.

include/mbedtls/mbedtls_config.h

yuhaoth · 2023-04-18T05:32:29Z

Last CI got a known failure in time test. That is discussing in #7419

include/mbedtls/mbedtls_config.h

daverodgman

Some more comments on naming, and a question about the checks that have been added.

tests/scripts/all.sh

tom-cosgrove-arm · 2023-04-18T13:40:17Z

.travis.yml

+        - make generated_files
+        - make
+        - programs/test/selftest
+        - tests/scripts/travis-log-failure.sh


Is travis-log-failure.sh needed? It says:

# Purpose # # List the server and client logs on failed ssl-opt.sh and compat.sh tests.

but we're only running selftest here

tom-cosgrove-arm · 2023-04-18T13:58:16Z

library/padlock.h

@@ -41,13 +41,16 @@
 /* Some versions of ASan result in errors about not enough registers */
 #if defined(MBEDTLS_HAVE_ASM) && defined(__GNUC__) && defined(__i386__) && \
    !defined(MBEDTLS_HAVE_ASAN)
-


(v minor) it doesn't seem necessary to remove this blank line

library/aes.c

Signed-off-by: Jerry Yu <[email protected]>

lpy4105

LGTM, thanks!

gilles-peskine-arm · 2023-10-10T07:38:20Z

tests/scripts/all.sh

@@ -4637,6 +4747,7 @@ component_test_tls13_only_record_size_limit () {

 component_build_mingw () {
    msg "build: Windows cross build - mingw64, make (Link Library)" # ~ 30s
+    scripts/config.py unset MBEDTLS_AESNI_C # AESNI depends on cpu modifiers


Disabling AESNI here is a test regression. It's apparently hiding a product regression. When tests fail, we shouldn't disable them, we should fix the bug!

yuhaoth marked this pull request as draft March 31, 2023 07:11

yuhaoth added component-crypto Crypto primitives and low-level interfaces needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Mar 31, 2023

yuhaoth force-pushed the pr/add-aes-accelerator-only-mode branch 2 times, most recently from 30fa44a to c0fb3b9 Compare March 31, 2023 09:35

yuhaoth linked an issue Apr 3, 2023 that may be closed by this pull request

Improve runtime cpu feature detection #6921

Closed

yuhaoth marked this pull request as ready for review April 3, 2023 02:52

yuhaoth force-pushed the pr/add-aes-accelerator-only-mode branch from c0fb3b9 to edc9c70 Compare April 4, 2023 02:29

yuhaoth mentioned this pull request Apr 4, 2023

Add runtime detection module #7078

Draft

3 tasks

yuhaoth linked an issue Apr 6, 2023 that may be closed by this pull request

Add hardware only config options to AES armv8 acceleration #7141

Closed

yuhaoth mentioned this pull request Apr 6, 2023

Code Size: Hardware-only AES #7366

Closed

yuhaoth force-pushed the pr/add-aes-accelerator-only-mode branch from edc9c70 to 5bb3073 Compare April 10, 2023 02:31

daverodgman requested changes Apr 17, 2023

View reviewed changes

yuhaoth force-pushed the pr/add-aes-accelerator-only-mode branch from 2f94ed7 to 64c7867 Compare April 18, 2023 04:36

daverodgman reviewed Apr 18, 2023

View reviewed changes

include/mbedtls/mbedtls_config.h Show resolved Hide resolved

daverodgman requested changes Apr 18, 2023

View reviewed changes

tom-cosgrove-arm removed the needs-reviewer This PR needs someone to pick it up for review label Apr 18, 2023

daverodgman reviewed Apr 18, 2023

View reviewed changes

tests/scripts/all.sh Outdated Show resolved Hide resolved

tom-cosgrove-arm reviewed Apr 18, 2023

View reviewed changes

yuhaoth added the needs-review Every commit must be reviewed by at least two team members, label Aug 18, 2023

yuhaoth requested a review from tom-cosgrove-arm August 18, 2023 01:39

yuhaoth removed the needs-work label Aug 18, 2023

lpy4105 reviewed Aug 18, 2023

View reviewed changes

library/aes.c Outdated Show resolved Hide resolved

yuhaoth added 2 commits August 18, 2023 17:26

Add missing check

372f7a0

Signed-off-by: Jerry Yu <[email protected]>

improve readability of error message

61fc5ed

Signed-off-by: Jerry Yu <[email protected]>

yuhaoth requested a review from lpy4105 August 18, 2023 09:29

revert padlock from aesni module

0a6272d

Signed-off-by: Jerry Yu <[email protected]>

yuhaoth mentioned this pull request Aug 18, 2023

Add architecture detection macros #8093

Merged

3 tasks

daverodgman approved these changes Aug 18, 2023

View reviewed changes

lpy4105 approved these changes Aug 18, 2023

View reviewed changes

lpy4105 added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 18, 2023

daverodgman added this pull request to the merge queue Aug 18, 2023

Merged via the queue into Mbed-TLS:development with commit 1fdc884 Aug 18, 2023

yuhaoth deleted the pr/add-aes-accelerator-only-mode branch August 21, 2023 01:34

yuhaoth added the priority-very-high Highest priority - prioritise this over other review work label Aug 23, 2023

mkow mentioned this pull request Sep 4, 2023

[Integrations/azure] Add RA-TLS/SecretProv libs for MAA gramineproject/contrib#38

Merged

akien-mga mentioned this pull request Sep 7, 2023

Compile error for AES-NI with MSVC on ARM32 (UWP), AES-NI intrinsics are only on x86 and x64 #8168

Closed

lpy4105 mentioned this pull request Sep 14, 2023

2.28: Fix AESNI selection #8209

Merged

3 tasks

daverodgman mentioned this pull request Sep 29, 2023

Improve AES hardware-only check #8281

Merged

3 tasks

gilles-peskine-arm reviewed Oct 10, 2023

View reviewed changes

lpy4105 mentioned this pull request Oct 13, 2023

Support cpuid for win32 #8339

Merged

3 tasks

tom-cosgrove-arm mentioned this pull request Oct 23, 2023

AES-NI compilation on x86 #8400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AES: Add accelerator only mode #7384

AES: Add accelerator only mode #7384

yuhaoth commented Mar 31, 2023 •

edited by daverodgman

Loading

daverodgman Apr 17, 2023

yuhaoth Apr 18, 2023

yuhaoth Apr 18, 2023

tom-cosgrove-arm Apr 18, 2023

yuhaoth Apr 18, 2023

gilles-peskine-arm Apr 18, 2023

yuhaoth Apr 19, 2023

daverodgman Apr 21, 2023

tom-cosgrove-arm Apr 21, 2023 •

edited

Loading

daverodgman Apr 21, 2023

yuhaoth commented Apr 18, 2023

daverodgman left a comment

tom-cosgrove-arm Apr 18, 2023 •

edited

Loading

tom-cosgrove-arm Apr 18, 2023

lpy4105 left a comment

gilles-peskine-arm Oct 10, 2023

AES: Add accelerator only mode #7384

AES: Add accelerator only mode #7384

Conversation

yuhaoth commented Mar 31, 2023 • edited by daverodgman Loading

Description

Gatekeeper checklist

Notes for the submitter

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

tom-cosgrove-arm Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth commented Apr 18, 2023

daverodgman left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpy4105 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhaoth commented Mar 31, 2023 •

edited by daverodgman

Loading

tom-cosgrove-arm Apr 21, 2023 •

edited

Loading

tom-cosgrove-arm Apr 18, 2023 •

edited

Loading