NUCLEO_F439ZI/mbedtls: add SHA256 hw_acceleration #4159

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

Description

Enable HW acceleration for SHA256 algorithm on STM32F439ZI
is = #3948

Status

READY

Steps to test or reproduce

To test this feature, you have to modify TESTS/mbedtls/selfttest/main.cpp in order to call sha256 self test:
add:
#include "mbedtls/sha256.h"
then

#if defined(MBEDTLS_SHA256_C)
MBEDTLS_SELF_TEST_TEST_CASE(mbedtls_sha256_self_test)
#endif

then

#if defined(MBEDTLS_SHA256_C)
    Case("mbedtls_sha256_self_test", mbedtls_sha256_self_test_test_case),
#endif

Testing

/morph test-nightly

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

@sg-

RonEld

reviewed and added comments

RonEld · 2017-04-12T10:23:43Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+void mbedtls_sha256_update( mbedtls_sha256_context *ctx, const unsigned char *input, size_t ilen )
+{
+    if (ctx->is224 == 0)
+        HAL_HASHEx_SHA256_Accumulate(&ctx->hhash_sha256, (uint8_t *)input, ilen);


what if ilen < 64?
Do you need an accumulation buffer like in SHA1 use case?

RonEld · 2017-04-12T10:25:42Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

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


is this include needed? Please check what header file includes can be moved to mbedtls_sha256_alt.c

RonEld · 2017-04-12T10:27:10Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

+ * \param output   SHA-224/256 checksum result
+ * \param is224    0 = use SHA256, 1 = use SHA224
+ */
+void mbedtls_sha256( const unsigned char *input, size_t ilen,


no need to define, as it is already defined in sha256.h outside the #ifdef (MBEDTLS_SHA256_ALT) check

RonEld · 2017-04-12T10:27:16Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

+ *
+ * \return         0 if successful, or 1 if the test failed
+ */
+int mbedtls_sha256_self_test( int verbose );


no need to define, as it is already defined in sha256.h outside the #ifdef (MBEDTLS_SHA256_ALT) check

RonEld · 2017-04-12T10:27:25Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

+int mbedtls_sha256_self_test( int verbose );
+
+#ifdef __cplusplus
+}


can be removed

RonEld · 2017-04-12T10:27:35Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

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


can be removed

0xc0170 · 2017-05-16T08:24:25Z

retest uvisor

adustm · 2017-05-23T12:15:53Z

Hello @RonEld
I just implemented SHA256 in the same way it was done for SHA1 and MD5.
Could you handle the review please ?
Cheers
Armelle

RonEld

minor comment, other than that, I will approve

RonEld · 2017-05-23T12:18:35Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+    }
+}
+
+void mbedtls_sha256_process( mbedtls_sha256_context *ctx, const unsigned char data[64] )


change 64 to MBEDTLS_SHA256_BLOCK_SIZE

RonEld · 2017-05-23T12:18:41Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+void mbedtls_sha256_process( mbedtls_sha256_context *ctx, const unsigned char data[64] )
+{
+    if (ctx->is224 == 0) {
+        HAL_HASHEx_SHA256_Accumulate(&ctx->hhash_sha256, (uint8_t *) data, 64);


change 64 to MBEDTLS_SHA256_BLOCK_SIZE

RonEld · 2017-05-23T12:18:49Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+    if (ctx->is224 == 0) {
+        HAL_HASHEx_SHA256_Accumulate(&ctx->hhash_sha256, (uint8_t *) data, 64);
+    } else {
+        HAL_HASHEx_SHA224_Accumulate(&ctx->hhash_sha256, (uint8_t *) data, 64);


change 64 to MBEDTLS_SHA256_BLOCK_SIZE

RonEld · 2017-05-23T12:19:56Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

+void mbedtls_sha256_finish( mbedtls_sha256_context *ctx, unsigned char output[32] );
+
+/* Internal use */
+void mbedtls_sha256_process( mbedtls_sha256_context *ctx, const unsigned char data[64] );


change 64 to MBEDTLS_SHA256_BLOCK_SIZE

gilles-peskine-arm · 2017-05-23T14:20:00Z

features/mbedtls/targets/TARGET_STM/sha256_alt.h

+ */
+typedef struct
+{
+    int is224;                  /*!< 0 => SHA-256, else SHA-224 */  


I suggest making this a bool (so that it's clear that only two values are allowed) and moving it to the end of the structure (to avoid wasting a little memory for alignment).

Hello,
This is coming from the official sha256.h.
I suggest that you create a change request to mbedtls official release, where it comes from :
https://github.com/ARMmbed/mbed-os/blob/master/features/mbedtls/inc/mbedtls/sha256.h#L51

@adustm I know. We're stuck with this in the portable code because we don't want to change types that are visible in the public API (it would break binary compatibility). Here the backward compatibility constraint does not apply. But it's extremely minor anyway, it would save 4 bytes per context at most.

This is a copy from the input parameter from the void mbedtls_sha256_starts( mbedtls_sha256_context *ctx, int is224 ) function.
I prefer to remain aligned with the public api if you don't mind (as you said, minor 4 bytes saving).

gilles-peskine-arm · 2017-05-23T14:48:14Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+void mbedtls_sha256_clone( mbedtls_sha256_context *dst,
+                         const mbedtls_sha256_context *src )
+{
+    *dst = *src;


I'm not sure this is correct, but again it might be because I am not familiar with the hardware. After calling the clone function, a user of the crypto library can alternate calls to the two contexts. If the context data is copied, in particular if the clones have the same contents in all the HASH_HandleTypeDef fields, won't they be sharing some resources?

see previous comment

gilles-peskine-arm · 2017-05-23T15:08:02Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+void mbedtls_sha256_update( mbedtls_sha256_context *ctx, const unsigned char *input, size_t ilen )
+{
+    size_t currentlen = ilen;
+    // store mechanism to handle MBEDTLS_SHA256_BLOCK_SIZE bytes per MBEDTLS_SHA256_BLOCK_SIZE bytes


I guess you mean “to handle MBEDTLS_SHA256_BLOCK_SIZE bytes at a time”? But looking at the code below it's a bit more complicated than that: buffer up to MBEDTLS_SHA256_BLOCK_SIZE bytes; only call the hardware if there are at least MBEDTLS_SHA256_BLOCK_SIZE bytes to process; if there are at least MBEDTLS_SHA256_BLOCK_SIZE bytes then process the largest possible multiple of 4 (not of 64).

correct, I will change the comment

gilles-peskine-arm · 2017-05-23T15:13:30Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+    if( ctx == NULL )
+        return;
+    /* Force the HASH Periheral Clock Reset */
+    __HAL_RCC_HASH_FORCE_RESET();


Is this the right place to reset the hardware? What about a scenario with two hash calculations, fore example: init(ctx1); init(ctx2); starts(ctx1); starts(ctx2); update(ctx1); update(ctx2); finish(ctx1); update(ctx2); free(ctx1); update(ctx2);. The free operation is there to free resources that are associated with a specific calculation, but the hardware is shared between all the hash calculations that are going on.

You are probably right about this. I will create this test to handle the multithread.

gilles-peskine-arm · 2017-05-23T15:26:20Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+        // now process every input as long as it is %4 bytes
+        size_t iter = currentlen / 4;
+        if (ctx->is224 == 0) {
+            HAL_HASHEx_SHA256_Accumulate(&ctx->hhash_sha256, (uint8_t *)(input + MBEDTLS_SHA256_BLOCK_SIZE - ctx->sbuf_len),  (iter * 4));


The documentation of HAL_HASHEx_SHA256_Accumulate states: “If the Size is not multiple of 64 bytes, the padding is managed by hardware.” So is it valid to call it with blocks that are not a multiple of 64 bytes (other than the last block)? E.g. with a call mbedtls_sha256_update(ctx, input1, 68); mbedtls_sha256_update(ctx, input2, 60); mbedtls_sha256_finalize(ctx), this code processes all 68 bytes of input1 then some padding?? then buffers the 60 bytes of input2 which are processed by finalize(). Is this correct?

Same remark for SHA224.

Please see my answer here : #4162 (comment)

gilles-peskine-arm · 2017-05-23T15:29:19Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

+    mbedtls_zeroize( ctx, sizeof( mbedtls_sha256_context ) );
+
+    /* Enable HASH clock */
+    __HAL_RCC_HASH_CLK_ENABLE();


Is this the right place to power on the hardware? See my comment about reset in mbedtls_sha256_free.

Powering the hw will have no effect if already done.

andresag01 · 2017-05-23T16:00:13Z

This is equivalent to PR #4162 but for a different target. I have already reviewed that one and left my comments there.

adustm · 2017-06-02T15:54:52Z

Hello @gilles-peskine-arm @andresag01 ,
I have reworked this branch to allow multiple context.
Let me know if you think it's ok.
Kind regards

gilles-peskine-arm

Looks good overall, see my comments in PR #4162

andresag01

@adustm: Many thanks for considering our comments and reworking. This PR is is very similar to #4160, please refer to my most recent comments there.

adustm · 2017-06-12T14:15:28Z

Hello @andresag01 and @yanesca
Could you approve this PR or let me know if anything is missing ?
@0xc0170 could you let me know what is wrong in the Cam-CI uvisor Build & Test script ?

Kind regards
Armelle

sg- · 2017-06-15T14:59:47Z

Given all the approvals and that mbed TLS 2.5 is part of master can this PR be rebased on master?

0xc0170 · 2017-06-16T08:33:09Z

@adustm Is there any problem to base this on master? It was changed a hour ago

adustm · 2017-06-16T08:37:03Z

Hello @0xc0170 , forget about it, I have seen just after that it was a modification from Sam.
The rebase on master shows many conflicts. I think I 'd better cherry-pick, isn't it ?

This will solve Size <4 issues

adustm · 2017-06-22T11:46:57Z

bump ?

adbridge · 2017-06-23T10:27:24Z

/morph test

mbed-bot · 2017-06-23T15:13:08Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 623

All builds and test passed!

adustm · 2017-06-23T15:48:08Z

It looks like this morph test was lucky and the other were not...

At least this one has a chance to be merged soon
:-s
kind regards
Armelle
cc @screamerbg

screamerbg · 2017-06-26T15:20:59Z

@0xc0170 @adbridge What is gating this PR?

0xc0170 · 2017-06-26T15:33:08Z

@adustm Can you please rebase? we merged another PR that caused a conflict here, we will then trigger CI

adustm · 2017-06-27T09:20:10Z

conflict solved

adbridge · 2017-06-27T10:11:36Z

retest uvisor

adbridge · 2017-06-27T10:12:03Z

/morph test

theotherjimmy · 2017-06-27T17:06:53Z

@adustm I have restarted Travis for you. If that does not resolve the issue we fixed on master I will rebase this PR for you to get travis passing.

@Patater You aborted this one too. Do you have plans to restart it?

mbed-bot · 2017-06-28T03:14:21Z

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 662

All builds and test passed!

adustm · 2017-06-29T08:17:37Z

👍

adbridge · 2017-07-03T10:29:16Z

This contains a 'merge' commit which should not have been allowed through.

adbridge · 2017-07-03T11:23:40Z

@adustm Please always use rebase rather than merge in the future :) . I will manually cherry pick changes to pull this through to 5.5.2

adustm · 2017-07-03T11:25:14Z

Hello @adbridge
Thank you.
For your information, this merge commit ec72ac0 is done through the github web interface when it shows a conflict and suggests to fix it directly there.
If it is a problem to use that option, somebody should remove this possibility.
Cheers
Armelle

adbridge · 2017-07-03T13:03:51Z

@adustm I agree. We will look into what options we allow from the git gui on these PRs !

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

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

RonEld suggested changes Apr 12, 2017

View reviewed changes

adustm force-pushed the STM_sha256_F439ZI branch from ece7870 to 959ecce Compare May 23, 2017 09:39

RonEld reviewed May 23, 2017

View reviewed changes

RonEld approved these changes May 23, 2017

View reviewed changes

gilles-peskine-arm reviewed May 23, 2017

View reviewed changes

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

NUCLEO_F756ZG/mbedtls: Add hw acceleration for SHA256 #4162

Merged

gilles-peskine-arm approved these changes Jun 6, 2017

View reviewed changes

andresag01 reviewed Jun 6, 2017

View reviewed changes

andresag01 approved these changes Jun 12, 2017

View reviewed changes

yanesca approved these changes Jun 13, 2017

View reviewed changes

sg- changed the base branch from mbed-os-workshop-17q2 to master June 15, 2017 14:59

adustm changed the base branch from master to mbed-os-workshop-17q2 June 16, 2017 07:22

adustm changed the base branch from mbed-os-workshop-17q2 to master June 16, 2017 08:35

adustm added 3 commits June 16, 2017 10:43

NUCLEO_F429ZI/mbedtls: add SHA256 hw_acceleration

0805876

Align SHA256 with MD5 and SHA1 implementation

4976e2f

This will solve Size <4 issues

Replace 64 by MBEDTLS_SHA256_BLOCK_SIZE

b929b54

theotherjimmy removed the needs: review label Jun 19, 2017

Merge branch 'master' into STM_sha256_F439ZI

ec72ac0

adbridge closed this Jun 27, 2017

adbridge reopened this Jun 27, 2017

sg- removed the needs: CI label Jun 27, 2017

adbridge added the needs: CI label Jun 27, 2017

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

theotherjimmy merged commit a39ac60 into ARMmbed:master Jun 28, 2017

theotherjimmy added release-version: 5.5.2 and removed ready for merge labels Jun 28, 2017

adustm deleted the STM_sha256_F439ZI branch June 29, 2017 08:17

adbridge removed the release-version: 5.5.2 label Jul 3, 2017

adbridge added the release-version: 5.5.2 label Jul 3, 2017

NUCLEO_F439ZI/mbedtls: add SHA256 hw_acceleration #4159

NUCLEO_F439ZI/mbedtls: add SHA256 hw_acceleration #4159

Conversation

adustm commented Apr 11, 2017 • edited by theotherjimmy Loading

Description

Status

Steps to test or reproduce

Testing

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

0xc0170 commented May 16, 2017

adustm commented May 23, 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

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

andresag01 commented May 23, 2017

adustm commented Jun 2, 2017

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

andresag01 left a comment

Choose a reason for hiding this comment

adustm commented Jun 12, 2017

sg- commented Jun 15, 2017

0xc0170 commented Jun 16, 2017

adustm commented Jun 16, 2017

adustm commented Jun 22, 2017

adbridge commented Jun 23, 2017

mbed-bot commented Jun 23, 2017

Output

adustm commented Jun 23, 2017

screamerbg commented Jun 26, 2017

0xc0170 commented Jun 26, 2017

adustm commented Jun 27, 2017

adbridge commented Jun 27, 2017

adbridge commented Jun 27, 2017

theotherjimmy commented Jun 27, 2017

mbed-bot commented Jun 28, 2017

Output

adustm commented Jun 29, 2017

adbridge commented Jul 3, 2017

adbridge commented Jul 3, 2017

adustm commented Jul 3, 2017

adbridge commented Jul 3, 2017

adustm commented Apr 11, 2017 •

edited by theotherjimmy

Loading