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

Add platform setup and teardown calls to mbedtls tests #6749

Merged
merged 2 commits into from May 17, 2018

Conversation

Projects
None yet
7 participants
@AndrzejKurek
Contributor

AndrzejKurek commented Apr 26, 2018

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

This comment has been minimized.

Member

ciarmcom commented Apr 26, 2018

ARM Internal Ref: IOTSSL-2245

@ciarmcom ciarmcom added the mirrored label Apr 26, 2018

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

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

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

This comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

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

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

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

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 comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

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.

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

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.

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

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

This comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

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

This comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

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

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

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.

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

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.

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

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

Log message is incorrect.

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

@mazimkhan What would you like to be different here?

This comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

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.

This comment has been minimized.

@mazimkhan

mazimkhan May 8, 2018

Contributor

Yes and it could say "platform setup failed".

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

This comment has been minimized.

@hanno-arm

hanno-arm May 8, 2018

Contributor

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?

This comment has been minimized.

@AndrzejKurek

AndrzejKurek May 8, 2018

Contributor

Ok, I'll update the PR.

@hanno-arm

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 cmonr added needs: CI and removed needs: review labels May 14, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 14, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 14, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

cmonr approved these changes May 15, 2018

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

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

13 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/astyle Passed, 845 warnings (+0 warnings)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9589 cycles (+30 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label May 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment