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

Update Mbed TLS to version 2.7.1 #6210

Merged
merged 3 commits into from Mar 2, 2018

Conversation

Projects
None yet
@k-stachowiak
Contributor

k-stachowiak commented Feb 26, 2018

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

This comment has been minimized.

Member

0xc0170 commented Feb 26, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 26, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

/morph export-build

{
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

This comment has been minimized.

@Patater

Patater Feb 27, 2018

Contributor

Missing semicolon?

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 27, 2018

Contributor

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

This comment has been minimized.

@cmonr

cmonr Feb 27, 2018

Contributor

@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.

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

@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

This comment has been minimized.

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 27, 2018

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

@k-stachowiak

This comment has been minimized.

Contributor

k-stachowiak commented Feb 28, 2018

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 k-stachowiak force-pushed the k-stachowiak:mbed-tls-2.7.1-update branch from 9bdec4e to 9d5d60b Feb 28, 2018

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

@k-stachowiak

This comment has been minimized.

Contributor

k-stachowiak commented Feb 28, 2018

@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?

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

This comment has been minimized.

@dgreen-arm

dgreen-arm Feb 28, 2018

This looks a bit misaligned.

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

This comment has been minimized.

@dgreen-arm

dgreen-arm Feb 28, 2018

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

This comment has been minimized.

@dgreen-arm

dgreen-arm Feb 28, 2018

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.

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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.

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

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.

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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?

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

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)

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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.

This comment has been minimized.

@LiyouZhou

LiyouZhou Mar 1, 2018

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.

This comment has been minimized.

@mazimkhan

mazimkhan Mar 1, 2018

Contributor

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.

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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).

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

Same for all targets.

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

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.

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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.

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

Please see my answer to the wrappers related question.

{
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;

This comment has been minimized.

@mazimkhan

mazimkhan Feb 28, 2018

Contributor

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

This comment has been minimized.

@k-stachowiak

k-stachowiak Feb 28, 2018

Contributor

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

This comment has been minimized.

Contributor

adbridge commented Feb 28, 2018

@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 adbridge self-requested a review Mar 1, 2018

@adbridge

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

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 1, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 1, 2018

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

This comment has been minimized.

Contributor

Patater commented Mar 1, 2018

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 1, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

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

This comment has been minimized.

Collaborator

studavekar commented Mar 1, 2018

/morph test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

@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

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

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

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 1, 2018

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

This comment has been minimized.

Contributor

cmonr commented Mar 1, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 2, 2018

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

@mbed-ci

This comment has been minimized.

@Patater

This comment has been minimized.

Contributor

Patater commented Mar 2, 2018

@adbridge @cmonr OK to merge now. Thanks.

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

19 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment