Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
[TLS / hw acceleration] AES ECB for NUCLEO_F439ZI #3691
This pull request contains the very first HW acceleration for STM devices
Once we agree on this files format and structure, we will be able to add soon AES-CBC mode, SHA1 etc...
Steps to test or reproduce
mbed test -m NUCLEO_F439ZI -t IAR -n tests-mbedtls-selftest
The idea of keeping the mbed TLS acceleration driver code in the
Does this present any issues for you?
features/mbedtls/targets/TARGET_STM/aes_alt.c is calling functions from targets/.../TARGET_STM32F4/device/stm32f4_hal_cryp.c
so from ST's point of view, ST code is separated out
Let me try to explain the reason for this request from my colleagues and I. If we can do it, let's go, if we can't, let it go. It is ARM to decide at the end :-)
ST code is spread out in several features (mbedTLS / usb / lwip... ).
ST code is specific for our targets:
When every code is located in a single directory tree we maximize the chance that every ST contributor is aware of the changes coming from others, and is better aware of the impact of his/her changes.
mbed compilation and link mechanism is creating a long list of include directories, so the code could be located here or there, I think it has no impact, has it ?
Anyway I have written this pull request following the specification (ST code in features/mbedtls).
I think this is a useful discussion, and we have been seeking feedback from partners on the hardware acceleration interfaces. However, this is going outside the scope of this PR, and it may be more useful to discuss this directly.
To clarify - the targets directory is concerned with the mbed HAL and the interfaces mbed OS presents. mbed TLS has a life outside of mbed OS and is used on other platforms, and has separate and distinct interfaces itself. For instance, note mbed OS has a TRNG HAL API, whereas mbed TLS has it's own entropy interface, a mbed OS platform code brings the two interfaces together.
I think it's understandable that platform/SoC/driver code that might reside in the TARGETS directory may be called by driver code in the mbed TLS code driving the hardware acceleration code. I don't think that is an issue, and you are correct to say we could place the sources in other places (and indeed could move them later). This is really about logical organisation and management.
I have done an initial review and found the following:
- There are some issues with the use of the _ALT macros.
- I do not think this code is thread safe.
- I think there is an opportunity to reconcile the
CRYP_HandleTypeDefas some of the data is duplicated, unused or redundant.
- Pointed out places where the function return codes are not checked for errors.
I agree with @adustm about having all target support code in one place under
This PR is not to the mbed TLS specifically (e.g. this is not http://github.com/ARMmbed/mbedtls), but to extend an existing target support in mbed OS for TLS features. If the TLS team would like to request from ST to contribute this HW crypto support to http://github.com/ARMmbed/mbedtls then please follow the formal process via email.
Also if there is visibility issue, then this could be addressed by adding a label for the ARM mbed TLS team to review.
Perhaps this is a wider question (hence cc @sg-):
Also it's important to understand the maintainability point of view, e.g. how much code mbed Partner teams are managing.
For example, the following structure may satisfy both ST team code maintenance and glue layers for features support
With this structure the feature driver code and the feature glue layer for would be gated by the presence of the feature (meaning that it won't be compiled if the feature is not enabled). Also it would be in one place from mbed Partner maintenance point of view.
Top level target code is organized to contain MCU SDKs and a binding to the mbed HAL. Any feature integrated as a mbed HAL interface and enabled by
In this case, I agree with @sbutcher-arm
From the developer guide for hardware acceleration: https://docs.mbed.com/docs/mbed-os-handbook/en/latest/advanced/tls_hardware_acceleration/#how-to-implement-the-functions
I hope everything will be fine now.
@adustm: Thanks for taking into account the previous review comments. I have reviewed the code once more and most of the issues I pointed out seem to have been resolved.
From my point of view, there are still a couple of issues left:
- The functions
mbedtls_aes_decrypt()do nor have return values, making difficult to check whether the crypto accelerator succeeded. This touches on the API design, so I have raised the issue ARMmbed/mbedtls#817.
- There have been some improvements, but I do not think that this code is thread-safe. Normally, we achieve this in mbed TLS by using the threading API (threading.h). Please note that there is a PR to add support for this: #3150
I have looked in the changes, and I saw that there is many code duplicity, and copy\paste from the sw implementation. I beleive this is due to the current implementation of the HW configuration. This code duplication could have been resolved by
Requires minor changes, but otherwise approved by me.
Looking over the code, and based on the feedback from @andresag01 and @RonEld, I can see we need to refine the hardware acceleration interface that mbed TLS presents. I don't want those changes to hold up this work, so we'll do that independently and then later can revise this code to take advantage of those changes. Please feel free to provide more feedback on those changes.
Thanks for your patience as we've worked through this pull request.
My only concern is the calls to
I have read PR#3150 but it is not clear to me what you are waiting for: shall I wait until this thread PR is integrated in mbed-tls then in mbed-os ? What would be the impact to my code ?
Or can it be treated separately in another PR later on ? I have work about AES for 2 other platforms + SHA1 and SHA256 that are pending the integration of the current PR. I would like to be able to progress on the subject.