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

yanesca · 2016-10-27T15:58:31Z

Description

This is an implementation of mbed TLS threading abstraction layer using RTOS mutexes: The application developer has to call the mbedtls_threading_set_mbed() function before calling any mbed TLS functions and the library will use RTOS mutexes to provide thread safety where it is sopported. (see the corresponding Knowledge Base article).

Status

IN DEVELOPMENT

Migrations

This PR does not change any existing behaviour:
NO

Todos

Documentation

Description:

This is an implementation of mbed TLS threading abstraction layer
using RTOS mutexes.

Design decisions:

The is_valid flag is not checked in the init function, because it is a
chance that an uninitialised variable has the value of one (it is a
small chance, but it would result in incorrect behaviour)
The checks on the is_valid flag are not abbreviated because an
uninitialised variable is nonzero in most of the cases
The type of a mutex is a pointer to void and not an incomplete struct,
because we use definitions with mbedtls_threading_mutex_t in the
library
There is a wrapper for mbedtls_threading_set_alt to make it more user
friendly and to enable the use of static functions

0xc0170 · 2016-10-28T08:11:24Z

Please send this to the master branch.

I'll label this as "do not merge" for the time being.

This is an implementation of mbed TLS threading abstraction layer using RTOS mutexes. Design decisions: - The is_valid flag is not checked in the init function, because it is a chance that an uninitialised variable has the value of one (it is a small chance, but it would result in incorrect behaviour) - The checks on the is_valid flag are not abbreviated because an uninitialised variable is nonzero in most of the cases - The type of a mutex is a pointer to void and not an incomplete struct, because we use definitions with mbedtls_threading_mutex_t in the library - There is a wrapper for mbedtls_threading_set_alt to make it more user friendly and to enable the use of static functions

yanesca · 2016-10-28T12:49:00Z

I have retested after the rebase on a K64F manually.

Defining threading support impacts application code, therefore the corresponding macros shouldn't be defined by default they should be turned on in the application specific configuration.

simonbutcher

This isn't ready for merging yet and needs rework.

simonbutcher · 2016-11-17T18:25:38Z

features/mbedtls/platform/inc/platform_mbed.h

+/*
+ * Enable mbed TLS to use mbed OS mutexes
+ */
+#if defined(MBEDTLS_THREADING)


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.

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.

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.

simonbutcher · 2016-11-17T18:31:26Z

features/mbedtls/platform/src/threading_alt.cpp

+        return;
+
+    delete mutex->mutex;
+


Need to add mutex->mutex = NULL;

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?

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.

simonbutcher · 2016-11-17T18:32:47Z

features/mbedtls/platform/src/threading_alt.cpp

+{
+    if( mutex == NULL || mutex->is_valid != true )
+        return( MBEDTLS_ERR_THREADING_BAD_INPUT_DATA );
+


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

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?

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.

simonbutcher · 2016-11-17T18:33:19Z

features/mbedtls/platform/src/threading_alt.cpp

+{
+    if( mutex == NULL || mutex->is_valid != true )
+        return( MBEDTLS_ERR_THREADING_BAD_INPUT_DATA );
+


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

I have a same concerns here as I had above.

Comments as above.

simonbutcher · 2016-11-17T18:44:11Z

features/mbedtls/platform/src/threading_alt.cpp

+    return( 0 );
+}
+
+void mbedtls_threading_set_mbed( void )


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

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

andresag01 · 2016-11-28T13:51:53Z

features/mbedtls/platform/inc/platform_mbed.h

+/*
+ * Enable mbed TLS to use mbed OS mutexes
+ */
+#if defined(MBEDTLS_THREADING)


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.

andresag01 · 2016-11-28T13:55:53Z

features/mbedtls/platform/inc/threading_alt.h

+    void* mutex;
+    char is_valid;
+} mbedtls_threading_mutex_t;
+#endif


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

andresag01 · 2016-11-28T13:59:16Z

features/mbedtls/platform/inc/platform_mbed.h

+ */
+#if defined(MBEDTLS_THREADING)
+#define MBEDTLS_THREADING_C
+#define MBEDTLS_THREADING_ALT


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?

andresag01 · 2016-11-28T14:09:14Z

features/mbedtls/platform/src/threading_alt.cpp

+    return( 0 );
+}
+
+void mbedtls_threading_set_mbed( void )


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

sg- · 2017-01-09T22:29:57Z

@yanesca wondering what the status of this is. Any updates? Please close if not planning to complete or if it will be completed in a different version.

yanesca · 2017-01-10T13:45:25Z

There are no updates yet, but we are going to complete it. In case there will be a decision to complete it in a different version, I will close this PR down.

stevew817 · 2017-06-21T20:10:50Z

@yanesca @sg- With the growing use of threads in mbed OS, what is the status of making mbedTLS itself support threading?

theotherjimmy · 2017-07-17T16:12:11Z

please reopen when unblocked.

0xc0170 added the do not merge label Oct 28, 2016

yanesca changed the base branch from mbed-os-5.2 to master October 28, 2016 12:24

yanesca force-pushed the mbedtls-threading-integration branch from 4cfdf7e to 66a97c7 Compare October 28, 2016 12:30

Move mbed TLS threading configuration to the application

55f6e98

Defining threading support impacts application code, therefore the corresponding macros shouldn't be defined by default they should be turned on in the application specific configuration.

simonbutcher suggested changes Nov 17, 2016

View reviewed changes

andresag01 reviewed Nov 28, 2016

View reviewed changes

andresag01 mentioned this pull request Jan 16, 2017

Add a new sample application to test thread safety ARMmbed/mbed-os-example-tls#21

Open

adustm mentioned this pull request Mar 7, 2017

[TLS / hw acceleration] AES ECB for NUCLEO_F439ZI #3691

Merged

theotherjimmy closed this Jul 17, 2017

theotherjimmy removed the do not merge label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yanesca commented Oct 27, 2016 •

edited

Loading

0xc0170 commented Oct 28, 2016

yanesca commented Oct 28, 2016

simonbutcher left a comment

simonbutcher Nov 17, 2016

yanesca Nov 18, 2016 •

edited

Loading

andresag01 Nov 28, 2016

simonbutcher Nov 17, 2016

yanesca Nov 18, 2016 •

edited

Loading

simonbutcher Nov 18, 2016

simonbutcher Nov 17, 2016

yanesca Nov 18, 2016 •

edited

Loading

simonbutcher Nov 18, 2016

simonbutcher Nov 17, 2016

yanesca Nov 18, 2016

simonbutcher Nov 18, 2016

simonbutcher Nov 17, 2016

andresag01 Nov 28, 2016

andresag01 Nov 28, 2016

andresag01 Nov 28, 2016

andresag01 Nov 28, 2016

andresag01 Nov 28, 2016

sg- commented Jan 9, 2017 •

edited

Loading

yanesca commented Jan 10, 2017

stevew817 commented Jun 21, 2017

theotherjimmy commented Jul 17, 2017

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

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

Conversation

yanesca commented Oct 27, 2016 • edited Loading

Description

Status

Migrations

Todos

Description:

0xc0170 commented Oct 28, 2016

yanesca commented Oct 28, 2016

simonbutcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg- commented Jan 9, 2017 • edited Loading

yanesca commented Jan 10, 2017

stevew817 commented Jun 21, 2017

theotherjimmy commented Jul 17, 2017

yanesca commented Oct 27, 2016 •

edited

Loading

yanesca Nov 18, 2016 •

edited

Loading

yanesca Nov 18, 2016 •

edited

Loading

yanesca Nov 18, 2016 •

edited

Loading

sg- commented Jan 9, 2017 •

edited

Loading