Skip to content

mbed TLS: threading support - DRAFT - Do not merge #3150

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

Closed
wants to merge 2 commits into from
Closed

mbed TLS: threading support - DRAFT - Do not merge #3150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66a97c7
mbed TLS: threading support - DRAFT
yanesca Oct 26, 2016
55f6e98
Move mbed TLS threading configuration to the application
yanesca Nov 7, 2016
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/mbedtls/inc/mbedtls/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1289,7 +1289,7 @@
*
* Uncomment this to allow your own alternate threading implementation.
*/
//#define MBEDTLS_THREADING_ALT
#define MBEDTLS_THREADING_ALT

/**
* \def MBEDTLS_THREADING_PTHREAD
Expand Down Expand Up @@ -2337,7 +2337,7 @@
*
* Enable this layer to allow use of mutexes within mbed TLS
*/
//#define MBEDTLS_THREADING_C
#define MBEDTLS_THREADING_C

/**
* \def MBEDTLS_TIMING_C
Expand Down
8 changes: 8 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 @@ -20,3 +20,11 @@
#if defined(DEVICE_TRNG)
#define MBEDTLS_ENTROPY_HARDWARE_ALT
#endif

/*
* Enable mbed TLS to use mbed OS mutexes
*/
#if defined(MBEDTLS_THREADING)
Copy link
Contributor

Choose a reason for hiding this comment

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

With MBEDTLS_THREADING we're effectively defining a new mbed OS macro to control mbed TLS. I would like to put it in a separate namespace from the normal mbed TLS configuration macros to make it more obvious.

Could we use something like TLS_CONFIG_XXXX? I'm open to ideas.

Copy link
Contributor Author

@yanesca yanesca Nov 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I like the idea of separating the namespaces. If we decide to do it, then I think we should be careful and stay consistent while doing so. For example, we should rename the hardware acceleration macro accordingly (now it is still in the library's namespace MBEDTLS_HW_SUPPORT)

I would suggest the MBED_TLS_ prefix. This way we can take into account that mbed TLS won't always be the TLS stack in an mbed OS application, respect the application developers choice and avoid confusions. I would omit the CONFIG part, because feel it a little bit redundant, since the aim of all of these macros is to configure the mbed TLS library within mbed OS. Also I think that if the macro names are shorter, then they are less intimidating and easier to use.

Copy link

Choose a reason for hiding this comment

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

According to this, threading is turned off by default? While I can see some of the benefits for this to happen, I would prefer to have the opposite default behaviour. Furthermore, I am not sure why we need the MBEDTLS_THREADING macro at all, it only seems to be a short hand for two defines i.e. MBEDTLS_THREADING_C and MBEDTLS_THREADING_ALT that would have to be declared somewhere anyways. I would prefer if the user just had to define/undefine the macros manually in the mbedtls user config. Also this would enable us to remove the MBEDTLS_THREADING macro.

#define MBEDTLS_THREADING_C
#define MBEDTLS_THREADING_ALT
Copy link

Choose a reason for hiding this comment

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

I would like to point out that defining MBEDTLS_THREADING_ALT also defines MUTEX_INIT which might not be what we want, and perhaps should be handled?

#endif
49 changes: 49 additions & 0 deletions features/mbedtls/platform/inc/threading_alt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/* mbed Microcontroller Library
* Copyright (c) 2016 ARM Limited
*
* 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.
*/

#ifndef MBEDTLS_THREADING_ALT_H
#define MBEDTLS_THREADING_ALT_H

#if defined(MBEDTLS_THREADING_ALT)

#ifdef __cplusplus
#include "Mutex.h"

typedef struct
{
rtos::Mutex* mutex;
char is_valid;
} mbedtls_threading_mutex_t;
#else
typedef struct
{
void* mutex;
char is_valid;
} mbedtls_threading_mutex_t;
#endif
Copy link

Choose a reason for hiding this comment

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

Could you please add the comment next to the #endif with the condition of the matching #if?


#ifdef __cplusplus
extern "C" {
#endif

void mbedtls_threading_set_mbed( void );

#ifdef __cplusplus
}
#endif

#endif /* MBEDTLS_THREADING_ALT */
#endif /* threading_alt.h */
79 changes: 79 additions & 0 deletions features/mbedtls/platform/src/threading_alt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/* mbed Microcontroller Library
* Copyright (c) 2016 ARM Limited
*
* 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.
*/

#if !defined(MBEDTLS_CONFIG_FILE)
#include "mbedtls/config.h"
#else
#include MBEDTLS_CONFIG_FILE
#endif

#if defined(MBEDTLS_THREADING_C)

#include "mbedtls/threading.h"

#if defined(MBEDTLS_THREADING_ALT)
static void threading_mutex_init_mbed( mbedtls_threading_mutex_t *mutex )
{
if( mutex == NULL )
return;

mutex->mutex = new rtos::Mutex();

mutex->is_valid = true;
}

static void threading_mutex_free_mbed( mbedtls_threading_mutex_t *mutex )
{
if( mutex == NULL || mutex->is_valid != true )
return;

delete mutex->mutex;

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add mutex->mutex = NULL;

Copy link
Contributor Author

@yanesca yanesca Nov 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I am sorry but I can't see any scenario where this would make any difference. Could you please give an example when it might be useful to set that pointer to NULL?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is just good defensive programming, and shouldn't need justification.

However - a scenario - imagine initialising the library, then tearing it down, because the application software encounters an error. The software then tries restarting the library, and because the mutex is statically allocated it and is only zeroed at boot, it will still contain the pointer to the old mutex. On calling threading_mutex_init_mbed() nothing is allocated because it's already non-zero - but the pointer is stale.

Now expect a failure.

This is artificial and may not work in real life - but the point remains - it's good practice to zero it after use.

mutex->is_valid = false;
}

static int threading_mutex_lock_mbed( mbedtls_threading_mutex_t *mutex )
{
if( mutex == NULL || mutex->is_valid != true )
return( MBEDTLS_ERR_THREADING_BAD_INPUT_DATA );

Copy link
Contributor

Choose a reason for hiding this comment

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

You're checking if mutex is NULL, but you should also check if mutex->mutex is NULL just to be safe.

Copy link
Contributor Author

@yanesca yanesca Nov 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think that the only case mutex->mutex can be NULL is when

  • mutex is uninitialised OR
  • the memory is corrupted / the structure has been accessed with some means other than these functions

In these cases if we suppose that the memory content is uniformly distributed, then the chance of not catching this with is_valid is 1:2^8 and therefore detecting this by checking for NULL is 1:2^40 or 1:2^72, depending on the architecture (it can be higher on architectures with smaller address space).

The check probably wouldn't help against an active adversary at all, because an active adversary would probably set the pointer value to something other than NULL if he can.

I think if we want to make it more robust then changing the type of is_valid to int would be much more effective, than adding this check for NULL.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is defensive programming to me. However, the case for testing for NULL is much weaker, because as you say, the chance of the pointer being NULL is logically improbable. I can accept not making the change.

if( mutex->mutex->lock() != osOK )
return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );

return( 0 );
}

static int threading_mutex_unlock_mbed( mbedtls_threading_mutex_t *mutex )
{
if( mutex == NULL || mutex->is_valid != true )
return( MBEDTLS_ERR_THREADING_BAD_INPUT_DATA );

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, you're checking if mutex is NULL, but you should also check if mutex->mutex is NULL just to be safe.

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 have a same concerns here as I had above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comments as above.

if( mutex->mutex->unlock() != osOK )
return( MBEDTLS_ERR_THREADING_MUTEX_ERROR );

return( 0 );
}

void mbedtls_threading_set_mbed( void )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function is confusingly named, and I'd prefer we didn't have it. Really the library should self-configure and this function causes life time issues, because now something has to call it during initialisation to allow the library to use the primitives. Currently the caller would be the mbed OS application developer - and that is unacceptable as it means we're adding a breaking API change that extends the mbed TLS library interface, but is also outside the library.

This is a library problem, which I'll raise separately and will need to be fixed elsewhere before we can merge this PR.

Back to the point - while we have to have the function, can we call it something like mbedtls_rtx_threading_init()?

_rtx_ because we're using the RTX primitives, and _init because it's more natural than _set_alt().

Copy link

Choose a reason for hiding this comment

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

As @sbutcher-arm mentioned, this is needed because of a library problem and should be fixed before the threading PR is merged.

{
mbedtls_threading_set_alt( threading_mutex_init_mbed,
threading_mutex_free_mbed, threading_mutex_lock_mbed,
threading_mutex_unlock_mbed );
}

#endif /* MBEDTLS_THREADING_ALT */

#endif /* MBEDTLS_THREADING_C */