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 3 commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions features/cryptocell/FEATURE_CRYPTOCELL310/Readme.md
View file
Open in desktop
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.

17 changes: 14 additions & 3 deletions ...ARGET_MCU_NRF52840/cc_platform_nrf52840.c → ...310/TARGET_MCU_NRF52840/crypto_platform.c
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* cc_platform_nrf52840.c
* crypto_platform.c
*
* Copyright (C) 2018, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
Expand All @@ -20,14 +20,25 @@

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

int cc_platform_setup( cc_platform_ctx *ctx )
static CRYS_RND_WorkBuff_t rndWorkBuff = { { 0 } } ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in the crypto_platform_ctx?

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 don't think so, it's only a sandbox for the initialization operation, and not used later

Copy link
Contributor

Choose a reason for hiding this comment

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

OK


int crypto_platform_setup( crypto_platform_ctx *ctx )
{
NRF_CRYPTOCELL->ENABLE = 1;

if( SaSi_LibInit( &ctx->rndState, &rndWorkBuff ) != 0 )
return ( MBEDTLS_PLATFORM_HW_FAILED );

return ( 0 );
}

void cc_platform_terminate( cc_platform_ctx *ctx )
void crypto_platform_terminate( crypto_platform_ctx *ctx )
{
SaSi_LibFini( &ctx->rndState );
NRF_CRYPTOCELL->ENABLE = 0;
}

#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT */
16 changes: 9 additions & 7 deletions ...CELL310/TARGET_MCU_NRF52840/cc_platform.h → ...310/TARGET_MCU_NRF52840/crypto_platform.h
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* cc_platform.h
* crypto_platform.h
*
* Copyright (C) 2018, Arm Limited, All Rights Reserved
* SPDX-License-Identifier: Apache-2.0
Expand All @@ -17,17 +17,19 @@
* limitations under the License.
*
*/
#ifndef __CC_PLATFORM_H_
#define __CC_PLATFORM_H_
#ifndef __CRYPTO_PLATFORM_H_
#define __CRYPTO_PLATFORM_H_
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
#include "crys_rnd.h"
/**
* \brief The CC platform context structure.
*
* \note This structure may be used to assist platform-specific
* setup or teardown operations.
*/
typedef struct {
char dummy; /**< Placeholder member, as empty structs are not portable. */
CRYS_RND_State_t rndState;
}
cc_platform_ctx;

#endif /* __CC_PLATFORM_H_ */
crypto_platform_ctx;
#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT */
#endif /* __CRYPTO_PLATFORM_H_ */
9 changes: 5 additions & 4 deletions features/cryptocell/FEATURE_CRYPTOCELL310/trng.c
View file
Open in desktop
Expand Up @@ -22,9 +22,10 @@

#include <string.h>
#include "trng_api.h"
#include "mbedtls/platform.h"

extern CRYS_RND_State_t rndState;
extern CRYS_RND_WorkBuff_t rndWorkBuff;
extern 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.

Is this still needed?

Copy link
Contributor Author

@RonEld RonEld Aug 31, 2018 •
edited

Choose a reason for hiding this comment

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

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

static CRYS_RND_WorkBuff_t rndWorkBuff;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should rndWorkBuff be part of mbedtls_platform_context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in #7099 (comment), this is only a sandbox for the initialization operation, and not used later, so I don't think it should be part of the context


/* Implementation that should never be optimized out by the compiler */
static void mbedtls_zeroize( void *v, size_t n ) {
Expand All @@ -48,7 +49,7 @@ CRYSError_t LLF_RND_GetTrngSource(

void trng_init(trng_t *obj)
{
RNG_PLAT_SetUserRngParameters(&rndState, obj);
RNG_PLAT_SetUserRngParameters(&ctx.platform_impl_ctx.rndState, obj);
}

void trng_free(trng_t *obj)
Expand All @@ -66,7 +67,7 @@ int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *outputLe
uint32_t actualLength;

ret = LLF_RND_GetTrngSource(
&rndState , /*in/out*/
&ctx.platform_impl_ctx.rndState , /*in/out*/
obj, /*in/out*/
0, /*in*/
&entropySizeBits, /*in/out*/
Expand Down
32 changes: 15 additions & 17 deletions ...cell/FEATURE_CRYPTOCELL310/platform_alt.h → features/mbedtls/platform/inc/platform_alt.h
View file
Open in desktop
Expand Up @@ -20,50 +20,48 @@

#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.
*
* \note This structure may be used to assist platform-specific
* 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 */
int reference_count;
}
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

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__ */

3 changes: 3 additions & 0 deletions features/mbedtls/platform/inc/platform_mbed.h
View file
Open in desktop
Expand Up @@ -24,3 +24,6 @@
#if defined(MBEDTLS_CONFIG_HW_SUPPORT)
#include "mbedtls_device.h"
#endif

#define MBEDTLS_PLATFORM_INVALID_DATA -0x0080
#define MBEDTLS_PLATFORM_HW_FAILED -0x0082
41 changes: 18 additions & 23 deletions ...cell/FEATURE_CRYPTOCELL310/platform_alt.c → features/mbedtls/platform/src/platform_alt.c
View file
Open in desktop
Expand Up @@ -20,38 +20,33 @@

#include "mbedtls/platform.h"
#if defined(MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT)
#include "sns_silib.h"
mbedtls_platform_context ctx = {0};

/* 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 mbedtls_platform_setup( mbedtls_platform_context *obsolete_ctx )
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check obsolete_ctx is NULL?

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

{
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 );
ctx.reference_count++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function need to work in multi-threaded environments (multiple callers from different threads)?

If so, consider using a mutex or atomic increment (core_util_atomic_incr_u32()). Otherwise, we risk calling setup multiple times if two threads overlap calls to mbedtls_platform_setup(). Both threads can read reference count of 0 into a register, increment the register copy, and write back the register to memory at the same time. Then both will proceed to call crypto_platform_setup().

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 did leave an open question in the Pr description, whether we should consider concurrency issues;

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

I'll change to atomic increase and decrease


if( SaSi_LibInit( &rndState, &rndWorkBuff ) != 0 )
return ( -1 );
return ( 0 );
if( ctx.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 );
}
return ( ret );
}

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

SaSi_LibFini( &rndState );
cc_platform_terminate( &ctx->platform_impl_ctx );
ctx.reference_count--;

if( ctx.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 );
Copy link
Contributor

Choose a reason for hiding this comment

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

This can call terminate multiple times if a client calls teardown too often. Do we care? Maybe this should only call terminate when reference_count goes from 1 to 0, and does nothing otherwise.

We might also want to use atomic decrement if we care about overlapping calls to mbedtls_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.

This is what I did in the begining. See #7099 (comment)

Do we care?

I don't think so, because basically it's an on\off button, us pressing the off button more than once. The likelihood for that is minor, as we do set it to 0 after termination

ctx.reference_count = 0;
}
}

#endif /* MBEDTLS_PLATFORM_SETUP_TEARDOWN_ALT*/