Skip to content
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

NUCLEO_F439ZI/mbedtls: add SHA1 hw_acceleration #4157

Merged
merged 13 commits into from
Jun 26, 2017

Conversation

adustm
Copy link
Member

@adustm adustm commented Apr 11, 2017

Description

Enable SHA1 for STM32F439ZI
Enable HW acceleration for SHA1 algorithm on STM32F439ZI
is PR #3947 on another branch

Status

READY

Steps to test or reproduce

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

#if defined(MBEDTLS_SHA1_C)
MBEDTLS_SELF_TEST_TEST_CASE(mbedtls_sha1_self_test)
#endif

then

#if defined(MBEDTLS_SHA1_C)
    Case("mbedtls_sha1_self_test", mbedtls_sha1_self_test_test_case),
#endif

@adustm
Copy link
Member Author

adustm commented Apr 11, 2017

@sg-

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added same comments as #4156
In fact, the subject of this PR is misleading. The subject is SHA1 HW acceleration, but the content is MD5 HW ac celeration. Please check the mistake


#define MBEDTLS_MD5_C
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

*/
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
__HAL_HASH_START_DIGEST();

HAL_HASH_MD5_Finish(&ctx->hhash_md5, output, 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 10? Is this hex value?


#if defined(MBEDTLS_MD5_ALT)
#include "mbedtls/platform.h"
#include "mbedtls/config.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this referenced?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this referenced?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this referenced?

#endif

#ifdef __cplusplus
extern "C" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed

#endif

#ifdef __cplusplus
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed

@adustm
Copy link
Member Author

adustm commented May 18, 2017

Hello,
Sorry for the mixup between md5 and sha1 branches. I've now fixed it (back to SHA1 code).

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one additional, minor, comment. After that, It's approved from my side

void mbedtls_sha1_finish( mbedtls_sha1_context *ctx, unsigned char output[20] );

/* Internal use */
void mbedtls_sha1_process( mbedtls_sha1_context *ctx, const unsigned char data[64] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change 64 to MBEDTLS_SHA1_BLOCK_SIZE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

mbedtls_sha1_process(ctx, ctx->sbuf);
// now process every input as long as it is %4 bytes
size_t iter = currentlen / 4;
HAL_HASH_SHA1_Accumulate(&ctx->hhash_sha1, (uint8_t *)(input+MBEDTLS_SHA1_BLOCK_SIZE-ctx->sbuf_len), (iter*4));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as in the SHA256:
Please verify how HAL_HASH_SHA1_Accumulate will behave if iter = 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @RonEld ,
It will call HASH_WriteData in stm32f4xx_hal_hash.c line 225

for(buffercounter = 0U; buffercounter < Size; buffercounter+=4U)
 {
   HASH->DIN = *(uint32_t*)inputaddr;
   inputaddr+=4U;
 }

It will do nothing in case the Size is =0 (tested for the 3 toolchains GCC_ARM / IAR / ARM)

@adustm
Copy link
Member Author

adustm commented May 23, 2017

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

Thanks in advance,
Armelle

Copy link

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remarks as PR #4159

@andresag01
Copy link

This is equivalent to #4162 and I have already reviewed that one.

@adustm
Copy link
Member Author

adustm commented Jun 2, 2017

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

Copy link

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, see my comments in PR #4162

Copy link

@andresag01 andresag01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

adustm commented Jun 12, 2017

Hello,
@yanesca @andresag01 , could you approve this PR or is there anything missing ?

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

@sg-
Copy link
Contributor

sg- commented Jun 15, 2017

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

@sg- sg- changed the base branch from mbed-os-workshop-17q2 to master June 15, 2017 14:58
@adustm adustm changed the base branch from master to mbed-os-workshop-17q2 June 16, 2017 07:24
@adustm adustm changed the base branch from mbed-os-workshop-17q2 to master June 16, 2017 09:12
@adustm
Copy link
Member Author

adustm commented Jun 16, 2017

Hello
Approvals are ok, rebase is done.
Kind regards

@adustm
Copy link
Member Author

adustm commented Jun 19, 2017

Hello, the scripts are still running after 3 days, I guess it should be executed again ?

Thanks in advance
Armelle

@theotherjimmy
Copy link
Contributor

Bumping CI by closing then opening.

@theotherjimmy
Copy link
Contributor

retest uvisor

1 similar comment
@adbridge
Copy link
Contributor

retest uvisor

@adustm
Copy link
Member Author

adustm commented Jun 22, 2017

bump ?

@adbridge
Copy link
Contributor

/morph test

@mbed-bot
Copy link

Result: FAILURE

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

/morph test

Output

mbed Build Number: 621

Test failed!

@adustm
Copy link
Member Author

adustm commented Jun 23, 2017

Hello @adbridge
My PR has nothing to do with rtos on the failing device. Could you check the results, please ?
Kind regards
Armelle
cc @screamerbg

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 26, 2017

/morph test

@mbed-bot
Copy link

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 636

All builds and test passed!

@adustm
Copy link
Member Author

adustm commented Jun 27, 2017

👍

@adustm adustm deleted the STM_sha1_F439ZI branch October 11, 2018 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants