Update Mbed TLS HW acceleration partner code to new hashing API #5973

k-stachowiak · 2018-01-30T14:06:10Z

Description

This pull request updates partner code in the Mbed TLS feature regarding the hardware acceleration for hashing on certain target devices.

Status

READY

Migrations

NO

This PR changes the alternative implementations of the following hashes:

MD5,
SHA1,
SHA256,
SHA512,

for the following targets (families):

Nuvoton M480,
Nuvoton NUC427,
Silicon Labs
STM32F4,
STM32F7,
STM32L4.

No user code needs to be changed as the old API has been maintained (however marked as deprecated) as a thin layer of functions calling the new implementations but ignoring the return codes. It is advisable, but not necessary, to refactor users' code to use the new API.

Related PRs

As of creating this PR the upstream functionality has not yet been merged into Mbed OS, therefore this PR should not be merged until Mbed TLS has been updated.

Todos

Imort the new version of Mbed TLS to enable the new API

Deploy notes

None

Steps to test or reproduce

None

0xc0170 · 2018-01-30T15:03:48Z

@hanno-arm Can you assign reviewers please?

hanno-becker · 2018-01-30T16:58:42Z

@0xc0170: I expect @mazimkhan and myself to review it - I can't request the review here, though. Could you please do that?

0xc0170 · 2018-02-01T11:12:12Z

@k-stachowiak Please resolve conflicts

k-stachowiak · 2018-02-01T13:32:29Z

Conflicts resolved.

mazimkhan

Some of MBEDTLS_ERR_<XYZ>_HW_ACCEL_FAILED are not defined. Unless they are generated in some way. Please explain or fix.

mazimkhan · 2018-02-05T14:18:02Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

    }
    if (st_md5_save_hw_context(ctx) != 1) {
-        return; // return HASH_BUSY timeout Error here
+        return MBEDTLS_ERR_MD5_HW_ACCEL_FAILED; // Hash busy timeout


A general error MBEDTLS_ERR_MD5_HW_ACCEL_FAILED here hides the timeout error 'HASH_BUSY' that could simply be to indicate that a retry after a delay may succeed. In general we have to preserve status indications from HW that do not mean actual failure. This should be reviewed by original developer to confirm if error code is correct.

The HW_ACCEK_FAILED codes have been added upstream in Mbed TLS, which have not yet been merged to master in Mbed OS. Neither has the int returning API. This PR must be somehow synchronized with the import of the newest upstream Mbed TLS code.

I agree that the codes should be agreed with the original developer. My intention was to return any reasonable error code from the already available ones while maintaining the information about the errors in the comments as it was provided in the first place.

I think that fine tuning the actual return codes (maybe expanding the error code space for the HW acceleration) may be a task big enough for a separate PR, however I am open to suggestions about changing the return codes in this PR.

mazimkhan · 2018-02-05T14:37:09Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

 {
    /* HASH IP initialization */
    if (HAL_HASH_DeInit(&ctx->hhash_md5) != 0) {
-        // error found to be returned
-        return;
+        return MBEDTLS_ERR_MD5_HW_ACCEL_FAILED;


MBEDTLS_ERR_MD5_HW_ACCEL_FAILED is used but not defined. Can't see a header file change.

mazimkhan · 2018-02-05T14:39:07Z

features/mbedtls/targets/TARGET_STM/sha1_alt.c

 {
    /* Deinitializes the HASH peripheral */
    if (HAL_HASH_DeInit(&ctx->hhash_sha1) == HAL_ERROR) {
-        // error found to be returned
-        return;
+        return MBEDTLS_ERR_SHA1_HW_ACCEL_FAILED;


MBEDTLS_ERR_SHA1_HW_ACCEL_FAILED used but not defined.

mazimkhan · 2018-02-05T14:40:19Z

features/mbedtls/targets/TARGET_STM/sha256_alt.c

    }
    if (st_sha256_save_hw_context(ctx) != 1) {
-        return; // return HASH_BUSY timeout Error here
+        return MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED; // Hash busy timeout


MBEDTLS_ERR_SHA256_HW_ACCEL_FAILED used but not defined.

mazimkhan · 2018-02-05T15:55:42Z

@0xc0170 this PR updates interface of the hashing functions by adding return value. This has to be utilised by the HW acceleration code. Is there a plan to notify partners to update their code and utilise new interface?

ciarmcom · 2018-02-07T12:50:23Z

ARM Internal Ref: IOTSSL-2072

cmonr · 2018-02-12T16:41:27Z

Making a note here that this PR will need to come in before #6040 (comment) can be continued.

The features/mbedtls/targets/TARGET_STM/* files include constant needed for the error codes returned from the MD functions. The features/mbedtls/targets/hash_wrappers.c provides thin redirection layer for the hardware accelerated MD implementations that rely on the old API. The TESTS/mbedtls/multi/main.cpp has been changed to use the new API as its build environment does not rely on the translation unit containing the necessary wrappers.

k-stachowiak · 2018-02-14T09:20:03Z

@0xc0170 It seems that the CIs are green so far. I am not sure how long will the other CI tasks take, so maybe it is a good time to resume the review?

0xc0170 · 2018-02-14T11:44:14Z

/morph build

mbed-ci · 2018-02-14T12:20:18Z

Build : FAILURE

Build number : 1137
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5973/

0xc0170 · 2018-02-14T13:23:14Z

@0xc0170 It seems that the CIs are green so far. I am not sure how long will the other CI tasks take, so maybe it is a good time to resume the review?

Please review the build failures

k-stachowiak · 2018-02-14T15:40:48Z

/morph build

mbed-ci · 2018-02-14T16:10:50Z

Build : SUCCESS

Build number : 1139
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5973/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-02-14T17:29:51Z

Test : SUCCESS

Build number : 945
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5973/945

mbed-ci · 2018-02-14T18:00:42Z

Exporter Build : SUCCESS

Build number : 814
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5973/

mazimkhan · 2018-02-15T13:49:54Z

@hanno-arm this PR has been merged without our approval. Please continue review and provide comments on dependent PR #6040

cmonr · 2018-02-15T16:48:23Z

@hanno-arm @mazimkhan My mistake on that. Won't happen again.

mazimkhan · 2018-02-16T09:11:11Z

@cmonr no worries at all.

hanno-becker · 2018-02-19T17:29:10Z

As observed by @LiyouZhou, this PR currently breaks the build for two reasons: 1. The new hashing API hasn't yet been merged into Mbed OS. 2. Alternative implementations must implement both the new and the legacy API: The dummy wrappers from sha_xxx.h are also guarded by SHA_XXX_ALT.

mazimkhan · 2018-02-19T18:29:57Z

@k-stachowiak how did this not caught in the CI and wasn't these wrappers suppose to provide legacy API?

k-stachowiak · 2018-02-21T08:08:45Z

@mazimkhan I don't know how this PR broke the build. I tested it locally and pushed here for the CI to perform tests. Neither my test nor the CIs have failed. While I agree that the PR should have been reviewed before merging, the automated tests gave misleadingly positive results. I did not expect anything here to break before the introduction of Mbed TLS 2.7.0.

LiyouZhou · 2018-02-21T15:57:52Z

run mbed new mbedtls-bug && cd mbedtls-bug
Put the following code into main.cpp

#include "mbedtls/sha256.h" /* SHA-256 only */

static const char hello_str[] = "Hello, world!";
static const unsigned char *hello_buffer = (const unsigned char *) hello_str;
static const size_t hello_len = sizeof hello_str - 1;

int main(void)
{
    unsigned char output2[32];
    mbedtls_sha256_context ctx2;

    mbedtls_sha256_init(&ctx2);
    mbedtls_sha256_starts(&ctx2, 0); /* SHA-256, not 224 */

    /* Simulating multiple fragments */
    mbedtls_sha256_update(&ctx2, hello_buffer, 1);
    mbedtls_sha256_update(&ctx2, hello_buffer + 1, 1);
    mbedtls_sha256_update(&ctx2, hello_buffer + 2, hello_len - 2);

    mbedtls_sha256_finish(&ctx2, output2);

    /* Or you could re-use the context by doing mbedtls_sha256_starts() again */
    mbedtls_sha256_free(&ctx2);
    return 0;
}

run mbed compile -m UBLOX_C030_U201 -t GCC_ARM will succeed.
run cd mbed-os && git checkout master && cd ..
run mbed compile -m UBLOX_C030_U201 -t GCC_ARM will fail with following msg:

[Error] main.cpp@24,5: 'mbedtls_sha256_starts' was not declared in this scope
[Error] main.cpp@27,5: 'mbedtls_sha256_update' was not declared in this scope
[Error] main.cpp@31,5: 'mbedtls_sha256_finish' was not declared in this scope
[ERROR] ./main.cpp: In function 'int main()':
./main.cpp:24:5: error: 'mbedtls_sha256_starts' was not declared in this scope
     mbedtls_sha256_starts(&ctx2, 0); /* SHA-256, not 224 */
     ^~~~~~~~~~~~~~~~~~~~~
./main.cpp:24:5: note: suggested alternative: 'mbedtls_sha256_starts_ret'
     mbedtls_sha256_starts(&ctx2, 0); /* SHA-256, not 224 */
     ^~~~~~~~~~~~~~~~~~~~~
     mbedtls_sha256_starts_ret
./main.cpp:27:5: error: 'mbedtls_sha256_update' was not declared in this scope
     mbedtls_sha256_update(&ctx2, hello_buffer, 1);
     ^~~~~~~~~~~~~~~~~~~~~
./main.cpp:27:5: note: suggested alternative: 'mbedtls_sha256_update_ret'
     mbedtls_sha256_update(&ctx2, hello_buffer, 1);
     ^~~~~~~~~~~~~~~~~~~~~
     mbedtls_sha256_update_ret
./main.cpp:31:5: error: 'mbedtls_sha256_finish' was not declared in this scope
     mbedtls_sha256_finish(&ctx2, output2);
     ^~~~~~~~~~~~~~~~~~~~~
./main.cpp:31:5: note: suggested alternative: 'mbedtls_sha256_init'
     mbedtls_sha256_finish(&ctx2, output2);
     ^~~~~~~~~~~~~~~~~~~~~
     mbedtls_sha256_init

Testing some how does not catch this.

0xc0170 · 2018-02-22T09:44:20Z

Thanks @LiyouZhou

As observed by @LiyouZhou, this PR currently breaks the build for two reasons: 1. The new hashing API hasn't yet been merged into Mbed OS. 2. Alternative implementations must implement both the new and the legacy API: The dummy wrappers from sha_xxx.h are also guarded by SHA_XXX_ALT.

@k-stachowiak This PR shall be reverted if causing build problems. Can you fix the problem asap?

mazimkhan · 2018-02-24T19:46:59Z

@mazimkhan I don't know how this PR broke the build. I tested it locally and pushed here for the CI to perform tests. Neither my test nor the CIs have failed. While I agree that the PR should have been reviewed before merging, the automated tests gave misleadingly positive results. I did not expect anything here to break before the introduction of Mbed TLS 2.7.0.

This might explain your error #6176 (comment)

Update Mbed TLS HW acceleration partner code to new hashing API

876a3b1

0xc0170 added the needs: review label Jan 30, 2018

cmonr requested review from mazimkhan and hanno-becker February 1, 2018 02:00

0xc0170 added needs: work tracking component: security and removed needs: review labels Feb 1, 2018

Resolve conflicts after master update

174530b

mazimkhan suggested changes Feb 5, 2018

View reviewed changes

Modify Silicon Labs target code to use MD API with the return values

256e1de

ciarmcom added the mirrored label Feb 7, 2018

mazimkhan mentioned this pull request Feb 9, 2018

DO NOT MERGE - Update Mbed TLS to version 2.7.0 #6040

Closed

Krzysztof Stachowiak added 2 commits February 13, 2018 10:07

Prepare test program to work in different MD implementations

f913940

k-stachowiak force-pushed the IOTSSL-1727-update-to-new-md-api branch from 929e1bc to e1d42a5 Compare February 13, 2018 15:01

Expand guards for conditional compilation of ALT MD API

2e9243f

k-stachowiak force-pushed the IOTSSL-1727-update-to-new-md-api branch from f149937 to 2e9243f Compare February 14, 2018 15:35

cmonr merged commit 06b6184 into ARMmbed:master Feb 14, 2018

cmonr added release-version: 5.8.0-rc1 release-version: 5.7.6 and removed needs: work component: security mirrored tracking release-version: 5.7.6 labels Feb 14, 2018

adbridge added the component: security label Feb 15, 2018

cmonr added component: security and removed component: security labels Feb 16, 2018

0xc0170 mentioned this pull request Feb 22, 2018

Revert "Update Mbed TLS HW acceleration partner code to new hashing API" #6163

Merged

This was referenced Feb 23, 2018

Merge 2.7.1 into feature-opaque-keys Mbed-TLS/mbedtls#1398

Merged

Update feature/mbedtls from mbedtls feature-opaque-keys branch #6176

Merged

k-stachowiak mentioned this pull request Feb 28, 2018

Update Mbed TLS to version 2.7.1 #6210

Merged

Update Mbed TLS HW acceleration partner code to new hashing API #5973

Update Mbed TLS HW acceleration partner code to new hashing API #5973

Uh oh!

Conversation

k-stachowiak commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

Uh oh!

0xc0170 commented Jan 30, 2018

Uh oh!

hanno-becker commented Jan 30, 2018

Uh oh!

0xc0170 commented Feb 1, 2018

Uh oh!

k-stachowiak commented Feb 1, 2018

Uh oh!

mazimkhan 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!

mazimkhan commented Feb 5, 2018

Uh oh!

ciarmcom commented Feb 7, 2018

Uh oh!

cmonr commented Feb 12, 2018

Uh oh!

k-stachowiak commented Feb 14, 2018

Uh oh!

0xc0170 commented Feb 14, 2018

Uh oh!

mbed-ci commented Feb 14, 2018

Uh oh!

0xc0170 commented Feb 14, 2018

Uh oh!

k-stachowiak commented Feb 14, 2018

Uh oh!

mbed-ci commented Feb 14, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 14, 2018

Uh oh!

mbed-ci commented Feb 14, 2018

Uh oh!

mazimkhan commented Feb 15, 2018

Uh oh!

cmonr commented Feb 15, 2018

Uh oh!

mazimkhan commented Feb 16, 2018

Uh oh!

hanno-becker commented Feb 19, 2018

Uh oh!

mazimkhan commented Feb 19, 2018

Uh oh!

k-stachowiak commented Feb 21, 2018

Uh oh!

LiyouZhou commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Feb 22, 2018

Uh oh!

mazimkhan commented Feb 24, 2018

Uh oh!

Uh oh!

k-stachowiak commented Jan 30, 2018 •

edited

Loading

LiyouZhou commented Feb 21, 2018 •

edited

Loading