NUCLEO_F756ZG / mbedTLS_SHA1 hw acceleration #3957

adustm · 2017-03-16T17:40:53Z

Description

mbedtls_SHA1 hw acceleration for NUCLEO_F756ZG

Status

READY

Related PRs

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

Deploy notes

Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

same as in #3947

adustm · 2017-03-16T17:41:27Z

+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| target            | platform_name | test suite             | test case                   | passed | failed | result | elapsed_time (sec) |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_aes_self_test       | 1      | 0      | OK     | 0.47               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_base64_self_test    | 1      | 0      | OK     | 0.12               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_ccm_self_test       | 1      | 0      | OK     | 0.12               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_ctr_drbg_self_test  | 1      | 0      | OK     | 0.12               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_ecp_self_test       | 1      | 0      | OK     | 2.01               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_entropy_self_test   | 1      | 0      | OK     | 0.08               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_gcm_self_test       | 1      | 0      | OK     | 2.6                |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_hmac_drbg_self_test | 1      | 0      | OK     | 0.13               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_mpi_self_test       | 1      | 0      | OK     | 0.25               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_rsa_self_test       | 1      | 0      | OK     | 0.34               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_sha1_self_test      | 1      | 0      | OK     | 0.14               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_sha256_self_test    | 1      | 0      | OK     | 0.47               |
| NUCLEO_F756ZG-IAR | NUCLEO_F756ZG | tests-mbedtls-selftest | mbedtls_sha512_self_test    | 1      | 0      | OK     | 0.83               |
+-------------------+---------------+------------------------+-----------------------------+--------+--------+--------+--------------------+

RonEld

It seems there are some memory leaks here
user defined configuration should not be defined in mbedtls_device.h

RonEld · 2017-03-29T16:24:45Z

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


-
+#define MBEDTLS_SHA1_C


This is an application specific definition, and should be defined by the end user. Please remove it

OK, I'll put that into targets.json

RonEld · 2017-03-29T16:25:19Z

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


This adds alternative AES support, but this PR is for SHA1 only.

Same comment as in PR for MD5. I based my PR on AES' one because I think it causes merge issues if I create new folders and new files with the same names in separate PRs, sorry for that.

RonEld · 2017-03-29T16:25:48Z

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

+#define MBEDTLS_AES_ALT
+#define MBEDTLS_SHA1_ALT
+
+#define MBEDTLS_SHA1_C


This is an application specific definition, and should be defined by the end user. Please remove it

OK, I'll put that into targets.json

RonEld · 2017-03-29T16:31:58Z

features/mbedtls/targets/TARGET_STM/sha1_alt.c

+
+    if (ilen <4)
+    {
+        ctx->sbuf=malloc(ilen);


looks like a memory leak:

where is this being released

even if it is being released in an internal API, if a user is calling twice to mbedtls_sha1_update with size smaller than 4, the first allocated buffer will not be freed

Thank you for noticing this. I'll work on that

RonEld · 2017-03-29T16:34:20Z

features/mbedtls/targets/TARGET_STM/sha1_alt.c

+            buf_len = ilen - modulus;
+            HAL_HASH_SHA1_Accumulate(&ctx->hhash_sha1, (uint8_t *)input, buf_len);
+            ctx->sbuf_len=modulus;
+            ctx->sbuf=malloc(ctx->sbuf_len);


looks like a memory leak:

where is this being released

even if it is being released in an internal API, if a user is calling twice to mbedtls_sha1_update with size which modulus is not 4, the first allocated buffer will not be freed

Thank you for noticing this. I'll work on that

RonEld · 2017-03-29T16:35:20Z

features/mbedtls/targets/TARGET_STM/sha1_alt.c

+                HAL_HASH_SHA1_Accumulate(&ctx->hhash_sha1, (uint8_t *)input, ilen);
+            else
+            {
+                intermediate_buf=malloc(ilen+ctx->sbuf_len);


looks like a memory leak - > where is it freed?

Thank you for noticing this. I'll work on that

RonEld · 2017-03-29T16:36:30Z

features/mbedtls/targets/TARGET_STM/sha1_alt.h

+#if defined MBEDTLS_SHA1_ALT
+
+#include "mbedtls/platform.h"
+#include "mbedtls/config.h"


no need to include this file, as it should be included in the c file before this header file is included

mbedtls/config.h not needed. Correct

RonEld · 2017-03-29T16:37:13Z

features/mbedtls/targets/TARGET_STM/sha1_alt.h

+
+#if defined MBEDTLS_SHA1_ALT
+
+#include "mbedtls/platform.h"


Is this file needed to be included in this header file?

It is needed to avoid warnings that 'malloc' is declared implicitly. I move it to sha1_alt.c instead

RonEld · 2017-03-29T16:39:25Z

features/mbedtls/targets/TARGET_STM/sha1_alt.h

+ * \param ilen     length of the input data
+ * \param output   SHA-1 checksum result
+ */
+void mbedtls_sha1( const unsigned char *input, size_t ilen, unsigned char output[20] );


no need to redefine this function, as it is defined in sha1.h, outside the #ifdef MBEDTLS_SHA1_ALT check

RonEld · 2017-03-29T16:39:34Z

features/mbedtls/targets/TARGET_STM/sha1_alt.h

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


no need to redefine this function, as it is defined in sha1.h, outside the #ifdef MBEDTLS_SHA1_ALT check

Move include platform from sha1_alt.h to sha1_alt.c

adustm · 2017-04-07T12:44:59Z

Hello @RonEld
I think I reworked every comment you had (targets.json appart: discussion on going in #3956)
Could you check the result, please ?
Is it ok if I use realloc function in commit 97fc115 ?

kind regards

sg- · 2017-04-10T16:33:06Z

@adustm Should this be moved against the workshop branch?

adustm added 4 commits March 16, 2017 17:27

Add AES HW encryption for NUCLEO_F756ZG in mbedtls

2379402

fix typo in directory name

3541939

NUCLEO_F429ZI/mbedtls: add SHA1 hw_acceleration

159499b

NUCLEO_F756ZG/mbedTLS : add hw acceleration for SHA1

dbdae01

0xc0170 requested a review from simonbutcher March 20, 2017 13:34

0xc0170 added the needs: review label Mar 20, 2017

RonEld suggested changes Mar 29, 2017

View reviewed changes

adustm added 5 commits April 7, 2017 14:34

use mbedtls_zeroize instead of memset(xxx,0,xxx)

ee3bf9c

Better explanation of the use of mbedtls_sha1_context fields

a552bb6

Remove unneeded function declarations + include file

1d62cf9

Move include platform from sha1_alt.h to sha1_alt.c

Move MBEDTLS_SHA1_C from mbedtls_device.h to targets.json

ff3017a

Improve memory management

97fc115

adustm mentioned this pull request Apr 11, 2017

NUCLEO_F756ZG / mbedTLS_SHA1 hw acceleration #4158

Merged

adustm closed this Apr 11, 2017



		#define MBEDTLS_SHA1_C

NUCLEO_F756ZG / mbedTLS_SHA1 hw acceleration #3957

NUCLEO_F756ZG / mbedTLS_SHA1 hw acceleration #3957

Uh oh!

Conversation

adustm commented Mar 16, 2017

Description

Status

Related PRs

Deploy notes

Steps to test or reproduce

Uh oh!

adustm commented Mar 16, 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!

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!

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!

adustm commented Apr 7, 2017

Uh oh!

sg- commented Apr 10, 2017

Uh oh!

Uh oh!