Update Mbed TLS to version 2.7.1 #6210

k-stachowiak · 2018-02-26T12:33:48Z

Test PR to verify (using CI) the MD API/ABI issues have been resolved. Unlike #6183 this PR is based on the Mbed TLS tree tagged as 2.7.1 rather than on a hot fix.

0xc0170 · 2018-02-26T12:50:30Z

/morph build

mbed-ci · 2018-02-26T13:10:48Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-02-26T17:08:04Z

Exporter Build : FAILURE

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

mbed-ci · 2018-02-26T18:07:49Z

Test : SUCCESS

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

cmonr · 2018-02-27T06:40:05Z

/morph export-build

Patater · 2018-02-27T09:46:38Z

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha1_alt.c

 {
    if (ctx->active_ctx == &ctx->hw_ctx) {
        mbedtls_sha1_hw_starts(&ctx->hw_ctx);
    } else if (ctx->active_ctx == &ctx->sw_ctx) {
        mbedtls_sha1_sw_starts(&ctx->sw_ctx);
    }
+    return 0


Missing semicolon?

Indeed. I will find build parameters to capture this (I did mbed test --compile against NUMAKER_PFM_M487, but that succeeds) and correct it.

@k-stachowiak Were you able to find the flag? I'd like to get at least an issue up so that this isn't lost until the next time we miss a semicolon.

@cmonr I found the reason for this not being caught automatically. The problem is that Mbed TLS has some features disabled by default, such as SHA1 or MD5. In this case this lead to not compiling the partner code for those hashes. In order to trigger build of these features I prepared an Mbed OS application using all the hashes and enabling them in the build command explicitly. I made sure all the changed code is compiled successfully. I will raise this with the Mbed TLS team today.

mbed-ci · 2018-02-27T12:42:01Z

Exporter Build : SUCCESS

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

cmonr · 2018-02-27T17:51:28Z

@0xc0170 What's the reason this is marked as DNM?

k-stachowiak · 2018-02-28T08:07:29Z

cmonr There were build issues not caught by the automatic tests, which have now been resolved. I will let you know when I have rebased the PR, and found Mbed TLS reviewers.

k-stachowiak · 2018-02-28T09:51:19Z

@cmonr I have rebased and cleaned up this PR. Also I have removed the "do not merge" prefix from the title, so it is ready to be reviewed by Mbed TLS members. I established that the reviews will be performed by @mazimkhan and @dgreen-arm. Could you please assign the reviewers to the PR?

dgreen-arm · 2018-02-28T13:32:41Z

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha512_alt.h

+ */
+MBEDTLS_DEPRECATED void mbedtls_sha512_process(
+                                            mbedtls_sha512_context *ctx,
+                                            const unsigned char data[128] );


This looks a bit misaligned.

dgreen-arm · 2018-02-28T13:44:37Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

    }
    if (st_md5_save_hw_context(ctx) != 1) {
-        return; // return HASH_BUSY timeout Error here
+        // Return HASH_BUSY timout error here


Spelling: timout should be timeout. Repeated a few times in this file.

Also in sha256_alt.c

mazimkhan

Since we are importing a Mbed TLS 2.7.1 and adapting mbed-os at the same time we don't need hash wrappers. Please remove them or clarify their requirement.

mazimkhan · 2018-02-28T13:49:35Z

features/mbedtls/targets/hash_wrappers.c

@@ -0,0 +1,356 @@
+#include "mbedtls/md2.h"


If we are importing a Mbed TLS 2.7.1 and adapting mbed-os at the same time. As in this PR. We should not need these wrappers. Please clarify their requirement.

When the MBEDTLS_*_ALT symbols are defined almost entire translation unit for the given hash is compiled out, therefore whenever _ALT implementation is provided, it must come along with the wrappers. In the proposed approach the wrappers are common for all targets in order to avoid doing this for each target separately.

Lets take an example mbedtls_md2_starts_ret (new) and mbedtls_md2_starts (old) are both flavors of the same API. New mbedtls_md2_starts_ret is called from md2.c.
Where is mbedtls_md2_starts called from?

As long as the deprecated API has not been removed I believe that any Mbed OS application may call it. It has been pointed out in other PR: #5973 (comment)

As long as the deprecated API has not been removed I believe that any Mbed OS application may call it. It has been pointed out in other PR: #5973 (comment)

I believe @LiyouZhou test code is to demonstrate the missing symbols issue in that PR. @LiyouZhou do you have a requirement to use deprecated API. It is not recommended to promote deprecated API.

So in my case the problem is when customer do their own implementation they only port the new API but not the old. That mean all old API will be broken on that particular platform. I think it is a requirement the old API not be broken all of a sudden.

Hi @LiyouZhou this PR updates the API as well as the implementation. Hence there are no old API calls from within the library.

Question to you is about the applications. Do you have an application that you think will break with new API. We recommend updating the application when you decide to update mbed-os version in it.

mazimkhan · 2018-02-28T13:53:16Z

features/mbedtls/targets/TARGET_NUVOTON/TARGET_M480/sha/sha1_alt.h

+ *                 stronger message digests instead.
+ *
+ */
+MBEDTLS_DEPRECATED void mbedtls_sha1_starts( mbedtls_sha1_context *ctx );


Why do partner code need to supply deprecated API when we are importing Mbed TLS 2.7.1 with new API at the same time?
These functions are consumed by md_wrap.c that is updated in 2.7.1 to use the new API (_ret).

Same for all targets.

Just like with the implementations in the .c files, the substantial parts of the Mbed TLS header's won't be included in the compilation when the _ALT symbols are defined. This means that when using certain targets the declarations of the function wouldn't be visible to an Mbed OS application when compiled. While the implementations can be provided together in a single wrappers translation unit, the declarations are expected to be available via specifically named header files, hence the declarations must be duplicated for all the targets.

My question is who is user of these deprecate APIs. These are called from within Mbed TLS and since 2.7.1 only new API is called.

Please see my answer to the wrappers related question.

mazimkhan · 2018-02-28T13:59:20Z

features/mbedtls/targets/TARGET_STM/md5_alt.c

 {
    if (st_md5_restore_hw_context(ctx) != 1) {
-        return; // Return HASH_BUSY timout error here
+        // Return HASH_BUSY timout error here
+        return MBEDTLS_ERR_MD5_HW_ACCEL_FAILED;


Please request feedback from original developer here. HASH_BUSY may be a special status indicating to try again.

This is a valid point. I thought about it and I don't think we can do much about it now. These are _ALT implementations of the Mbed TLS API, and I don't think they can return anything else than what is publicly declared in the Mbed TLS headers (which as far as I can tell don't include codes for a busy resource).

Additionally the previous implementation would just stop the execution upon this error, so the behavior is maintained. The worst way to look at the proposed change is that the error message may be imprecise.

I am afraid that until a new error code is introduced in Mbed TLS (like ...ACCEL_BUSY) we should go with the proposed solution and figure out a way to extend the error codes with HW acceleration in mind.

adbridge · 2018-02-28T15:01:03Z

@k-stachowiak @Patater If you are hoping to get this in for 5.8 then this needs to fully reviewed, approved, passed ci and merged by approx midday on Friday...

mazimkhan

Given that we are considering supporting any mbed-os applications that might still be using deprecated API. Hence, approving this PR.

adbridge

As Azim has approved these changes, I am also approving in order to allow merge

adbridge · 2018-03-01T10:57:03Z

/morph build

mbed-ci · 2018-03-01T11:21:09Z

Build : SUCCESS

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

Triggering tests

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

Patater · 2018-03-01T12:41:28Z

@adbridge Please hold off on merging this until we have approval to release.

mbed-ci · 2018-03-01T12:44:47Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-01T15:13:42Z

Test : FAILURE

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

adbridge · 2018-03-01T16:31:53Z

@studavekar Looks like all the tests for NUCLEO_F429ZI aborted! Any idea why ? Has the board gone down ?

cmonr · 2018-03-01T17:30:16Z

Replying for history's sake, two of the four NUCLEO_F429ZI devices went down, causing tests to load balance on the remaining devices. More tests across fewer devices -> not enough time to complete tests -> Jenkins would eventually abort the build due to a timeout.

Devices have been power cycled and CI will be resumed once reviewers have responded.

studavekar · 2018-03-01T19:05:26Z

/morph test

cmonr · 2018-03-01T19:23:25Z

@Patater By approval, are you referring to the reviewers of this PR, or something outside of this? If the latter, how will we know we have the OK?

cmonr · 2018-03-01T20:14:00Z

Odd hangup with the Test CI. Doing a full rebuild, whilst nothing is in the other queues.

/morph build

mbed-ci · 2018-03-01T20:43:37Z

Build : SUCCESS

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

Triggering tests

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

cmonr · 2018-03-01T20:57:30Z

Still waiting on a clarification from @Patater. Not sure if this means you want the OK from all reviewers, or an OK from somewhere else.

mbed-ci · 2018-03-01T22:02:04Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-02T00:45:27Z

Test : FAILURE

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

cmonr · 2018-03-02T02:59:02Z

Restarting test, since lone K64F error test was seen earlier today.
/morph test

mbed-ci · 2018-03-02T04:43:28Z

Test : SUCCESS

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

Patater · 2018-03-02T07:49:30Z

@adbridge @cmonr OK to merge now. Thanks.

0xc0170 added needs: CI do not merge labels Feb 26, 2018

Patater reviewed Feb 27, 2018

View reviewed changes

cmonr added needs: review and removed needs: CI labels Feb 27, 2018

k-stachowiak added 2 commits February 28, 2018 10:02

Update Mbed TLS to version 2.7.1

9472750

Adjust partner code for MD HW acceleration to new MD API

9d5d60b

k-stachowiak force-pushed the mbed-tls-2.7.1-update branch from 9bdec4e to 9d5d60b Compare February 28, 2018 09:04

k-stachowiak changed the title ~~DO NOT MERGE - Update Mbed TLS to version 2.7.1~~ Update Mbed TLS to version 2.7.1 Feb 28, 2018

Patater mentioned this pull request Feb 28, 2018

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

Closed

dgreen-arm reviewed Feb 28, 2018

View reviewed changes

mazimkhan suggested changes Feb 28, 2018

View reviewed changes

kjbracey removed the do not merge label Feb 28, 2018

Correct typos and code alignment

6370a35

k-stachowiak mentioned this pull request Feb 28, 2018

DO NOT MERGE - Mbed TLS 2.7.0 -> 2.7.1 MD API hot fix #6183

Closed

mazimkhan approved these changes Mar 1, 2018

View reviewed changes

adbridge self-requested a review March 1, 2018 10:55

adbridge approved these changes Mar 1, 2018

View reviewed changes

dgreen-arm approved these changes Mar 1, 2018

View reviewed changes

adbridge merged commit 6e1cd9b into ARMmbed:master Mar 2, 2018

adbridge added release-version: 5.8.0-rc1 and removed needs: review labels Mar 2, 2018

Update Mbed TLS to version 2.7.1 #6210

Update Mbed TLS to version 2.7.1 #6210

Uh oh!

Conversation

k-stachowiak commented Feb 26, 2018

Uh oh!

0xc0170 commented Feb 26, 2018

Uh oh!

mbed-ci commented Feb 26, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 26, 2018

Uh oh!

mbed-ci commented Feb 26, 2018

Uh oh!

cmonr commented Feb 27, 2018

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!

mbed-ci commented Feb 27, 2018

Uh oh!

cmonr commented Feb 27, 2018

Uh oh!

k-stachowiak commented Feb 28, 2018

Uh oh!

k-stachowiak commented Feb 28, 2018

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazimkhan Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adbridge commented Feb 28, 2018

Uh oh!

mazimkhan left a comment

Choose a reason for hiding this comment

Uh oh!

adbridge left a comment

mazimkhan Feb 28, 2018 •

edited

Loading

mazimkhan Mar 1, 2018 •

edited

Loading

cmonr commented Mar 1, 2018 •

edited

Loading