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.

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

Support armv8 crypto extensions for AES and GCM #5387

Closed
gilles-peskine-arm opened this issue Jan 5, 2022 · 23 comments · Fixed by #6895, #6918 or #7316
Closed

Support armv8 crypto extensions for AES and GCM #5387

gilles-peskine-arm opened this issue Jan 5, 2022 · 23 comments · Fixed by #6895, #6918 or #7316
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-l Estimated task size: large (2w+)

Comments

@gilles-peskine-arm
Copy link
Contributor

The goal of this task is to allow Mbed TLS to use ISA extensions for AES and GCM on armv8 platforms.

A work in progress from 2018 is in #1173 and #1424 (https://github.com/ARMmbed/mbedtls/tree/archive/armv8_crypto_extensions).

@yanesca
Copy link
Contributor

yanesca commented Jan 6, 2023

We had a discussion about this and agreed to:

  • Prefer intrinsics if possible
  • Provide a tri-state choice for configuration: H/W acceleration not used; H/W acceleration used if detected at runtime; H/W acceleration (only) used (will fail if H/W support not present).

@yanesca yanesca added this to Mbed TLS 3.4 release in EPICs for Mbed TLS Jan 6, 2023
@yuhaoth yuhaoth added the size-l Estimated task size: large (2w+) label Jan 10, 2023
@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 10, 2023

Do we plan to add Arm64 CI test? I think it should be added, this issue can not be covered for time being.

@yanesca
Copy link
Contributor

yanesca commented Jan 10, 2023

Yes, I think we should add testing to the CI at least with qemu. However, if it doesn't make things too difficult, I suggest add the feature first (validate it with manual testing) and add the tests to the CI only afterwards.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 11, 2023

Add list of sub-tasks.

  • Implement AES relative functions. ( WIP at Add AES with armv8 crypto extension #6895, pass test_suite_{aes,gcm}.* )
  • Implement GCM relative functions. ( WIP at Add AES with armv8 crypto extension #6895, pass test_suite_{aes,gcm}.* )
  • Improve compile time architecture detection.
  • Improve runtime CPU feature sets detection.
  • Clang and GCC versions compatible tests.
  • Add HardWare only config options
  • Add MSVC support.
  • Add tests for aarch64. Base on qemu or Arm64 node.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 12, 2023

Known issues when development.

  • illegal instructions when MBEDTLS_SHA512_USE_A64_CRYPTO_IF_PRESENT enabled and -O3 . Until now, I did not know which options cause the issues.
  • Compiler feature sets detection is moved the c source file due to compile fail at generate_psa_test.py . That need discussion how to resolve that.

@gilles-peskine-arm
Copy link
Contributor Author

Compiler feature sets detection is moved the c source file due to compile fail at generate_psa_test.py

What's the problem?

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 12, 2023

What's the problem?

Reproduce the issue on Arm64 host with below scripts.

scripts/config.py full
CFLAGS="-march=armv8.2-a+sha3+crypto" cmake .
make

It reports

[ 46%] Generating suites/test_suite_psa_crypto_generate_key.generated.data, suites/test_suite_psa_crypto_not_supported.generated.data, suites/test_suite_psa_crypto_op_fail.generated.data, suites/test_suite_psa_crypto_storage_format.current.data, suites/test_suite_psa_crypto_storage_format.v0.data
In file included from include/mbedtls/build_info.h:141,
                 from include/psa/crypto_platform.h:39,
                 from include/psa/crypto.h:25,
                 from /tmp/tmp--woaiv4gs.c:4:
include/mbedtls/check_config.h:740:8: error: #error "Must use minimum -march=armv8.2-a+sha3 for MBEDTLS_SHA512_USE_A64_CRYPTO_*"
  740 | #      error "Must use minimum -march=armv8.2-a+sha3 for MBEDTLS_SHA512_USE_A64_CRYPTO_*"
      |        ^~~~~
include/mbedtls/check_config.h:767:2: error: #error "Must use minimum -march=armv8-a+crypto for MBEDTLS_SHA256_USE_A64_CRYPTO_*"
  767 | #error "Must use minimum -march=armv8-a+crypto for MBEDTLS_SHA256_USE_A64_CRYPTO_*"
      |  ^~~~~
Traceback (most recent call last):
  File "/home/haoyu01/workspace_ceph/mbedtls/tests/../tests/scripts/generate_psa_tests.py", line 920, in <module>
    test_data_generation.main(sys.argv[1:], __doc__, PSATestGenerator)
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/test_data_generation.py", line 235, in main
    generator.generate_target(target)
  File "/home/haoyu01/workspace_ceph/mbedtls/tests/../tests/scripts/generate_psa_tests.py", line 917, in generate_target
    super().generate_target(name, self.info)
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/test_data_generation.py", line 188, in generate_target
    self.write_test_data_file(name, test_cases)
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/test_data_generation.py", line 175, in write_test_data_file
    test_case.write_data_file(filename, test_cases)
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/test_case.py", line 99, in write_data_file
    for tc in test_cases:
  File "/home/haoyu01/workspace_ceph/mbedtls/tests/../tests/scripts/generate_psa_tests.py", line 769, in all_test_cases
    if key.location_value() != 0:
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/psa_storage.py", line 179, in location_value
    return self.lifetime.value() >> 8
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/psa_storage.py", line 82, in value
    self.update_cache()
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/psa_storage.py", line 54, in update_cache
    values = c_build_helper.get_c_expression_values(
  File "/mnt/local/workspace_ceph/mbedtls/tests/scripts/../../scripts/mbedtls_dev/c_build_helper.py", line 148, in get_c_expression_values
    subprocess.check_call(cmd + [c_name])
  File "/home/haoyu01/.pyenv/aarch64/versions/3.10.5/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['cc', '-Iinclude', '-o/tmp/tmp--woaiv4gs', '/tmp/tmp--woaiv4gs.c']' returned non-zero exit status 1.
make[2]: *** [tests/CMakeFiles/test_suite_psa_generated_data.dir/build.make:85: tests/suites/test_suite_psa_crypto_generate_key.generated.data] Error 1

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 12, 2023

Compiler feature sets detection is moved the c source file

it is implemented at e9a7159

@gilles-peskine-arm
Copy link
Contributor Author

Ah, ok. make generated_files needs a host C compiler. When cross-compiling, you need to do it in two steps:

CC=host-cc make generated_files
CC=cross-cc make

Ideally cmake would automatically do the right thing, but we haven't done anything to make it work, and I don't know how hard it is.

This is documented in README.md.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 12, 2023

Ah, ok. make generated_files needs a host C compiler. When cross-compiling, you need to do it in two steps:

CC=host-cc make generated_files
CC=cross-cc make

Ideally cmake would automatically do the right thing, but we haven't done anything to make it work, and I don't know how hard it is.

This is documented in README.md.

But my host is arm64. I build it on ARM64 host.

@gilles-peskine-arm
Copy link
Contributor Author

But my host is arm64. I build it on ARM64 host.

Oh? Then I don't understand the error. All that's required for generate_psa_tests is that it works to compile a program and run it on the build machine. If make generated_files && CFLAGS="-march=armv8.2-a+sha3+crypto" make all test works then doing CFLAGS="-march=armv8.2-a+sha3+crypto" make all test directly should work as well.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 12, 2023

subprocess.CalledProcessError: Command '['cc', '-Iinclude', '-o/tmp/tmp--woaiv4gs', '/tmp/tmp--woaiv4gs.c']' returned non-zero exit status 1.

That equal cc -Iinclude -o execute -c a.c . CFLAGS="-march=armv8.2-a+sha3+crypto" is not set in this case.

@gilles-peskine-arm
Copy link
Contributor Author

I see. generate_psa_tests.py honors CC but not CFLAGS. This might not be right. Also, it honors mbedtls_config.h, which is not really right since the result shouldn't depend on the configuration.

None of this should be a problem in the full config though, only if you activate an armv8 acceleration option. When testing with armv8 acceleration, please run make generated_files first. Note that this is done automatically in all.sh (all components start with the generated files present).

@tom-cosgrove-arm
Copy link
Contributor

generate_psa_test.py (specifically c_build_helper.py) isn't passing on CFLAGS

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 13, 2023

I'd like reduce the scope of this issue.

I won't resolve the issues raised at #5387 (comment). and I will add it as known issue.

Below points need confirm.

  • For GCC, the minimal version supported should be gcc6 .
  • For clang, the minimal version supported should be clang4 .
  • MSVC won't be supported in this issue
  • For runtime detection, only linux is supported. Other platform will suppose be supported.
  • For check_config.h, I won't create compiler flag check like sha256 due to ARM64 HOST compile break.

I will create new issues to cover left topics before Chinese New Year.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 13, 2023

Update , some sub-tasks are moved to other issues

@gilles-peskine-arm
Copy link
Contributor Author

For check_config.h, I won't create compiler flag check like sha256 due to ARM64 HOST compile break.

We should not check compiler flags in check_config.h because it's included in application code. You don't need to compile your application for a platform that has crypto instructions, only the crypto library.

aes.c is the right place to check compiler support.

@yuhaoth
Copy link
Contributor

yuhaoth commented Jan 13, 2023

For check_config.h, I won't create compiler flag check like sha256 due to ARM64 HOST compile break.

We should not check compiler flags in check_config.h because it's included in application code. You don't need to compile your application for a platform that has crypto instructions, only the crypto library.

aes.c is the right place to check compiler support.

Got it . I just split the tasks. And I will proposal my solution on #5758. Let's continue the discussion in it.

@yanesca
Copy link
Contributor

yanesca commented Feb 9, 2023

Regarding #6895 (comment), I agree with @yuhaoth and I think that SHA3 extension is specific enough to make an exception for it. I think we should constrain the SHA3 extension to the SHA module. If we can't do it from the build system, then we should turn it on in the source file.

@gilles-peskine-arm @tom-cosgrove-arm what do you think?

@gilles-peskine-arm
Copy link
Contributor Author

@yanesca I'm sorry, I have no idea what exception you're referring to. I've posted my thoughts on #7004 (review) and #6895 (comment).

@yuhaoth
Copy link
Contributor

yuhaoth commented Feb 13, 2023

Yes, I think we should add testing to the CI at least with qemu.

I have added Arm64 travis ci test in #6895 . for qemu, I will try add a guide firstly.

@yanesca yanesca moved this from Mbed TLS 3.4 release to AES armv8 acceleration in EPICs for Mbed TLS Feb 21, 2023
@yanesca
Copy link
Contributor

yanesca commented Feb 21, 2023

Raised #7141 for tracking hardware only config options and collected the AES armv8 related issues in an epic:
https://github.com/orgs/Mbed-TLS/projects/1#column-19366507

@daverodgman daverodgman added this to AES armv8 acceleration in Backlog for Mbed TLS Feb 22, 2023
@daverodgman daverodgman removed this from AES armv8 acceleration in EPICs for Mbed TLS Feb 22, 2023
@daverodgman daverodgman added this to AES armv8 acceleration in EPICs for Mbed TLS Feb 22, 2023
@daverodgman daverodgman removed this from AES armv8 acceleration in Backlog for Mbed TLS Feb 22, 2023
@yanesca
Copy link
Contributor

yanesca commented Mar 1, 2023

Reopening as GCM support is still need: #6918

@yanesca yanesca reopened this Mar 1, 2023
@yuhaoth yuhaoth linked a pull request Mar 20, 2023 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-l Estimated task size: large (2w+)
Projects
None yet
6 participants