STM32L486RG/mbedtls: add aes hw acceleration #4163

adustm · 2017-04-11T15:24:38Z

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 · 2017-04-11T15:24:56Z

@sg-

stevew817 · 2017-04-12T13:18:22Z

@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 · 2017-04-12T13:37:38Z

@adustm There are conflicts

RonEld

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

RonEld · 2017-04-12T15:05:55Z

features/mbedtls/targets/TARGET_STM/TARGET_STM32L4/TARGET_NUCLEO_L486RG/mbedtls_device.h

+#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


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

ok, I move them to aes_alt.h

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

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 :)

RonEld · 2017-04-12T15:06:33Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

-          break;
+#if defined (TARGET_STM32L486xG)
+            return(MBEDTLS_ERR_AES_INVALID_KEY_LENGTH);
+#else


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

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.

RonEld · 2017-04-12T15:09:48Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+        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 */


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?

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.

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

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

RonEld · 2017-04-12T15:10:17Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+             (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) 


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?

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 · 2017-04-12T15:12:40Z

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

adustm · 2017-04-13T11:50:39Z

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

stevew817 · 2017-04-13T12:31:15Z

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 · 2017-04-24T15:33:34Z

bump @adustm. Could you fix the merge conflicts?

theotherjimmy · 2017-05-02T16:00:14Z

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.

gilles-peskine-arm · 2017-05-23T13:54:38Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

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


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

gilles-peskine-arm · 2017-05-23T14:04:06Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

@@ -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)


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).

@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

@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 · 2017-06-08T14:54:40Z

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 · 2017-06-13T06:54:50Z

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

simonbutcher · 2017-06-13T12:21:54Z

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

adustm · 2017-06-13T12:44:41Z

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

adustm · 2017-06-19T12:50:12Z

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

yanesca

Looks good to me!

adbridge · 2017-06-23T10:28:22Z

/morph test

mbed-bot · 2017-06-23T16:21:33Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 625

Test failed!

theotherjimmy · 2017-06-26T15:05:24Z

/morph test

mbed-bot · 2017-06-27T04:29:53Z

Result: FAILURE

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

/morph test

Output

mbed Build Number: 642

Test failed!

adustm · 2017-06-27T06:39:16Z

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 · 2017-06-27T17:12:01Z

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

/morph test

theotherjimmy · 2017-06-27T17:12:43Z

@Patater Could you restart this one too?

mbed-bot · 2017-06-28T12:11:23Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 669

All builds and test passed!

theotherjimmy · 2017-06-28T14:27:12Z

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

adustm · 2017-06-29T09:41:43Z

Solving conflict

theotherjimmy · 2017-06-30T18:43:47Z

/morph test

mbed-bot · 2017-06-30T21:14:07Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 712

All builds and test passed!

adbridge · 2017-07-14T10:39:43Z

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

0xc0170 added the needs: review label Apr 12, 2017

0xc0170 requested review from yanesca and RonEld April 12, 2017 13:37

RonEld suggested changes Apr 12, 2017

View reviewed changes

RonEld mentioned this pull request Apr 18, 2017

Add support for Alternative CCM Mbed-TLS/mbedtls#867

Closed

gilles-peskine-arm reviewed May 23, 2017

View reviewed changes

yanesca suggested changes Jun 8, 2017

View reviewed changes

adustm force-pushed the STM_aes_l486rg branch from 2cd78d1 to e6a97f0 Compare June 12, 2017 16:07

adustm force-pushed the STM_aes_l486rg branch from e6a97f0 to c1d3755 Compare June 13, 2017 12:12

adustm changed the base branch from mbed-os-workshop-17q2 to master June 13, 2017 12:59

adustm force-pushed the STM_aes_l486rg branch from c1d3755 to eb23869 Compare June 19, 2017 12:33

RonEld approved these changes Jun 19, 2017

View reviewed changes

yanesca approved these changes Jun 19, 2017

View reviewed changes

theotherjimmy added needs: CI and removed needs: review labels Jun 19, 2017

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

theotherjimmy added needs: work and removed ready for merge labels Jun 28, 2017

adustm added 3 commits June 29, 2017 16:05

STM32L486RG/mbedtls: add aes hw acceleration

2da3128

STM32L486RG/mbedtls: add aes hw acceleration

d19c9ea

Move definitions in aes_alt.c + Factorize code

e2c96e9

adustm force-pushed the STM_aes_l486rg branch from bab71e2 to e2c96e9 Compare June 29, 2017 14:11

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

theotherjimmy added ready for merge and removed needs: CI labels Jun 30, 2017

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

theotherjimmy added release-version: 5.5.3 and removed ready for merge labels Jul 6, 2017

adustm deleted the STM_aes_l486rg branch January 15, 2018 16:18

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

STM32L486RG/mbedtls: add aes hw acceleration #4163

STM32L486RG/mbedtls: add aes hw acceleration #4163

Conversation

adustm commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Steps to test or reproduce

Uh oh!

adustm commented Apr 11, 2017

Uh oh!

stevew817 commented Apr 12, 2017

Uh oh!

0xc0170 commented Apr 12, 2017

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanesca Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adustm Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld commented Apr 12, 2017

Uh oh!

adustm commented Apr 13, 2017

Uh oh!

stevew817 commented Apr 13, 2017

Uh oh!

theotherjimmy commented Apr 24, 2017

Uh oh!

theotherjimmy commented May 2, 2017

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adustm Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

adustm commented Jun 8, 2017

Uh oh!

adustm commented Apr 11, 2017 •

edited

Loading

yanesca Jun 8, 2017 •

edited

Loading

adustm Jun 19, 2017 •

edited

Loading

adustm Jun 19, 2017 •

edited

Loading

theotherjimmy commented Jun 28, 2017 •

edited

Loading