Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add reference counter for platform context #7099

Merged
merged 8 commits into from
Sep 3, 2018
Merged

Add reference counter for platform context #7099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ca94a49
Add reference counter for platform context
Jun 4, 2018
c3b31bc
Add Mbed TLS Platform module errors
Jun 4, 2018
127b68f
Make the platform context a global variable
Aug 28, 2018
4794389
Rename error codes
Aug 29, 2018
1f5cee9
Address concurrency and style issues
Aug 30, 2018
666ebe3
Change the terminate limit check
Aug 30, 2018
c1b6fdc
Rename parameter name
Aug 31, 2018
a2531b5
Fix build error on IAR
Sep 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions features/cryptocell/FEATURE_CRYPTOCELL310/Readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ To port your CC 310 driver to Mbed OS on your specific target, do the following:
1. In `objects.h`, include `objects_cryptocell.h`. You can use the `FEATURE_CRYPTOCELL310` precompilation check as defined above.
1. In `features/cryptocell/FEATURE_CRYPTOCELL310/TARGET_<target name>`, add your platform-specific libraries for all toolchains in `TOOLCHAIN_ARM`, `TOOLCHAIN_GCC_ARM` and `TOOLCHAIN_IAR` respectively.
1. Add your CC setup code:
* Implement `cc_platform_setup()` and `cc_platform_terminate()` to enable CC on your platform, in case you have board-specific setup functionality, required for CC setup. These functions can be empty.
* Define `cc_platform_ctx` in `cc_platform.h` in a way that suits your implementation.
* Implement `crypto_platform_setup()` and `crypto_platform_terminate()` to enable CC on your platform, in case you have board-specific setup functionality, required for CC setup. You MUST call 'SaSi_LibInit()` and 'SaSi_LibFini()' respectively in these functions.
* Define `crypto_platform_ctx` in `crypto_platform.h` in a way that suits your implementation.

33 changes: 0 additions & 33 deletions features/cryptocell/FEATURE_CRYPTOCELL310/TARGET_MCU_NRF52840/cc_platform_nrf52840.c
View file
Open in desktop

This file was deleted.

31 changes: 11 additions & 20 deletions ...cell/FEATURE_CRYPTOCELL310/platform_alt.c → ...310/TARGET_MCU_NRF52840/crypto_platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* platform_alt.c
* crypto_platform.c
*
* Copyright (C) 2018, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
Expand All @@ -18,40 +18,31 @@
*
*/

#include "mbedtls/platform.h"
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
#include "platform_alt.h"
#include "nrf52840.h"
#include "sns_silib.h"

#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
/* once https://github.com/ARMmbed/mbedtls/issues/1200 will be supported,
* rndState should be part of mbedtls_platform_context
* Until then, we should keep it global and extern */

CRYS_RND_State_t rndState = { { 0 } } ;
CRYS_RND_WorkBuff_t rndWorkBuff = { { 0 } } ;


int mbedtls_platform_setup( mbedtls_platform_context *ctx )
int crypto_platform_setup( crypto_platform_ctx *ctx )
{
int ret = 0;
if( ctx == NULL )
return ( -1 );

/* call platform specific code to setup CC driver*/
if( ( ret = cc_platform_setup( &ctx->platform_impl_ctx ) ) != 0 )
return ( ret );
NRF_CRYPTOCELL->ENABLE = 1;

if( SaSi_LibInit( &rndState, &rndWorkBuff ) != 0 )
return ( -1 );
return ( -1 );

return ( 0 );
}

void mbedtls_platform_teardown( mbedtls_platform_context *ctx )
void crypto_platform_terminate( crypto_platform_ctx *ctx )
{
if( ctx == NULL )
return;

SaSi_LibFini( &rndState );
cc_platform_terminate( &ctx->platform_impl_ctx );
NRF_CRYPTOCELL->ENABLE = 0;
}

#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT*/
#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT */
8 changes: 6 additions & 2 deletions ...CELL310/TARGET_MCU_NRF52840/cc_platform.h → ...310/TARGET_MCU_NRF52840/crypto_platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
*/
#ifndef __CC_PLATFORM_H_
#define __CC_PLATFORM_H_
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
/**
* \brief The CC platform context structure.
*
Expand All @@ -27,7 +28,10 @@
*/
typedef struct {
char dummy; /**< Placeholder member, as empty structs are not portable. */
/*
* Add CRYS_RND_State_t rndState; when https://github.com/ARMmbed/mbedtls/issues/1200 is supported
*/
}
cc_platform_ctx;

crypto_platform_ctx;
#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT */
#endif /* __CC_PLATFORM_H_ */
30 changes: 14 additions & 16 deletions ...cell/FEATURE_CRYPTOCELL310/platform_alt.h → features/mbedtls/platform/inc/platform_alt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@

#ifndef __PLATFORM_ALT__
#define __PLATFORM_ALT__
#include "cc_platform.h"
#include "crys_rnd.h"
#include "platform_mbed.h"
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
#include "crypto_platform.h"

/**
* \brief The platform context structure.
Expand All @@ -30,40 +31,37 @@
* setup or teardown operations.
*/
typedef struct {
cc_platform_ctx platform_impl_ctx; /** A context holding all the partner's platform specific context */
/*
* Add CRYS_RND_State_t rndState; when https://github.com/ARMmbed/mbedtls/issues/1200 is supported
* */
crypto_platform_ctx platform_impl_ctx; /* A context holding all the platform specific context for cryptography. Should be defined in crypto_platform.h */
}
mbedtls_platform_context;


void mbedtls_platform_init( mbedtls_platform_context* ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did this function declaration come from? Do we need it here? Where are the Doxygen comments for it? Why isn't mbedtls_platform_teardown() in this header as well?

In the header file that documents mbedtls_platform_setup() and mbedtls_platform_teardown(), we should comment as to their thread-safety. (As in, if it is safe to call the functions concurrently from multiple threads.)

Copy link
Contributor Author

@RonEld RonEld Aug 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbedtls_platform_init() is a residue from previous work, it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the header file that documents mbedtls_platform_setup() and mbedtls_platform_teardown(), we should comment as to their thread-safety. (As in, if it is safe to call the functions concurrently from multiple threads.)

Probably, but this should be done as a PR in the Mbed TLS repository

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mbedtls_platform_setup() and mbedtls_platform_teardown() are part of the Mbed TLS platform.h API, and described there

/**
* \brief This function performs any partner platform initialization operations,
* needed top enable CryptoCell.
* \brief This function performs any platform initialization operations,
* needed for setting up cryptographic modules.
*
* \param ctx The platform specific context.
*
* \return \c 0 on success.
*
* \note This function is intended to allow platform-specific initialization for CryptoCell,
* and is called before initializing the CC library(SaSi_LibInit). Its
* \note This function is intended to allow platform-specific initialization for Mbed TLS,
* and is called before initializing the Mbed TLS functions. Its
* implementation is platform-specific, and its implementation MUST be provided.
*
*/
int cc_platform_setup( cc_platform_ctx *ctx );
int crypto_platform_setup( crypto_platform_ctx *ctx );

/**
* \brief This function performs any partner platform teardown operations, to disable CryptoCell.
* \brief This function performs any platform teardown operations, to disable cryptographic operations.
*
* \param ctx The platform specific context.
*
* \note This function is called after terminating CC library(SaSi_LibFini)
* and intended to free any resource used for CryptoCell by the platform.
* \note This function is intended to free any resource used Mbed TLS by the platform.
* Its implementation is platform-specific,and its implementation MUST be provided.
*
*/
void cc_platform_terminate( cc_platform_ctx *ctx );

void crypto_platform_terminate( crypto_platform_ctx *ctx );
#endif
#endif /* __PLATFORM_ALT__ */

59 changes: 59 additions & 0 deletions features/mbedtls/platform/src/platform_alt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* platform_alt.c
*
* Copyright (C) 2018, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

#include "mbedtls/platform.h"
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)

static int reference_count = 0;

int mbedtls_platform_setup( mbedtls_platform_context *ctx )
{
int ret = 0;
if( ctx == NULL )
return ( -1 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created Mbed-TLS/mbedtls#1993 to add the platform error code


reference_count++;

if( reference_count == 1 )
{
/* call platform specific code to setup crypto driver*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space after driver.

ret = crypto_platform_setup( &ctx->platform_impl_ctx );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

}
return ( ret );
}

void mbedtls_platform_teardown( mbedtls_platform_context *ctx )
{
if( ctx == NULL )
return;

if( reference_count == 0 )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it would be safer to reassign reference_count.

So...

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

return;

reference_count--;

if( reference_count == 0 )
{
/* call platform specific code to terminate crypto driver*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space after driver.

crypto_platform_terminate( &ctx->platform_impl_ctx );
}
}

#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT*/