NUCLEO_F756ZG / mbedTLS: MD5 hw acceleration #4156

adustm · 2017-04-11T14:41:08Z

Description

mbedtls_MD5 hw acceleration for NUCLEO_F756ZG

Status

READY

Related PRs

This PR is the same as #3956, but based on another mbed-os branch

** This PR needs #3954 + #3950 to be merged first**

Steps to test or reproduce

same as in #3950

adustm · 2017-04-11T14:43:59Z

@sg-

RonEld

I have done my review, and added some comments\questions

RonEld · 2017-04-12T09:56:35Z

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


+#define MBEDTLS_MD5_C


Please remove

@adustm I am not sure this was removed

RonEld · 2017-04-12T10:09:13Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

+ */
+void mbedtls_md5_update( mbedtls_md5_context *ctx, const unsigned char *input, size_t ilen )
+{
+    HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, (uint8_t *)input, ilen);


what if ilen < 64?
Don't you need some accumulation buffer like in the SHA1 use case? Is this done in HAL_HASH_MD5_Accumulate?

I'll check that

RonEld · 2017-04-12T10:09:45Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

+{
+    __HAL_HASH_START_DIGEST();
+
+    HAL_HASH_MD5_Finish(&ctx->hhash_md5, output, 10);


why 10? Is this hex value?

Disregard, I understand now this is timeout value, and not length

This is a timeout value.

RonEld · 2017-04-12T10:12:00Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+ */
+typedef struct
+{
+    uint32_t total[2];          /*!< number of bytes processed  */


where is this referenced?

ok to remove. They are unused

RonEld · 2017-04-12T10:12:05Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+typedef struct
+{
+    uint32_t total[2];          /*!< number of bytes processed  */
+    uint32_t state[4];          /*!< intermediate digest state  */


where is this referenced?

ok to remove. They are unused

RonEld · 2017-04-12T10:12:11Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+{
+    uint32_t total[2];          /*!< number of bytes processed  */
+    uint32_t state[4];          /*!< intermediate digest state  */
+    unsigned char buffer[64];   /*!< data block being processed */


where is this referenced?

ok to remove. They are unused

RonEld · 2017-04-12T10:14:30Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+
+#if defined(MBEDTLS_MD5_ALT)
+#include "mbedtls/platform.h"
+#include "mbedtls/config.h"


is this include needed? isn't it already included in md5.h and md5.c before the inclusion of md5_alt.h?
Please check what header files inclusions are needed in this header files, and what should be moved to md5_alt.c file

RonEld · 2017-04-12T10:14:51Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+#endif
+
+#ifdef __cplusplus
+extern "C" {


can be removed

RonEld · 2017-04-12T10:14:56Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+#endif
+
+#ifdef __cplusplus
+}


can be removed

RonEld

Accidently commented here instead of 4157

sg- · 2017-05-16T08:27:45Z

/morph test-nightly

mbed-bot · 2017-05-16T20:53:45Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 230

Test failed!

…fdef __cplusplus

+ remove unused includes files

adustm · 2017-05-23T11:45:41Z

Hello, could you let me know what is wrong with the

Cam-CI uvisor Build & Test

, please ?
Thanks in advance
Armelle

adustm · 2017-05-23T12:17:31Z

Hello @yanesca, you have been added as a reviewer by @0xc0170 .
Could you also review this PR, please ?

Thanks in advance,
Armelle

RonEld

one small comment

RonEld · 2017-05-23T12:25:45Z

features/mbedtls/targets/TARGET_STM/md5_alt.h

+void mbedtls_md5_finish( mbedtls_md5_context *ctx, unsigned char output[16] );
+
+/* Internal use */
+void mbedtls_md5_process( mbedtls_md5_context *ctx, const unsigned char data[64] );


change 64 to MBEDTLS_MD5_BLOCK_SIZE

gilles-peskine-arm

According to my interpretation of the HAL documentation, mbedtls_md5_update will not work with input buffers of size that is not a multiple of 64. This could be tested with ad hoc tests, or by performing an RSA PSS signature off-device and checking it on the device.

gilles-peskine-arm · 2017-05-23T15:39:50Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

+        mbedtls_md5_process(ctx, ctx->sbuf);
+        // now process every input as long as it is %4 bytes
+        size_t iter = currentlen / 4;
+        HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, (uint8_t *)(input+MBEDTLS_MD5_BLOCK_SIZE-ctx->sbuf_len), (iter*4));


The documentation of HAL_HASH_MD5_Accumulate states:

If the Size is multiple of 64 bytes, appending the input buffer is possible.
If the Size is not multiple of 64 bytes, the padding is managed by hardware and appending the input buffer is no more possible.

So I don't think this code is correct: it can process blocks of 4 bytes. My understanding is that every block other than the last block must have a size that is a multiple of 64.

The documentation is slightly different for SHA-1 and SHA-256, see https://github.com/ARMmbed/mbed-os/pull/4159/files#r118019139

andresag01 · 2017-05-24T10:14:11Z

Same come comment as @gilles-peskine-arm. Otherwise same comments as #4162.

# Conflicts: # features/mbedtls/targets/TARGET_STM/md5_alt.h

adustm · 2017-06-12T13:56:16Z

Hello,
@yanesca, @gilles-peskine-arm, could you approve this PR or let me know whait is missing ?

@0xc0170 do you know what is failing in the Cam-CI uvisor Build & Test script ?
Kind regards
Armelle

0xc0170 · 2017-06-12T15:09:37Z

@0xc0170 do you know what is failing in the Cam-CI uvisor Build & Test script ?

It's uvisor related issue , @Patater can you please help?

Anyway, this is sent to workshop branch, the most of work was already pushed to master, shall this be rebased and redirected to master? @sbutcher-arm @sg- what do you think ? We got couple of similar PR.

Patater · 2017-06-12T16:18:45Z

@0xc0170

If this PR is rebased and redirected to master, the uVisor CI will have a chance of passing. As of now, the workshop branch is too old to work with uVisor CI due to uVisor tests needing RTX5.

adustm · 2017-06-13T06:48:37Z

Hello,
I can try to rebase on master for this PR and we'll see if it works before I do the same for the 6 other PRs.
Kind regards
Armelle

adustm · 2017-06-13T07:34:04Z

Hello @0xc0170 @Patater

I cannot rebase, as the workshop branch is far from the master branch. You first have to merge the workshop branch in the master branch.
At least I am missing PR #4161 in the master branch
Do you agree ?

Or would it be acceptable that you bypass the Cam-CI uvisor Build & Test check when you merge in a branch that is not the master ? This check will be repassed when the workshop branch will be merged into the master branch anyway?

Kind regards

adustm · 2017-06-23T14:57:21Z

Hello @adbridge @0xc0170
Any idea when this will be integrated in the master branch ?

cc @screamerbg

adbridge · 2017-06-27T11:05:46Z

@adustm This PR needs to be redirected to master and then rebased once done (it is still raised against the workshop branch.)

0xc0170 requested review from yanesca and RonEld April 12, 2017 09:49

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

RonEld suggested changes Apr 12, 2017

View reviewed changes

adustm force-pushed the STM_md5_f756zg branch 2 times, most recently from 00f86c1 to 13561ef Compare May 15, 2017 12:51

adustm added 9 commits May 22, 2017 11:25

mbedtls/F439ZI: add md5 HASH feature

8757af6

NUCLEO_F756ZG/mbedtls : MD5 hw activation

f66c41a

Move MBEDTLS_MD5_C define from mbedtls_device.h to targets.json

66dae43

Remove unnecessary functions in md5_alt.h file

ce39ad9

Remove unused variables in mbedtls_md5_context + remove unnecessary i…

e3813a9

…fdef __cplusplus

Handle 64bytes per 64 bytes

7348f4a

+ remove unused includes files

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

d14c928

Improve md5 buffer storing concept

074695a

Replace 64 by a define

268eec6

adustm force-pushed the STM_md5_f756zg branch from 13561ef to 268eec6 Compare May 22, 2017 11:10

Remove MBEDTLS_MD5_C

26c3ced

RonEld reviewed May 23, 2017

View reviewed changes

adustm added 2 commits May 23, 2017 14:58

Change 64 by MBEDTLS_MD5_BLOCK_SIZE

70fb233

Remove MBEDTLS_MD5_C from F439ZI even for this STM32F7 pull request

e69f48c

RonEld approved these changes May 23, 2017

View reviewed changes

gilles-peskine-arm suggested changes May 23, 2017

View reviewed changes

gilles-peskine-arm mentioned this pull request May 23, 2017

mbedtls/F439ZI: add md5 HASH feature #4160

Merged

adustm added 2 commits June 8, 2017 14:54

Handle context swap + Modify macro name ST_MD5_BLOCK_SIZE

9906e6a

# Conflicts: # features/mbedtls/targets/TARGET_STM/md5_alt.h

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

6cd569d

andresag01 approved these changes Jun 12, 2017

View reviewed changes

0xc0170 removed the needs: review label Jun 13, 2017

0xc0170 merged commit bad936c into ARMmbed:mbed-os-workshop-17q2 Jun 13, 2017

adustm deleted the STM_md5_f756zg branch October 11, 2018 09:18


		#define MBEDTLS_MD5_C

NUCLEO_F756ZG / mbedTLS: MD5 hw acceleration #4156

NUCLEO_F756ZG / mbedTLS: MD5 hw acceleration #4156

Conversation

adustm commented Apr 11, 2017 • edited Loading

Description

Status

Related PRs

Steps to test or reproduce

adustm commented Apr 11, 2017

RonEld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld left a comment • edited Loading

Choose a reason for hiding this comment

sg- commented May 16, 2017

mbed-bot commented May 16, 2017

Output

adustm commented May 23, 2017

adustm commented May 23, 2017

RonEld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented May 24, 2017

adustm commented Jun 12, 2017

0xc0170 commented Jun 12, 2017

Patater commented Jun 12, 2017

adustm commented Jun 13, 2017

adustm commented Jun 13, 2017 • edited Loading

adustm commented Jun 23, 2017

adbridge commented Jun 27, 2017

adustm commented Apr 11, 2017 •

edited

Loading

RonEld Apr 13, 2017 •

edited

Loading

RonEld left a comment •

edited

Loading

adustm commented Jun 13, 2017 •

edited

Loading