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 reference counter for platform context #7099

Merged
merged 8 commits into from Sep 3, 2018

Conversation

Projects
None yet
8 participants
@RonEld
Contributor

RonEld commented Jun 4, 2018

Description

  1. Move the mbedtls_platform_context to be platform code, in features/mbedtls/platfrom/.
  2. Add static refernce counter, to setup and teardown the platform code only once.
  3. Adjust Cryptocell porting accordingly.

Should work with system modules using Mbed TLS, such as BLE, that wouldn't terminate the cryptographic driver for the application, in the middle of operation.
Adresses first part of #7069

CC @sbutcher-arm @Patater @pan-

Question: Should we protect the reference counter with a mutex?

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
Add reference counter for platform context
1. Move the `mbedtls_platform_context` to be platform code, in `features/mbedtls/platfrom/`.
2. Add static refernce counter, to setup and teardown the platform code only once.
3. Adjust Cryptocell porting accordingly.

@0xc0170 0xc0170 requested review from ARMmbed/mbed-os-tls, Patater and pan- Jun 4, 2018

@pan-

pan- requested changes Jun 4, 2018 edited

Hi Ron,

Thanks for this submission that intend to solves #7069. Unfortunately reference
counting
is not the right tool to solve the issue; not at the mbed tls level.

Reference counting implies that a single instance/context is shared across multiple
parts of the program. In our case, it is expected that application code using mbed
tls submit a different/specific context every time mbedtls_platform_setup is
called.

With the code as it is proposed, a context will never be initialized if their is at least
another context initialized. This can have dramatic consequences if crypto_platform_ctx
actually contains valuable informations like CRYS_RND_State_t: The code may setup one
instance and teardown one that hasn't been initialized.

I strongly believe that keeping track of initialization is an implementation detail
and this as no place in the platform code at mbed tls level.

@sbutcher-arm

I think we need to consider the interface changes carefully before we make this change.

cc: @Patater

{
int ret = 0;
if( ctx == NULL )
return ( -1 );

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Jun 4, 2018

Contributor

I don't think we should be returning -1, however, we have failed to define in Mbed TLS any error codes for function.

Maybe we should define error codes now locally in Mbed OS, and upstream them to Mbed TLS in time for the following release of Mbed OS? Possibly INVALID_INPUT for the case above, and HW_FAILURE in case of failure from the accelerator. What do you think?

This comment has been minimized.

@RonEld

RonEld Jun 4, 2018

Contributor

I agree, but as you said, there is currently no error codes available

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Jun 4, 2018

Contributor

You could define some locally in Mbed OS, and also submit a PR to upstream the same codes.

As and when the version of Mbed TLS with the new codes becomes available, we can remove the local definitions.

We control both sides (upstream and downstream) so there's no risk of reusing the same values.

This comment has been minimized.

@RonEld

RonEld Aug 29, 2018

Contributor

I created ARMmbed/mbedtls#1993 to add the platform error code

if( reference_count == 1 )
{
/* call platform specific code to setup crypto driver*/
ret = crypto_platform_setup( &ctx->platform_impl_ctx );

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Jun 4, 2018

Contributor

This makes crypto_platform_setup() part of the API for crypto drivers. That will need documenting in the Mbed OS Porting Guide, and also as an interface change will need wider review.

Is there anyway of avoiding this interface change? I'm not sure there is, but we should consider alternatives if we can.

This comment has been minimized.

@RonEld

RonEld Jun 4, 2018

Contributor

I agree it needs documenting in the handbook.

As for interface change, it's basically only a change in the existing name of the function ( cc_platform_setup renamed to crypto+platform_setup, and a bit of who is responsible of implementing CC initialization has changed). Although it is an API change, I believe it is a minor one
Unfortunately, I don't see any other way to have initialization of the platform, in an opaque way, and make it portable to other APIs. ( we can have the whole mbedtls_platform_setup() functino implemented in driver code, but then we guideusers to implement the reference count , which is very common to all the drivers )

if( ctx == NULL )
return;
if( reference_count == 0 )

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Jun 4, 2018

Contributor

I don't think you need two if statements, and it looks a bit odd.

Have you considered using a less than operator? eg.

    reference_count--;
    if( reference_count <= 0 )
    {
        crypto_platform_teardown();
    }

This comment has been minimized.

@RonEld

RonEld Jun 4, 2018

Contributor

I agree it looks a bit odd, but what you are suggesting is prone to errors:

  1. crypto_platform_teardown() might be called twice, if reference_count is 0 or -1.
  2. corner cases where we get the reference counter to -2, and then when some chooses to setup, it won't initialize the platform, because the counter will be increased to -1.

Perhaps assigning reference_count to 0 after calling crypto_platform_teardown()?

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Jun 4, 2018

Contributor

I agree, it would be safer to reassign reference_count.

So...

    reference_count--;
    if( reference_count <= 0 )
    {
        crypto_platform_teardown();
        reference_count = 0;
    }
@RonEld

This comment has been minimized.

Contributor

RonEld commented Jun 4, 2018

@pan- The idea in the mbedtls layer, is the platform itslef is shared across all the modules.
It's basically telling: " start cryptography operations" and "stop cryptography operations".

Specifically for NRF53840, there shouldn't be a problem, as the driver is initialized once, and terminated once. The CRYS_RND_State_t holds the trng data for seeding the DRBG. It is used in the CC initialization because the API requires it, but in actual usage, this struct is used in the trng function, for gathering the entropy.

I agree there might be a need for additional tweaking, such as protecting the CRYS_RND_State_t in trng_api.c with a mutex, and protecting the reference coutner with a mutex, or maybe additional changes, but I think it could be postponed,

@pan-

This comment has been minimized.

Member

pan- commented Jun 4, 2018

The idea in the mbedtls layer, is the platform itslef is shared across all the modules.

Thanks for the clarification.

In that case wouldn't it make sense to remove crypto_platform_ctx from mbedtls_platform_context and declare a single instance of crypto_platform_ctx alongside reference_count ? That instance would be the only one passed to crypto_platform_setup and crypto_platform_teardown. It would prevent calling teardown on an uninitialized crypto_platform_ctx.

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Jun 4, 2018

Hi @pan-

That's not really how we intended the platform context to be used. It's not well documented, mainly because no one has provided an implementation until now, but the context is supposed to be a one-per-platform singleton.

The intention was you should initialise once.

The implication of what you're suggesting is that the API we're providing isn't adequate, and you actually need a mbedtls_platform_setup(), that takes no context as parameter (as you can't track them), and possibly returns one instead?

Add Mbed TLS Platform module errors
1. Add error codes for platform setup \ teardown.
2. Reassign `reference_count` to 0 after terminating platform,
and remove condition for 0
@RonEld

This comment has been minimized.

Contributor

RonEld commented Jun 4, 2018

@sbutcher-arm I addressed your comments

@pan-

This comment has been minimized.

Member

pan- commented Jun 4, 2018

Hi @sbutcher-arm

I agree that if the context passed into mbedtls_platform_setup and mbedtls_platform_teardown is a singleton then reference counting is an adequate solution. Thanks for the clarification.

Given that in the current design the platform context singleton lives outside mbed tls platform layer; library and system code API using mbed tls must be updated to accept the platform context singleton instantiated by the application. In some cases - like BLE - that would force us to expose implementation details that are target specific in our abstract interfaces.

I wonder if it wouldn't be easier to encapsulate the platform context singleton into mbed tls platform layer and completely hide its existence and management from the API exposed to applications. In other words remove the context parameter in setup and teardown.

What do you think ?

PS: If the context is a singleton it may be better to add a reference counter field in the platform context structure rather than having another singleton in the global scope.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 8, 2018

@RonEld What is the status of this PR? Are there any outstanding issues to be resolved (the changes requested in the review for instance) ?

@RonEld

This comment has been minimized.

Contributor

RonEld commented Jun 10, 2018

@0xc0170 I have addressed all the issues. The current status is that there is an architectural decision on how the API should actually look.
I can't really do much until the decision will be made.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 18, 2018

@pan- @sbutcher-arm @RonEld Are there conversations happening offline somewhere, or has discussion on this PR simply become stale?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 29, 2018

Since no progress has been made on this PR for over two weeks, and there's no sign of external conversations, I'm going to close this PR.

Once updates are available, it can be opened.

@cmonr cmonr closed this Jun 29, 2018

@0xc0170 0xc0170 removed the needs: review label Jun 29, 2018

Make the platform context a global variable
Make the platform context a global variable,
adding the refernce counter to it.
@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 28, 2018

@cmonr I have available update for this PR. Do you want me to create a new PR or should this one be opened?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 28, 2018

@RonEld You can commit it to this PR and it can be reopened.

@cmonr cmonr reopened this Aug 28, 2018

@cmonr cmonr added the needs: work label Aug 28, 2018

@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 29, 2018

@pan- I have updated the context to be a global one, holding the reference counter within. You can now call the mbedtls_platform_setup() and mbedtls_platform_teardown() with a NULL argument, as it's being ignored for Mbed OS now.
Note I haven't made it exactly a singleton ( not static ), as it needs to be accessed by other modules. For example, by the trng module, in cryptocell.
Does this fit your needs?

@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 29, 2018

@pan-

pan- approved these changes Aug 29, 2018

@RonEld RonEld referenced this pull request Aug 29, 2018

Merged

Add platform error codes #1993

Rename error codes
1. Rename error codes to fit Mbed TLS error code names.
2. Remove the Invalid input error code, as it's not used anymore.
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

@cmonr @0xc0170 Could you please start CI as this PR was approved?

We will review and run CI (this is expected to be in few days, currently 5.10 PR in the queue)

@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 30, 2018

@0xc0170

currently 5.10 PR in the queue

To my understanding, this PR is targeting 5.10

@cmonr cmonr added needs: CI and removed needs: review labels Aug 30, 2018

@sbutcher-arm

Minor comments only. My only real concern is your use of the word 'blocker' which I think is misleading.

int mbedtls_platform_setup( mbedtls_platform_context *ctx )
int mbedtls_platform_setup( mbedtls_platform_context *obsolete_ctx )

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Aug 31, 2018

Contributor

Why is the context called obsolete? I think that's misleading. It's simply unused.

This comment has been minimized.

@RonEld

RonEld Aug 31, 2018

Contributor

It's obsolete, because it's not used and ignored anymore. I can rename

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Sep 2, 2018

Contributor

If it were obsolete it would no longer be used in any context - but that's not true. We're just choosing not to use it in Mbed OS. It could be used in other OS's and platforms.

I really think it's misleading still. Thanks for changing it.

int mbedtls_platform_setup( mbedtls_platform_context *ctx )
int mbedtls_platform_setup( mbedtls_platform_context *obsolete_ctx )

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Aug 31, 2018

Contributor

Should we check obsolete_ctx is NULL?

This comment has been minimized.

@RonEld

RonEld Aug 31, 2018

Contributor

I don't think so, it is just being ignored. This way, it will be backwards compatible to applications \ modules using this function, that are still sending a content as parameter. Note the example applications weren't updated yet with NULL as parameter

extern CRYS_RND_State_t rndState;
extern CRYS_RND_WorkBuff_t rndWorkBuff;
extern mbedtls_platform_context ctx;

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Aug 31, 2018

Contributor

Is this still needed?

This comment has been minimized.

@RonEld

RonEld Aug 31, 2018

Contributor

yes. Since there isn't any getter function for the context, we still need access to members of it. The RNG functions need the rndState, which is now member of the context

@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 31, 2018

@sbutcher-arm Thank you for your comments. Please refer me to where I use the word "blocker" so i will rephrase.

Rename parameter name
Rename `obsolete_ctx` to `unused_ctx` as it is simply unused.
@RonEld

This comment has been minimized.

Contributor

RonEld commented Aug 31, 2018

@sbutcher-arm i renamed the parameter name to unused_ctx

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 2, 2018

Fix build error on IAR
IAR fails to build when a variable is initialized with empty curly braces.
Added `{ { 0 } }` to fix that.
@RonEld

This comment has been minimized.

Contributor

RonEld commented Sep 2, 2018

build failure on IAR was fixed

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 2, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 2, 2018

Waiting for the last approval from @sbutcher-arm

@cmonr cmonr merged commit 3be076c into ARMmbed:master Sep 3, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 601 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9935 cycles (-361 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the needs: review label Sep 3, 2018

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