Add platform setup and teardown calls to mbedtls tests #6749

AndrzejKurek · 2018-04-26T07:47:37Z

Description

This test adds platform setup and teardown calls to Mbed TLS tests, so that all of the custom cryptographic hardware setup can be performed before running any tests. This has been tested using the current head of mbed-os master, on K64F with GCC_ARM.

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X] Feature
[ ] Breaking change

ciarmcom · 2018-04-26T11:50:19Z

ARM Internal Ref: IOTSSL-2245

mazimkhan · 2018-05-08T08:51:17Z

TESTS/mbedtls/multi/main.cpp

+#if defined(MBEDTLS_PLATFORM_C)
+#include "mbedtls/platform.h"
+#else
+#include <stdio.h>


Is there a reason to include this stuff in the #else block.

To be consistent with other examples, and to allow compilation on platforms without PLATFORM_C define.

@hanno-arm made a more specific comment #6749 (review)

mazimkhan · 2018-05-08T08:53:33Z

TESTS/mbedtls/multi/main.cpp

-    Harness::run(specification);
+    int ret = 0;
+#if defined(MBEDTLS_PLATFORM_C)
+    mbedtls_platform_context platform_ctx;


mbedtls_platform_context can be defined platform specific. We don't know if it will fit stack of an embedded app. It may be more robust to declare it static.

This declaration is consistent with other examples, and in my opinion should be up to the implementer to change, if the stack is small. I would say that this is a standard place to have the context.

Are you suggesting that if an implementation introduces mbedtls_platform_context that is inappropriate for the stack then they should change all tests. If we anticipate that, we can avoid it by creating it static or on heap.

@AndrzejKurek Do you mean Mbed TLS internal example programs or other test programs in the Mbed OS repository?

@hanno-arm I mean our examples. Mbed OS repository examples present mixed approach to how and where are dependencies declared.

@mazimkhan How do we know if someone doesn't prefer to have it on the stack instead? I can introduce your change, but I feel like it's a matter of preferences.

It is about robustness and life time. The life time of platform context is process. It will live as a singleton instance throughout the process. Hence no harm in having it ``static``` or global.

Secondly, it will be robust to have it statically allocated since memory requirement will be checked at compile time. With current implementation it may fail at run time and will require debugging and code change to adjust for the application.

I don't see the reason to have a platform structure in the first place, as opposed to having parameterless functions mbedtls_platform_setup() and mbedtls_platform_teardown() and leaving it to the implementation to allocate and maintain platform specific globals. Also, how would the platform context be actually used in the way it currently works? E.g., how would a custom printf implementation have access to it?

This is an independent design discussion, though, that we should probably have outside of this PR.

mazimkhan · 2018-05-08T08:54:40Z

TESTS/mbedtls/multi/main.cpp

+    mbedtls_platform_context platform_ctx;
+    if((ret = mbedtls_platform_setup(&platform_ctx))!= 0)
+    {
+        mbedtls_printf("Mbed TLS selftest failed! mbedtls_platform_setup returned %d\n", ret);


Log message is incorrect.

@mazimkhan What would you like to be different here?

I believe that Azim is talking about the "selftest" log in a "multi" test. Copy and paste error on my side, I'll fix that.

Yes and it could say "platform setup failed".

hanno-becker · 2018-05-08T10:49:06Z

TESTS/mbedtls/multi/main.cpp

+#include <stdio.h>
+#include <stdlib.h>
+#define mbedtls_printf     printf
+#define mbedtls_snprintf   snprintf


It looks as if we only need mbedtls_printf in this file, so can we get rid of stdlib.h, mbedtls_snprintf, mbedtls_exit, MBEDTLS_EXIT_SUCCESS and MBEDTLS_EXIT_FAILURE?

Ok, I'll update the PR.

hanno-becker

I share @mazimkhan's objections against the use of platform context structures at the API boundary, but this is an independent design discussion. Taken the current design as granted, I approve the changes in this PR.

mazimkhan

I approve this PR. Although I believe that creating mbedtls_platform_context on stack is not good. Since it's size is implementation dependent, it can cause run time stack overflow. I suggest creating it global/static/on heap to avoid future run time error.

cmonr · 2018-05-14T16:38:15Z

/morph build

mbed-ci · 2018-05-14T18:53:35Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-05-15T11:27:25Z

Test : SUCCESS

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

mbed-ci · 2018-05-15T18:22:25Z

Exporter Build : SUCCESS

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

Add platform setup and teardown calls to mbedtls tests

1938faa

0xc0170 requested review from mazimkhan and hanno-becker April 26, 2018 07:50

0xc0170 added tracking component: security needs: review labels Apr 26, 2018

ciarmcom added the mirrored label Apr 26, 2018

0xc0170 previously approved these changes Apr 26, 2018

View reviewed changes

cmonr added the release-version: 5.9.0-rc1 label May 8, 2018

mazimkhan suggested changes May 8, 2018

View reviewed changes

hanno-becker reviewed May 8, 2018

View reviewed changes

Update logs and remove unnecessary defines for Mbed TLS tests

29515c5

AndrzejKurek dismissed 0xc0170’s stale review via 29515c5 May 8, 2018 11:53

hanno-becker approved these changes May 8, 2018

View reviewed changes

mazimkhan approved these changes May 8, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels May 14, 2018

cmonr approved these changes May 15, 2018

View reviewed changes

cmonr added ready for merge and removed needs: CI labels May 15, 2018

cmonr merged commit 77f5c4a into ARMmbed:master May 17, 2018

cmonr removed the ready for merge label May 17, 2018

ciarmcom added the closed_in_jira label May 17, 2018

Add platform setup and teardown calls to mbedtls tests #6749

Add platform setup and teardown calls to mbedtls tests #6749

Uh oh!

Conversation

AndrzejKurek commented Apr 26, 2018

Description

Pull request type

Uh oh!

ciarmcom commented Apr 26, 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!

Choose a reason for hiding this comment

Uh oh!

mazimkhan May 8, 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!

AndrzejKurek May 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

mazimkhan left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented May 14, 2018

Uh oh!

mbed-ci commented May 14, 2018

Triggering tests

Uh oh!

mbed-ci commented May 15, 2018

Uh oh!

mbed-ci commented May 15, 2018

Uh oh!

Uh oh!

mazimkhan May 8, 2018 •

edited

Loading

AndrzejKurek May 8, 2018 •

edited

Loading