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

STM32L486RG/mbedtls: add aes hw acceleration #4163

Merged
merged 3 commits into from Jul 6, 2017

Conversation

Projects
None yet
10 participants
@adustm
Member

adustm commented Apr 11, 2017

Description

Add hw acceleration for mbedTLS/aes on NUCLEO_L486RG
Please be aware that this device does not support 192bit key size.
The aes selftest and gcm selftest shall be modified not to run this key size. As it is located in the official mbedtls stack, I did not touch it.
is #3962

Status

DO NOT MERGE

Steps to test or reproduce

Run aes seltest from TESTS/mbedtls/selftest/main.cpp

+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| target            | platform_name | test suite             | test case                   | passed | failed | result | elapsed_time (sec) |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.9                |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.11               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.13               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 6.56               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.1                |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 1.75               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.31               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.53               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 1.66               |
| NUCLEO_L486RG-IAR | NUCLEO_L486RG | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 4.51               |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
@adustm

This comment has been minimized.

Member

adustm commented Apr 11, 2017

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Apr 12, 2017

@adustm We have an AES accelerator as well that doesn't support 192 bits. Did you get a signoff on just returning INVALID_KEY_LENGTH for -192 operations when AES_ALT is in use?

@0xc0170 0xc0170 requested review from yanesca and RonEld Apr 12, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2017

@adustm There are conflicts

@RonEld

same comments as were in #3962 and added a question to answer

#define __HAL_RCC_CRYP_CLK_ENABLE __HAL_RCC_AES_CLK_ENABLE
#define __HAL_RCC_CRYP_FORCE_RESET __HAL_RCC_AES_FORCE_RESET
#define __HAL_RCC_CRYP_RELEASE_RESET __HAL_RCC_AES_RELEASE_RESET
#define CRYP AES

This comment has been minimized.

@RonEld

RonEld Apr 12, 2017

Contributor

Aren't these implementation specific definitions?
Shouldn't these be in some STM common header file, or in the aes_alt.c file?
mbedtls_device.h should be included only from mbedtls files(specifically platform_mbed.h), so I think these definitions will not be used anywhere

This comment has been minimized.

@adustm

adustm Jun 13, 2017

Member

ok, I move them to aes_alt.h

This comment has been minimized.

@RonEld

RonEld Jun 13, 2017

Contributor

Please check if you can define these in aes_alt.c instead. As there are many modified files, I can't check this.

This comment has been minimized.

@adustm

adustm Jun 13, 2017

Member

OK to move to aes_alt.c.
The many modified files were due to a rebase on the master branch. I've fixed it now. Only 3 remaining modified files :)

This comment has been minimized.

@adustm
break;
#if defined (TARGET_STM32L486xG)
return(MBEDTLS_ERR_AES_INVALID_KEY_LENGTH);
#else

This comment has been minimized.

@RonEld

RonEld Apr 12, 2017

Contributor

The alternative implementation should support the full API. If HW doesn't support, then the SW implementation should be used

This comment has been minimized.

@yanesca

yanesca Jun 8, 2017

Contributor

This was true at the time of the comment, but since then it has been decided that AES192 is and exception from this rule and hardware drivers not supporting AES192 can return an error.

if ((ctx->hcryp_aes.Init.OperatingMode != CRYP_ALGOMODE_KEYDERIVATION_DECRYPT) || \
(ctx->hcryp_aes.Init.ChainingMode != CRYP_CHAINMODE_AES_CBC) || \
(ctx->hcryp_aes.Init.KeyWriteFlag != CRYP_KEY_WRITE_ENABLE)) {
/* Re-initialize AES IP with proper parameters */

This comment has been minimized.

@RonEld

RonEld Apr 12, 2017

Contributor

what if this if statement is false? HAL_CRYP_DeInit will not be called, but HAL_CRYP_Init will still be called. Is there a resource leak risk here?

This comment has been minimized.

@adustm

adustm Jun 13, 2017

Member

Hello,
This if statement tests if we call the function but the HW module has been used for another purpose since its initialization. It launches both HAL_CRYP_Deinit and HAL_CRYP_init.

There is not else because otherwise, the HW module is already well initialized.

This comment has been minimized.

@RonEld

RonEld Jun 13, 2017

Contributor

Thanks for the explanation, Why then this check is not done for ECB as well?

This comment has been minimized.

@adustm

adustm Jun 19, 2017

Member

Hi,
In ECB mode, it is done internally in the HAL_CRYP_AESECB_Encrypt and HAL_CRYP_AESECB_Decrypt functions.

(ctx->hcryp_aes.Init.ChainingMode != CRYP_CHAINMODE_AES_CBC) || \
(ctx->hcryp_aes.Init.KeyWriteFlag != CRYP_KEY_WRITE_ENABLE)) {
/* Re-initialize AES IP with proper parameters */
if (HAL_CRYP_DeInit(&ctx->hcryp_aes) != HAL_OK)

This comment has been minimized.

@RonEld

RonEld Apr 12, 2017

Contributor

what if this if statement is false? HAL_CRYP_DeInit will not be called, but HAL_CRYP_Init will still be called. Is there a resource leak risk here?

This comment has been minimized.

@adustm

adustm Jun 13, 2017

Member

The only case where HAL_CRYP_DeInit can return HAL_ERROR is when the ctx->hcryp_aes is NULL.
In this case, HAL_CRYP_Init would also return HAL_ERROR.

It means that either the ctx object has been corrupted or the mbedtls_aes_crypt_xxx function was called without a call to aes_set_key_xxx function.

@RonEld

This comment has been minimized.

Contributor

RonEld commented Apr 12, 2017

@stevew817 At the moment we still instruct to support the full functionality

@adustm

This comment has been minimized.

Member

adustm commented Apr 13, 2017

@stevew817 : no. One of my next task will be to add the SW implementation in case of 192 key size...

@stevew817

This comment has been minimized.

Contributor

stevew817 commented Apr 13, 2017

Gotcha. Guess that'll get added to my to-do list as well, then, though it would really make more sense to solve this at the mbedTLS level.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

bump @adustm. Could you fix the merge conflicts?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 2, 2017

bump @adustm

@gilles-peskine-arm

I cannot say pass/fail because I'm not familiar with the hardware. I have left a couple of comments on a few potential issues.

@@ -148,14 +156,46 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
if( mode == MBEDTLS_AES_DECRYPT )

This comment has been minimized.

@gilles-peskine-arm

gilles-peskine-arm May 23, 2017

The common code between the if branch and the else branch is growing, consider factoring them.

This comment has been minimized.

@adustm

This comment has been minimized.

@adustm
@@ -148,14 +156,46 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx,
if( mode == MBEDTLS_AES_DECRYPT )
{
ctx->hcryp_aes.Init.pInitVect = &iv[0]; // used in process, not in the init
#if defined (TARGET_STM32L486xG)

This comment has been minimized.

@gilles-peskine-arm

gilles-peskine-arm May 23, 2017

Is this actually specific to the STM32L486? I'm not familiar with this platform, but looking for documentation online, it seems that those HAL APIs exist for a much broader family. Also, it's odd that the ECB code uses some wrappers (HAL_CRYP_AESECB_Encrypt and HAL_CRYP_AESECB_Decrypt) marked as legacy but the CBC code doesn't. It's hard to figure out when looking at the code whether it's using the hardware correctly, even when a key is used successively with distinct mode. It's also hard to ascertain that the accelerator is properly reset as soon as a key is no longer in use (which is good hygiene so that a security breach does not expose previously-used keys, and is mandated by most security standards).

This comment has been minimized.

@adustm

adustm Jun 19, 2017

Member

@gilles-peskine-arm
I have checked internally what is called 'legacy code'.
I cannot use the legacy code for CBC mode with the STM32L4 family, as it contains a bug (it calls the init function every time, wich is not compatible with the use of the iv buffer). As it is legacy code, it will not be updated soon. That's why I use the suggested non-legacy code.
The provided legacy code for ECB does not show any problem with the tests I did, this is the reason why I use it to simplify the user's readability. If you think we could encounter an issue, do not hesitate to provide me with a use case to test.

Kind regards
Armelle

This comment has been minimized.

@gilles-peskine-arm

gilles-peskine-arm Jun 19, 2017

@adustm Thanks for investigating! I raised this point because the code looked weird. Since you've found that there's a reason for the weirdness, I have no reason to think that the code is wrong.

@yanesca

I have reviewed the PR, but didn't find any new issues.

Our rule regarding AES192 has been changed since the last reviews, I left a comment about it, but I am writing it here too. AES192 is an exception to the rule that requires hardware accelerator drivers to support the full mbed TLS API and they can return an error if they encounter AES192.

@adustm

This comment has been minimized.

Member

adustm commented Jun 8, 2017

Hello @yanesca

AES192 is an exception to the rule that requires hardware accelerator drivers to support the full mbed TLS API and they can return an error if they encounter AES192.

This is great news. I'll rebase and update this PR as soon as I'll have finished with MD5/SHA1/SHA256 of F7 and F4.

Thanks !
Armelle

@adustm adustm force-pushed the adustm:STM_aes_l486rg branch from 2cd78d1 Jun 12, 2017

@adustm

This comment has been minimized.

Member

adustm commented Jun 13, 2017

Hello @yanesca
AES and GCM selftests don't pass. I need to bypass the 192 key size in the for loop.
Would you like me to push the modified mbedtls/src/aes.c and mbedtls/src/gcm.c files ?

Kind regards
Armelle

@adustm adustm force-pushed the adustm:STM_aes_l486rg branch Jun 13, 2017

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Jun 13, 2017

@adustm - The issue of AES-192 support has been raised as an issue on mbed TLS here.

@adustm

This comment has been minimized.

Member

adustm commented Jun 13, 2017

I did an error in the rebase work, I'll fix that soon

@adustm adustm changed the base branch from mbed-os-workshop-17q2 to master Jun 13, 2017

@adustm adustm force-pushed the adustm:STM_aes_l486rg branch Jun 19, 2017

@adustm

This comment has been minimized.

Member

adustm commented Jun 19, 2017

Hello, I think we are ready for this PR, unless anyone has anyelse to add (@RonEld @gilles-peskine-arm @yanesca , could you check if an approval is possible ?)
I have passed the mbedtls selftest (a more complete version as shown at the PR creation) for NUCLEO_L486RG and also validated that I did not break NUCLEO_F439ZI and NUCLEO_F756ZG code.

Kind regards
Armelle

@RonEld

RonEld approved these changes Jun 19, 2017

@yanesca

Looks good to me!

@adbridge

This comment has been minimized.

Contributor

adbridge commented Jun 23, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 23, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 625

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 27, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 642

Test failed!

@adustm

This comment has been minimized.

Member

adustm commented Jun 27, 2017

Hi,
another morph test failure, with another board build failure... I think it's not related to this PR.
Either somebody solves the morph test unstability, or somebody should accept to bypass the broken morph test...

Kind regards
Armelle
cc @screamerbg

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

@adustm I'll rerun the tests, as you are correct, your PR should not be blocked by an unrelated timing failure.

/morph test

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

@Patater Could you restart this one too?

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 28, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 669

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jun 28, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 28, 2017

@adustm I merged #4610 And it created a merge conflict for this PR. Sorry about that. Could you rebase?

@adustm

This comment has been minimized.

Member

adustm commented Jun 29, 2017

Solving conflict

@adustm adustm force-pushed the adustm:STM_aes_l486rg branch to e2c96e9 Jun 29, 2017

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Jun 29, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 30, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 30, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 712

All builds and test passed!

@theotherjimmy theotherjimmy merged commit dea489e into ARMmbed:master Jul 6, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adbridge

This comment has been minimized.

Contributor

adbridge commented Jul 14, 2017

This has an empty commit which probably should have been picked up during the review stage before it was merged!

@adustm adustm deleted the adustm:STM_aes_l486rg branch Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment