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

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
Original file line number Diff line number Diff line change
@@ -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_ERR_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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Loading

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
31 changes: 14 additions & 17 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,50 +20,47 @@

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


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

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

#define MBEDTLS_ERR_PLATFORM_HW_FAILED -0x0080
40 changes: 18 additions & 22 deletions ...cell/FEATURE_CRYPTOCELL310/platform_alt.c → features/mbedtls/platform/src/platform_alt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,38 +20,34 @@

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

/* 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 */
mbedtls_platform_context ctx = { };

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 );
core_util_atomic_incr_u32( ( volatile uint32_t * )&ctx.reference_count, 1 );

if( SaSi_LibInit( &rndState, &rndWorkBuff ) != 0 )
return ( -1 );
return ( 0 );
if( ctx.reference_count == 1 )
{
/* call platform specific code to setup crypto 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 );
core_util_atomic_decr_u32( ( volatile uint32_t * )&ctx.reference_count, 1 );
if( ctx.reference_count < 1 )
{
/* call platform specific code to terminate crypto 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*/