-
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
Arm v8 ISA Crypto extensions #1424
Conversation
Assigning to @gilles-peskine-arm and @yanesca for review. |
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.
Two major design issues, in addition to a few minor style issues:
- Why are we making the primitive functions part of the public interface? If there's no good reason for it, they should remain internal.
- If the option is activated at compile time, it gets used at runtime. But on an ARMv8 platform there's a good chance that software is deployed in the form of portable binaries that must support systems both with and without the crypto extensions, so we should allow dynamic choice. It would also make sense to do CE-only builds for (I think rarer) deployments where the CE are guaranteed to be available and code size is at a premium.
Other than that I'm happy with the code. I haven't reviewed the assembly parts for correctness, but if they work (proved by testing), I'm happy with them.
ChangeLog
Outdated
@@ -25,6 +25,7 @@ Features | |||
uses PBKDF2-SHA2, such as OpenSSL 1.1. Submitted by Antonio Quartulli, | |||
OpenVPN Inc. Fixes #1339 | |||
* Add support for public keys encoded in PKCS#1 format. #1122 | |||
* ARMv8 Crypto Extensions: Faster AES and GCM |
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.
Needs improvement. Suggestion:
Support ARMv8 Cryptography Extensions for AES and GCM.
* \file armv8ce_aes.h | ||
* | ||
* \brief ARMv8 Cryptography Extensions -- Optimized code for AES and GCM | ||
* |
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.
Cut off the Doxygen part so that the copyright notice is in an ordinary comment.
* | ||
* \brief ARMv8 Cryptography Extensions -- Optimized code for AES and GCM | ||
* | ||
* Copyright (C) 2006-2017, ARM Limited, All Rights Reserved |
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.
Copyright 2017-2018
@@ -0,0 +1,60 @@ | |||
/** |
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 are these public functions? Why is this not entirely internal to aes.c
and gcm.c
?
I know that's how it was done for AES-NI, but I don't see a good reason. @mpg Do you know?
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.
Honestly, AES-NI was just doing it the same way as Via Padlock support, so the person who knows the reasons if Paul.
However, if I hazard a guess as to why, I think it's mainly because it looked convenient (avoid clutter in aes.c
) and concerns about minimizing the exposed API and ABI were probably given less importance at that time than now.
include/mbedtls/config.h
Outdated
* | ||
* Requires: MBEDTLS_HAVE_ASM | ||
* | ||
* This module utilizes ARMv8 Crypto Extensions for AES and GCM |
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.
“utilizes” → “adds support”; “Crypto” → “Cryptography” (let's use the official name); full stop at the end of the sentence.
library/CMakeLists.txt
Outdated
@@ -6,6 +6,7 @@ set(src_crypto | |||
aes.c | |||
aesni.c | |||
arc4.c | |||
armv8ce_aes.c |
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.
indent glitch
@@ -42,7 +42,9 @@ | |||
#if defined(MBEDTLS_AESNI_C) | |||
#include "mbedtls/aesni.h" | |||
#endif | |||
|
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.
please keep a blank line
@@ -851,6 +853,11 @@ int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx, | |||
return( mbedtls_aesni_crypt_ecb( ctx, mode, input, output ) ); | |||
#endif | |||
|
|||
#if defined(MBEDTLS_ARMV8CE_AES_C) | |||
// We don't do runtime checking for ARMv8 Crypto Extensions |
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? It could make sense to build without software support, to save code size. But as long as software support is present, I think there should be a fallback.
Since it's impossible to detect the presence of the crypto extensions at runtime in unprivileged mode (unfortunately), this should be a runtime setting — a global variable accessed through a setter function, I guess.
Note that it's possible in principle to have accelerated GCM but not AES or vice versa. I don't know if that happens in practice.
On Linux the setting can be initialized by checking for the pmull
and aes
flags of /proc/cpuinfo
. I don't know if we want to bother implementing that.
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.
You almost certainly don't want to go to /proc/cpuinfo... To detect processor features in runtime, use hwcaps instead:
#include <asm/hwcap.h>
#include <sys/auxv.h>
int crypto_extensions_present(void)
{
unsigned long hwcaps = getauxval(AT_HWCAP);
return (hwcaps & HWCAP_AES) && (hwcaps & HWCAP_PMULL);
}
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.
@pawelmoll Thanks! How portable is this in terms of kernel, libc, toolchain, versions? Can we assume it's always there when compiling for Aarch64/Aarch32 or should we provide a compile-time option?
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.
According to its man page:
The getauxval() function was added to glibc in version 2.16.
(which, it seems, was released in 2012).
As to hwcaps themselves, AArch64 (aka arm64) kernels were providing them from the day one :-) (or at least early enough that you wouldn't care).
As to AArch32/arm environment... It seems that the AES and MULL got into AT_HWCAP2 word:
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/include/uapi/asm/hwcap.h)
but I must say that I have no crypto-enabled platform I could test it on...
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.
Re AArch32 again: they were added "only" in 5.3
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/arm/include/uapi/asm/hwcap.h?id=8258a9895c99cdaacad8edc4748c0a624c710961)
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.
So that's Linux-only, not some portability layer? At this point, for Arch64, we can probably reasonably assume that __linux__
implies ≥5.3. Should we also have a guard on __GLIBC__
?
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's definitely a Linux thing, yes. And yes, on AArch64 I'd just assume HW_CAPS have all what you need (checked and the AES and MULL bits were added in 3.13).
As to "non-glibc libc-s", musl and bionic seem to have the function:
http://git.musl-libc.org/cgit/musl/tree/src/misc/getauxval.c
https://android.googlesource.com/platform/bionic/+/master/libc/bionic/getauxval.cpp
but, for example, uclibc doesn't...
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, there's an old lwn article describing auxv:
Even without using the new library function, an application that wants to access the auxiliary vector merely needs to obtain the address of the location that follows the NULL pointer at the end of the environment list.
|
||
#if defined(MBEDTLS_ARMV8CE_AES_C) | ||
|
||
#include <arm_neon.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.
Is this header present in all toolchains for ARMv8 (that we care about)?
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's part of ACLE, I believe, so yes - the intrinsics should be readily available:
https://developer.arm.com/documentation/101028/0006/3--C-language-extensions?lang=en
library/armv8ce_aes.c
Outdated
|
||
if( mode == MBEDTLS_AES_ENCRYPT ) | ||
{ | ||
for( i = ctx->nr - 1; i ; i-- ) // encryption loop |
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.
Style: please write i != 0
for the condition. Applies to the decryption loop as well.
@@ -6,6 +6,7 @@ set(src_crypto | |||
aes.c | |||
aesni.c | |||
arc4.c |
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 new file needs to be added to the VS build as well.
A compact patch that provides AES and GCM implementations that utilize the ARMv8 Crypto Extensions. The config flag is MBEDTLS_ARMV8CE_AES_C, which is disabled by default as we don't do runtime checking for the feature. The new implementation lives in armv8ce_aes.c. Provides similar functionality to #432 Thanks to Barry O'Rourke and others for that contribtion. Tested on a Cortex A53 device and QEMU. On a midrange phone the real AES-GCM throughput increases about 4x, while raw AES speed is up to 10x faster. When cross-compiling, you want to set something like: export CC='aarch64-linux-gnu-gcc' export CFLAGS='-Ofast -march=armv8-a+crypto' scripts/config.pl set MBEDTLS_ARMV8CE_AES_C QEMU seems to also need export LDFLAGS='-static' Then run normal make or cmake etc.
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
231265b
to
e680037
Compare
ARMv8 Crypto Extensions - as written by @mjosaarinen and originally submitted as #1173. This PR is resubmitted as a branch on the mbedtls repository to allow other members of the Mbed TLS team to push commits to the branch and permit review and rework.
To quote the original PR: