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

F756/F439: add mbedtls md5 hash feature #4695

Merged
merged 14 commits into from Aug 10, 2017

Conversation

Projects
None yet
6 participants
@0xc0170
Member

0xc0170 commented Jul 3, 2017

From workshop branch, rebased on top of master

For more info, see #4160

@adustm @RobMeades please review first

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jul 3, 2017

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

targets/targets.json Outdated
@@ -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"],

This comment has been minimized.

@adustm

adustm Jul 3, 2017

Member

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

features/mbedtls/targets/TARGET_STM/TARGET_STM32F4/TARGET_NUCLEO_F439ZI/mbedtls_device.h Outdated
@@ -26,4 +26,7 @@
#define MBEDTLS_SHA1_ALT
#define MBEDTLS_MD5_ALT
#define MBEDTLS_MD5_C

This comment has been minimized.

@adustm

adustm Jul 3, 2017

Member

You should remove this define MBEDTLS_MD5_C.

@adustm

This comment has been minimized.

Member

adustm commented Jul 3, 2017

Thank you @0xc0170

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 3, 2017

@adustm I pushed a fix, should be fine now

@adustm

adustm approved these changes Jul 4, 2017

@adustm

This comment has been minimized.

Member

adustm commented Jul 4, 2017

LGTM

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

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 10, 2017

@yanesca Could you review?

features/mbedtls/targets/TARGET_STM/TARGET_STM32F7/TARGET_NUCLEO_F756ZG/mbedtls_device.h Outdated
#ifndef MBEDTLS_DEVICE_H
#define MBEDTLS_DEVICE_H
#define MBEDTLS_AES_ALT

This comment has been minimized.

@yanesca

yanesca Jul 11, 2017

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 Jul 11, 2017

Member

@adustm Please can you comment?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 17, 2017

@adustm Could you comment on this?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 24, 2017

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 0xc0170 force-pushed the 0xc0170:fix_4160 branch Jul 26, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 26, 2017

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

This comment has been minimized.

Member

adustm commented Jul 27, 2017

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 0xc0170 changed the title from mbedtls/F439ZI: add md5 HASH feature to F756/F439: add mbedtls md5 hash feature Jul 31, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 31, 2017

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 0xc0170 force-pushed the 0xc0170:fix_4160 branch 2 times, most recently Jul 31, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 31, 2017

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

@adustm

This comment has been minimized.

Member

adustm commented Jul 31, 2017

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

@adustm

This comment has been minimized.

Member

adustm commented Aug 1, 2017

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

This comment has been minimized.

Member

adustm commented Aug 1, 2017

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 0xc0170 force-pushed the 0xc0170:fix_4160 branch Aug 2, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 2, 2017

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

@adustm

This comment has been minimized.

Member

adustm commented Aug 3, 2017

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)

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 3, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 927

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 3, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 3, 2017

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 932

Test failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 7, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 7, 2017

Result: FAILURE

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

/morph test-nightly

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_4160 branch to 744c364 Aug 9, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 9, 2017

@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

This comment has been minimized.

Member

0xc0170 commented Aug 9, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 9, 2017

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!

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

@adustm

This comment has been minimized.

Member

adustm commented Aug 10, 2017

@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

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

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test-nightly 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment