F756/F439: add mbedtls md5 hash feature #4695

0xc0170 · 2017-07-03T15:26:40Z

From workshop branch, rebased on top of master

For more info, see #4160

RobMeades · 2017-07-03T15:32:01Z

The macro MBEDTLS_CONFIG_HW_SUPPORT seems to have been added to targets.json twice, otherwise fine by me.

adustm · 2017-07-03T15:49:13Z

targets/targets.json

@@ -1066,6 +1066,7 @@
        "inherits": ["FAMILY_STM32"],
        "core": "Cortex-M7F",
        "extra_labels_add": ["STM32F7", "STM32F756", "STM32F756xG", "STM32F756ZG"],
+        "macros_add": ["MBEDTLS_CONFIG_HW_SUPPORT", "MBEDTLS_MD5_C", "MBEDTLS_CONFIG_HW_SUPPORT"],


Hi, This should be :
"macros_add": ["USBHOST_OTHER","MBEDTLS_CONFIG_HW_SUPPORT"],
The MBEDTLS_MD5_C should be added in the mbed_app.json if needed.
Cheers

adustm · 2017-07-03T15:51:20Z

features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h

@@ -26,4 +26,7 @@

 #define MBEDTLS_SHA1_ALT

+#define MBEDTLS_MD5_ALT
+
+#define MBEDTLS_MD5_C


You should remove this define MBEDTLS_MD5_C.

adustm · 2017-07-03T15:54:11Z

Thank you @0xc0170

0xc0170 · 2017-07-03T17:54:57Z

@adustm I pushed a fix, should be fine now

adustm · 2017-07-04T06:31:35Z

LGTM

theotherjimmy · 2017-07-10T16:41:22Z

@yanesca Could you review?

yanesca · 2017-07-11T09:48:41Z

features/mbedtls/targets/TARGET_STM/TARGET_STM32F7/TARGET_NUCLEO_F756ZG/mbedtls_device.h

+#ifndef MBEDTLS_DEVICE_H
+#define MBEDTLS_DEVICE_H
+
+#define MBEDTLS_AES_ALT


The PR title suggests that it adds MD5 support for F439ZI. This file adds both AES and MD5 support for F756ZG. Is this intentional?

@adustm Please can you comment?

theotherjimmy · 2017-07-17T15:44:21Z

@adustm Could you comment on this?

0xc0170 · 2017-07-24T14:13:17Z

The PR title suggests that it adds MD5 support for F439ZI. This file adds both AES and MD5 support for F756ZG. Is this intentional?

@adustm @LMESTM @jeromecoutant @bcostm Can anyone commment on this?

0xc0170 · 2017-07-26T12:05:21Z

rebased to resolve conflict

The PR title suggests that it adds MD5 support for F439ZI. This file adds both AES and MD5 support for F756ZG. Is this intentional?

@adustm @LMESTM @jeromecoutant @bcostm Can anyone commment on this?

should I remove MBEDTLS_AES_ALT ?

adustm · 2017-07-27T08:05:20Z

Hello Martin,
MD5 code is compatible for both F756ZG and F439ZI.
I suggest you rename the PR to F756/F439 add mbedtls_MD5 feature, and only add MBEDTLS_MD5_ALT in both targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h and targets/TARGET_STM/TARGET_STM32F7/TARGET_NUCLEO_F756ZG/mbedtls_device.h.

This will avoid us to create another PR for F756ZG later on.

MBEDTLS_AES_ALT for F756ZG is already present in the master branch. It should not be removed by your PR.

Kind regards

Armelle

0xc0170 · 2017-07-31T13:36:46Z

Thanks @adustm . I renamed the PR. I noticed after reading your reply, that the rebase did not end up with the correct result. features/mbedtls/targets/TARGET_STM/TARGET_STM32F7/TARGST_NUCLEO_F756ZG/mbedtls_device.h (this path is not correct), I'll try to correct the rebase again and fix it

0xc0170 · 2017-07-31T13:50:28Z

@adustm Rebased, please review (if possible to test?)

adustm · 2017-07-31T15:36:02Z

Hi, I'll try to spend time on it tomorrow. cheers

adustm · 2017-08-01T09:17:50Z

Hi Martin, the MD5 test is failing in the master branch, but is still ok in the former workshop branch. I'm trying to figure out why.

Cheers
Armelle

adustm · 2017-08-01T13:13:52Z

Hello @0xc0170
I found the issue. I have a complete mbedtls-selftest that launches SHA256 then MD5 selftest. The SHA256 algorithm mode is kept in the hw memory (the 3 of them are sharing the same HW block).

It means that MD5 can work in standalone as it is today, but not if some SHA256 has been formerly enabled.
Either you add 1 line in the MD5_alt.c file otherwise the HW will keep the SHA256 algorithm mode.
md5_alt.c, line 103, add :

     /* HASH Configuration */
     ctx->hhash_md5.Init.DataType = HASH_DATATYPE_8B;
+    /* clear CR ALGO value */
+    HASH->CR &= ~HASH_CR_ALGO_Msk;
     if (HAL_HASH_Init(&ctx->hhash_md5) != 0) {
         // return error code
         return;

Or I wait for your PR to be merged, and I create a PR to fix that in md5 / SHA1 and SHA256 alt files (the 3 of them have the same problem).

What do you prefer ?
For your information, with the above modification, it gives :

+-----------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| target                | platform_name | test suite             | test case                   | passed | failed | result | elapsed_time (sec) |
+-----------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.56               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 3.15               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.08               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 2.6                |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_md5_self_test       | 1      | 0      | OK     | 0.85               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.26               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.35               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 0.27               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 2.16               |
+-----------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+

Kind regards
Armelle

0xc0170 · 2017-08-02T13:06:04Z

@adustm Fixed as proposed (check the last commit, 3 files were updated), rebased again.

adustm · 2017-08-03T08:41:58Z

Thanks @0xc0170
LGTM

+-----------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| target                | platform_name | test suite             | test case                   | passed | failed | result | elapsed_time (sec) |
+-----------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.55               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.13               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 2.97               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.09               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 2.62               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.26               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.39               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 0.27               |
| NUCLEO_F439ZI-ARM     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 1.62               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.54               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 2.87               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.08               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 2.6                |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.25               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.34               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 0.26               |
| NUCLEO_F439ZI-IAR     | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 1.71               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.56               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.13               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.12               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 3.16               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.08               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 2.6                |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.06               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_md5_self_test       | 1      | 0      | OK     | 0.85               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.25               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.36               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 0.27               |
| NUCLEO_F439ZI-GCC_ARM | NUCLEO_F439ZI | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 2.16               |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+

(as stated above, I use a more complete version of the mbedtls-selftest to check all these)

0xc0170 · 2017-08-03T08:55:22Z

/morph test-nightly

mbed-bot · 2017-08-03T14:58:48Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 927

Test failed!

0xc0170 · 2017-08-03T15:32:58Z

/morph test-nightly

mbed-bot · 2017-08-03T22:48:32Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 932

Test failed!

0xc0170 · 2017-08-07T13:29:25Z

/morph test-nightly

mbed-bot · 2017-08-07T19:37:24Z

Result: FAILURE

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

/morph test-nightly

…fdef __cplusplus

+ remove unused includes files

The hw block for mbedtls is shared, thus HASH algo value should be cleared in the init.

0xc0170 · 2017-08-09T06:47:47Z

@adustm There was a change in mbedtls config for F7 nucleo again, I had to rebase, resolve conflicts. I'll keep an eye not to do any changes to related files here until this one gets it otherwise it wont.

0xc0170 · 2017-08-09T06:47:53Z

/morph test-nightly

mbed-bot · 2017-08-09T16:11:03Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 974

All builds and test passed!

adustm · 2017-08-10T08:09:09Z

@0xc0170

There was a change in mbedtls config for F7 nucleo again, I had to rebase, resolve conflicts.

Rebasing / resolve conflicts is what I am doing since the creation of the 8 mbedtls hw acceleration in April... thank you for your effort

0xc0170 added the needs: review label Jul 3, 2017

adustm reviewed Jul 3, 2017

View reviewed changes

adustm approved these changes Jul 4, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jul 4, 2017

RobMeades approved these changes Jul 6, 2017

View reviewed changes

theotherjimmy added needs: review and removed needs: CI labels Jul 10, 2017

yanesca reviewed Jul 11, 2017

View reviewed changes

0xc0170 force-pushed the fix_4160 branch from 2db3e45 to 83c8653 Compare July 26, 2017 12:03

0xc0170 changed the title ~~mbedtls/F439ZI: add md5 HASH feature~~ F756/F439: add mbedtls md5 hash feature Jul 31, 2017

0xc0170 force-pushed the fix_4160 branch 2 times, most recently from b645fcb to f6f787e Compare July 31, 2017 13:49

0xc0170 force-pushed the fix_4160 branch from f6f787e to 72b6959 Compare August 2, 2017 13:05

adustm and others added 14 commits August 9, 2017 07:45

NUCLEO_F756ZG/mbedtls : MD5 hw activation

bd1c4f5

Move MBEDTLS_MD5_C define from mbedtls_device.h to targets.json

c174191

Remove unnecessary functions in md5_alt.h file

e126975

Remove unused variables in mbedtls_md5_context + remove unnecessary i…

29114f1

…fdef __cplusplus

Handle 64bytes per 64 bytes

b2092f0

+ remove unused includes files

Fix use case with size = 0 (md5_selftest #1)

d39d52b

Improve md5 buffer storing concept

53a8b75

Replace 64 by a define

766e451

Move MBEDTLS_MD5_C from mbetdls_device.h to targets.json

de28992

Remove MBEDTLS_MD5_C

9572183

Handle context swap + Modify macro name ST_MD5_BLOCK_SIZE

f033c87

Add a check 'non busy' status of the HW before save restore procedures

85c8bf8

STM32F4: remove md5 from the mbedtls config file

d92e4b5

STM mbedtls: clear algo value for md5/sha1 and sha256

744c364

The hw block for mbedtls is shared, thus HASH algo value should be cleared in the init.

0xc0170 force-pushed the fix_4160 branch from 72b6959 to 744c364 Compare August 9, 2017 06:46

0xc0170 added ready for merge and removed needs: CI labels Aug 9, 2017

theotherjimmy merged commit 9edd0e8 into ARMmbed:master Aug 10, 2017

theotherjimmy added release-version: 5.5.5 and removed ready for merge labels Aug 10, 2017

F756/F439: add mbedtls md5 hash feature #4695

F756/F439: add mbedtls md5 hash feature #4695

Uh oh!

Conversation

0xc0170 commented Jul 3, 2017

Uh oh!

RobMeades commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adustm commented Jul 3, 2017

Uh oh!

0xc0170 commented Jul 3, 2017

Uh oh!

adustm commented Jul 4, 2017

Uh oh!

theotherjimmy commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Jul 17, 2017

Uh oh!

0xc0170 commented Jul 24, 2017

Uh oh!

0xc0170 commented Jul 26, 2017

Uh oh!

adustm commented Jul 27, 2017

Uh oh!

0xc0170 commented Jul 31, 2017

Uh oh!

0xc0170 commented Jul 31, 2017

Uh oh!

adustm commented Jul 31, 2017

Uh oh!

adustm commented Aug 1, 2017

Uh oh!

adustm commented Aug 1, 2017

Uh oh!

0xc0170 commented Aug 2, 2017

Uh oh!

adustm commented Aug 3, 2017

Uh oh!

0xc0170 commented Aug 3, 2017

Uh oh!

mbed-bot commented Aug 3, 2017

Output

Uh oh!

0xc0170 commented Aug 3, 2017

Uh oh!

mbed-bot commented Aug 3, 2017

Output

Uh oh!

0xc0170 commented Aug 7, 2017

Uh oh!

mbed-bot commented Aug 7, 2017

Uh oh!

0xc0170 commented Aug 9, 2017

Uh oh!

0xc0170 commented Aug 9, 2017

Uh oh!

mbed-bot commented Aug 9, 2017

Output

Uh oh!

adustm commented Aug 10, 2017

Uh oh!

Uh oh!