Support hw-accelerated AES on Thumb and Arm #8326

daverodgman · 2023-10-08T11:32:17Z

Description

Extend support for AESCE to 32-bit builds.

Saves around 450b for Thumb 2 -Oz (but not for TF-M, because our reference target doesn't have Neon).

Manually tested via qemu, for arm, thumb and aarch64 with gcc and clang.

Do we want to put this in the Changelog as Security, because it enables const-time AES for these targets?

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

changelog provided
backport no - not a bugfix
tests provided - build test only. Manually tested with qemu

yuhaoth

Some comments

library/aesce.c

yuhaoth · 2023-10-09T03:30:34Z

I just push a commit to remove plain c code. mtest reports no code size improvement because mbedtls_internal_aes_[en|de]crypt use plain c .

I test it with below commands. Clang version is 14.0

# Plain C only
../mbedtls-docs/tools/bin/mtest -s -MBEDTLS_AESCE_C -MBEDTLS_AES_USE_HARDWARE_ONLY armv8-thumb2
# AESCE only
../mbedtls-docs/tools/bin/mtest -s MBEDTLS_AESCE_C MBEDTLS_AES_USE_HARDWARE_ONLY armv8-thumb2
# Both
../mbedtls-docs/tools/bin/mtest -s MBEDTLS_AESCE_C -MBEDTLS_AES_USE_HARDWARE_ONLY armv8-thumb2

And the result is

	with commit	Without commit
Plain C only	260410	260410
AESCE only	258328	260652
Both	262023	261979

yuhaoth

Just check the clang versions. poly64*_t are available in clang 11.0 on aarch32

tests/scripts/all.sh

library/aesce.c

daverodgman · 2023-10-09T13:51:03Z

@yuhaoth I think the existing implementation of vget_low_p64 for GCC 5 is wrong - looking at arm_neon.h different behaviour is needed for big/little endian. I think that the implementation I've done for clang is correct (it is just a trivial wrapper around vget_low_u64 which already handles endianness) - probably we should use this for gcc as well. WDYT?

daverodgman · 2023-10-09T14:31:40Z

I just push a commit to remove plain c code. mtest reports no code size improvement because mbedtls_internal_aes_[en|de]crypt use plain c .

This looks good - please raise as a separate PR when this is in - let's avoid scope creep on this one.

daverodgman · 2023-10-09T16:56:50Z

@bensze01 please could you add the linux-headers package to the CI (at least on the Ubuntu image) so that we can test the runtime-detection here? thanks

gilles-peskine-arm · 2023-10-09T17:36:49Z

please could you add the linux-headers package to the CI (at least on the Ubuntu image) so that we can test the runtime-detection here? thanks

It's easy to add the package to the docker images, but should we? Compiling a Linux application should not require kernel headers. If we do something that relies on them (and we aren't testing mbedtls as a Linux kernel component), it's good that it fails to compile on the CI: it tells us we're doing something wrong.

daverodgman · 2023-10-09T17:43:16Z

It's easy to add the package to the docker images, but should we? Compiling a Linux application should not require kernel headers. If we do something that relies on them (and we aren't testing mbedtls as a Linux kernel component), it's good that it fails to compile on the CI: it tells us we're doing something wrong.

That is what I thought. On my local Ubuntu 22.04, it's OK to only include <sys/auxv.h> because (AIUI) it pulls in a copy of the necessary defines that is intended for user-space to use (in <bits/hwcap.h>, provided by libc6-dev), so I removed our pre-existing include of <asm/hwcap.h> and it worked fine. It didn't work on the CI though - maybe that is lacking libc6-dev? So I reverted to including <asm/hwcap.h>, but I'm open to suggestions as to the preferred way to do it.

Currently in development we include <asm/hwcap.h>, and we don't build-test it in the CI. That also seems to be the case for SHA256 (see somewhat related #7198 ). #8298 adds a build test for SHA256 - unsure why that is able to pass (without needing hwcap.h), but this PR is not. Maybe HWCAP_SHA2 is defined by auxv.h whereas the ones we check here are not?

gilles-peskine-arm · 2023-10-09T18:47:39Z

sys/auxv.h is in the libc6-dev package on on an arm64 Ubuntu. More generally, it's in the glibc dev package on architectures that have this header (it doesn't exist for x86). On other architectures, it's in an extra libc package that's part of the cross-compilation toolchain, e.g. linux-libc-dev-armel-cross, which libc6-dev-armel-cross. depends on.

I don't know why the cross-compilation toolchain packages are split this way. The native packages have a similar split, but sys/auxv.h is in libc6-dev, not linux-libc-dev.) linux-libc6-dev-xxx contains some configuration-agnostic and backward-compatible (but architecture-dependent) kernel headers repackaged by glibc and intended to compile user mode applications, as opposed to linux-headers-xxx containing configuration-specific and version-specific kernel headers intended to compile kernel modules. My earlier comment was about the linux-headers-xxx packages. I don't see any reason not to install the linux-libc-dev-xxx packages.

In fact, we do have the linux-libc-dev-xxx packages, because the libc6-dev-xxx packages depend on them. But we only have libc6-dev-armel-cross, i.e. armv7 without SIMD. What I think we're missing here is arm64 packages, for armv8 support.

yuhaoth · 2023-10-10T02:33:04Z

@yuhaoth I think the existing implementation of vget_low_p64 for GCC 5 is wrong - looking at arm_neon.h different behaviour is needed for big/little endian. I think that the implementation I've done for clang is correct (it is just a trivial wrapper around vget_low_u64 which already handles endianness) - probably we should use this for gcc as well. WDYT?

Yes, it is wrong.

yuhaoth · 2023-10-10T03:13:17Z

library/aesce.c

-#   if __clang_major__ < 4
-#       error "Minimum version of Clang for MBEDTLS_AESCE_C is 4.0."
+#   if defined(MBEDTLS_ARCH_IS_ARM32) && (__clang_major__ < 11)
+#       error "Minimum version of Clang for MBEDTLS_AESCE_C on 32-bit Arm or Thumb is 111.0."


wrong typo

Suggested change

# error "Minimum version of Clang for MBEDTLS_AESCE_C on 32-bit Arm or Thumb is 111.0."

# error "Minimum version of Clang for MBEDTLS_AESCE_C on 32-bit Arm or Thumb is 11.0."

tom-cosgrove-arm · 2023-10-10T07:22:31Z

library/sha256.c

        asm ("sha256h q0, q0, v0.4s" : : : "v0");
+#else
+        asm ("sha256h.32 q0, q0, q0" : : : "q0");


Would it make sense to put the volatile on these two lines? (I was going to raise a PR to do this, but it would conflict with this one)

DId you mean to make this comment on the SHA PR? I can do that there if you like.

Well, yes, but we should always have asm volatile

Not for asm that does not have optimisable-out side effects - for this (i.e. the normal case where the asm is just used to compute something) we can allow the compiler to re-order this with respect to surrounding code if it wants.

daverodgman · 2023-10-10T09:05:13Z

sys/auxv.h is in the libc6-dev package on on an arm64 Ubuntu. More generally, it's in the glibc dev package on architectures that have this header (it doesn't exist for x86). On other architectures, it's in an extra libc package that's part of the cross-compilation toolchain, e.g. linux-libc-dev-armel-cross, which libc6-dev-armel-cross. depends on.

I don't know why the cross-compilation toolchain packages are split this way. The native packages have a similar split, but sys/auxv.h is in libc6-dev, not linux-libc-dev.) linux-libc6-dev-xxx contains some configuration-agnostic and backward-compatible (but architecture-dependent) kernel headers repackaged by glibc and intended to compile user mode applications, as opposed to linux-headers-xxx containing configuration-specific and version-specific kernel headers intended to compile kernel modules. My earlier comment was about the linux-headers-xxx packages. I don't see any reason not to install the linux-libc-dev-xxx packages.

In fact, we do have the linux-libc-dev-xxx packages, because the libc6-dev-xxx packages depend on them. But we only have libc6-dev-armel-cross, i.e. armv7 without SIMD. What I think we're missing here is arm64 packages, for armv8 support.

Thanks. I think that it's not the arm64 packages that we are missing, but the 32-bit Armv8 packages, i.e. the hard-float ABI packages?

On further investigation on my system (aarch64 Ubuntu 22.04), linux-libc-dev provides the aarch64 definitions in asm/hwcap. The 32-bit definitions are also in a (different) asm/hwcap, which is provided by linux-libc-dev-armhf-cross. There is a second copy of the native 64-bit definitions in bits/hw_cap which is automatically pulled in via sys/auxv.h.

In searching the web, asm/hwcap seems to be how these flags are normally included - see e.g. https://community.arm.com/arm-community-blogs/b/operating-systems-blog/posts/runtime-detection-of-cpu-features-on-an-armv8-a-cpu#:~:text=include%20%3Csys/auxv.h%3E-,%23include%20%3Casm/hwcap.h%3E,-int%20main()%0A%7B%0A%C2%A0%C2%A0%C2%A0%20long%20hwcaps2

So in this case, I think we're missing linux-libc-dev-armhf-cross from the CI, and possibly linux-libc-dev-arm64-cross. And I think the code should include asm/hwcap.h.

tom-cosgrove-arm · 2023-10-10T09:11:18Z

ChangeLog.d/sha256-armce-arm.txt

+Features
+    * Support Armv8 Crypto Extension acceleration for SHA-256
+      when compiling for Thumb or 32-bit Arm.
+


(a) extra blank line at the end of the file
(b) PR title references AES, this is about SHA-256 - isn't that #8298?

Again, you've commented on the wrong PR :-)

Is this PR missing needs-preceding-pr?

It's based on 8298 in order to pick up the definition of MBEDTLS_ARCH_IS_ARMV8. Yes, I'll add the label.

daverodgman · 2023-10-10T13:19:55Z

CI failure is spurious - OpenCI passed

Co-authored-by: Jerry Yu <[email protected]> Signed-off-by: Dave Rodgman <[email protected]>

Signed-off-by: Dave Rodgman <[email protected]>

yuhaoth

LGTM

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2024-01-30T13:47:52Z

tests/scripts/all.sh

+support_build_aes_armce() {
+    # clang >= 4 is required to build with AES extensions
+    ver="$(clang --version|grep version|sed -E 's#.*version ([0-9]+).*#\1#')"
+    [ "${ver}" -ge 11 ]


Bah, missed that this doesn't match the comment two lines above. I have a PR touching this function that I'll fix this in

gilles-peskine-arm · 2024-06-19T12:47:07Z

tests/scripts/all.sh

+    make -B library/aesce.o CC=clang CFLAGS="--target=arm-linux-gnueabihf -mcpu=cortex-a32+crypto -mthumb -S"
+    not grep -E 'aes[0-9a-z]+.[0-9]\s*[qv]' library/aesce.o
+    msg "clang, test aarch64 crypto instructions not built"
+    make -B library/aesce.o CC=clang CFLAGS="--target=aarch64-linux-gnu -march=armv8-a -S"


@tom-cosgrove-arm I'm confused as to exactly in what setting we test the absence of AES instructions. Specifically, why are we testing that on armv8-a and not armv8-a+crypto? Is that just copypasta or is there a reason?

Actually, I think the errors are on lines 4441 and 4444. Here we are checking that we don't emit AES instructions when we don't ask for them, so that (for example) we won't get SIGILL when run on an older Raspberry Pi (IIRC the RPi5 is the first with crypto extensions). If we passed +crypto to the compiler it would be allowed to use any of those instructions itself, so could give false positives. So not having +crypto here is correct, but having +crypto in the two cases above is incorrect

I see, thanks! I'll fix it in #9286.

Hmmm...

we don't emit AES instructions when we don't ask for them

But if we're telling the compiler not to use AES instructions, how would the test ever fail? If we were enabling assembly that calls AES instructions?

And below with sha256:

make -B library/sha256.s CC=clang CFLAGS="--target=aarch64-linux-gnu -march=armv8-a" grep -E 'sha256[a-z0-9]+\s+[qv]' library/sha256.s

So we do find sha256 instructions on armv8-a, presumably from our assembly?

Just following this thread for interest, but I just noticed and was about to raise: does this grepping work? It seems like we are grepping for the assembler mnemonic in the object file, which doesn't seem like it would work, even if we have intrinsics.

Surely we need to objdump it first?

[I have checked on x86 in case I'm missing something and the aeskeygenassist intrinsic doesn't leave a symbol in the object file, for example]

The -S in CFLAGS means "output assembly, not object code, into the output file" (which we still name with.o, but hey-ho)

Ah okay, makes sense, thanks!

commit acbdbc7c2b32368b07a5b49a170b202d880afa12 Author: Gilles Peskine <[email protected]> Date: 2024-06-19 14:16:05 +0200 Use .s extension for assembly Having assembly files called *.o was confusing.

daverodgman mentioned this pull request Oct 8, 2023

Support SHA256 acceleration on Armv8 thumb2 and arm #8298

Merged

3 tasks

daverodgman force-pushed the aesce-thumb2 branch from cb9a096 to ebaf8c3 Compare October 8, 2023 11:42

yuhaoth suggested changes Oct 9, 2023

View reviewed changes

library/aesce.c Outdated Show resolved Hide resolved

library/aesce.c Outdated Show resolved Hide resolved

library/aesce.c Outdated Show resolved Hide resolved

library/aesce.c Outdated Show resolved Hide resolved

yuhaoth suggested changes Oct 9, 2023

View reviewed changes

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

library/aesce.c Outdated Show resolved Hide resolved

daverodgman force-pushed the aesce-thumb2 branch from ab90683 to 8e39454 Compare October 9, 2023 16:33

yuhaoth reviewed Oct 10, 2023

View reviewed changes

tom-cosgrove-arm reviewed Oct 10, 2023

View reviewed changes

daverodgman added the needs-preceding-pr Requires another PR to be merged first label Oct 10, 2023

daverodgman requested review from yuhaoth and tom-cosgrove-arm October 10, 2023 09:30

daverodgman removed the needs-ci Needs to pass CI tests label Oct 10, 2023

daverodgman added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 24, 2023

yuhaoth removed the needs-preceding-pr Requires another PR to be merged first label Oct 25, 2023

daverodgman and others added 13 commits October 25, 2023 09:06

Add link to ACLE docs in comment

f60e44d

Co-authored-by: Jerry Yu <[email protected]> Signed-off-by: Dave Rodgman <[email protected]>

Don't use #ifdef on vreinterpretq_xxx

2c25bdb

Co-authored-by: Jerry Yu <[email protected]> Signed-off-by: Dave Rodgman <[email protected]>

Code style

f4ee5d4

Signed-off-by: Dave Rodgman <[email protected]>

Tidy-up: move GCM code into one place

46267f6

Signed-off-by: Dave Rodgman <[email protected]>

Don't fail tests if hwcap.h not present

7057c08

Signed-off-by: Dave Rodgman <[email protected]>

Share some definitions that are common for clang and GCC 5

90291df

Signed-off-by: Dave Rodgman <[email protected]>

Fix #error typo

b34fe8b

Signed-off-by: Dave Rodgman <[email protected]>

Add volatile to prevent asm being optimised out

cb5c9fb

Signed-off-by: Dave Rodgman <[email protected]>

Use MBEDTLS_ARCH_IS_ARMV8_A not MBEDTLS_ARCH_IS_ARMV8

9fd1b52

Signed-off-by: Dave Rodgman <[email protected]>

Update Changelog to specify Armv8-A

2fe5b85

Signed-off-by: Dave Rodgman <[email protected]>

Test for presence/absence of AES instructions

f3501b4

Signed-off-by: Dave Rodgman <[email protected]>

Clarify changelog

c619906

Signed-off-by: Dave Rodgman <[email protected]>

Remove dependency on asm/hwcap.h

5e41937

Signed-off-by: Dave Rodgman <[email protected]>

daverodgman force-pushed the aesce-thumb2 branch from 972eb15 to 5e41937 Compare October 25, 2023 08:07

daverodgman added 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 and removed needs-work needs-reviewer This PR needs someone to pick it up for review labels Oct 25, 2023

yuhaoth approved these changes Nov 7, 2023

View reviewed changes

tom-cosgrove-arm requested review from tom-cosgrove-arm and removed request for tom-cosgrove-arm November 7, 2023 11:09

tom-cosgrove-arm approved these changes Nov 27, 2023

View reviewed changes

tom-cosgrove-arm 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 Nov 27, 2023

daverodgman added this pull request to the merge queue Nov 27, 2023

Merged via the queue into Mbed-TLS:development with commit 9fbac38 Nov 27, 2023

daverodgman mentioned this pull request Nov 28, 2023

Disable hw AES on Arm for IAR #8573

Merged

3 tasks

tom-cosgrove-arm reviewed Jan 30, 2024

View reviewed changes

gilles-peskine-arm reviewed Jun 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support hw-accelerated AES on Thumb and Arm #8326

Support hw-accelerated AES on Thumb and Arm #8326

daverodgman commented Oct 8, 2023 •

edited

Loading

yuhaoth left a comment

yuhaoth commented Oct 9, 2023 •

edited

Loading

yuhaoth left a comment •

edited

Loading

daverodgman commented Oct 9, 2023

daverodgman commented Oct 9, 2023

daverodgman commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

daverodgman commented Oct 9, 2023 •

edited

Loading

gilles-peskine-arm commented Oct 9, 2023

yuhaoth commented Oct 10, 2023

yuhaoth Oct 10, 2023

tom-cosgrove-arm Oct 10, 2023

daverodgman Oct 10, 2023

tom-cosgrove-arm Oct 10, 2023

daverodgman Oct 10, 2023

daverodgman commented Oct 10, 2023 •

edited

Loading

tom-cosgrove-arm Oct 10, 2023

daverodgman Oct 10, 2023

tom-cosgrove-arm Oct 10, 2023

daverodgman Oct 10, 2023 •

edited

Loading

daverodgman commented Oct 10, 2023

yuhaoth left a comment

tom-cosgrove-arm left a comment

tom-cosgrove-arm Jan 30, 2024

gilles-peskine-arm Jun 19, 2024

tom-cosgrove-arm Jun 19, 2024

gilles-peskine-arm Jun 19, 2024

gilles-peskine-arm Jun 19, 2024

davidhorstmann-arm Jun 19, 2024 •

edited

Loading

tom-cosgrove-arm Jun 19, 2024

davidhorstmann-arm Jun 19, 2024

gilles-peskine-arm Jun 19, 2024

	# error "Minimum version of Clang for MBEDTLS_AESCE_C on 32-bit Arm or Thumb is 111.0."
	# error "Minimum version of Clang for MBEDTLS_AESCE_C on 32-bit Arm or Thumb is 11.0."

Support hw-accelerated AES on Thumb and Arm #8326

Support hw-accelerated AES on Thumb and Arm #8326

Conversation

daverodgman commented Oct 8, 2023 • edited Loading

Description

PR checklist

yuhaoth left a comment

Choose a reason for hiding this comment

yuhaoth commented Oct 9, 2023 • edited Loading

yuhaoth left a comment • edited Loading

Choose a reason for hiding this comment

daverodgman commented Oct 9, 2023

daverodgman commented Oct 9, 2023

daverodgman commented Oct 9, 2023

gilles-peskine-arm commented Oct 9, 2023

daverodgman commented Oct 9, 2023 • edited Loading

gilles-peskine-arm commented Oct 9, 2023

yuhaoth commented Oct 10, 2023

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

daverodgman commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daverodgman Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

daverodgman commented Oct 10, 2023

yuhaoth left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a 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

davidhorstmann-arm Jun 19, 2024 • edited Loading

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

daverodgman commented Oct 8, 2023 •

edited

Loading

yuhaoth commented Oct 9, 2023 •

edited

Loading

yuhaoth left a comment •

edited

Loading

daverodgman commented Oct 9, 2023 •

edited

Loading

daverodgman commented Oct 10, 2023 •

edited

Loading

daverodgman Oct 10, 2023 •

edited

Loading

davidhorstmann-arm Jun 19, 2024 •

edited

Loading