STM32 MBEDTLS support with HW crypto #12747

jeromecoutant · 2020-04-02T12:47:11Z

Summary of changes

This pull request enables HW crypto use for all STM32 which support it

STM32F7 specific information:

AES_ALT, MD5_ALT, SHA1_ALT, SHA256_ALT are enabled
CCM_ALT and GCM_ALT support will be added later (internal ticket on going)
Enabled for all STM32F756xG targets (NUCLEO_F756ZG)

STM32F4 specific information:

AES_ALT, MD5_ALT, SHA1_ALT, SHA256_ALT are enabled
CCM_ALT and GCM_ALT support will be added later (internal ticket on going)
Enabled for all STM32F439xI (WIO_3G, MODULE_UBLOX_ODIN_W2, NUCLEO_F439ZI, MTB_STM32_F439) and STM32F437xG targets (UBLOX_C030)
@ARMmbed/team-ublox

STM32L4 specific information:

This family doesn’t use the common CRYP ST API
Only AES_ALT is enabled
Enabled for all STM32L486xG (MTB_ADV_WISE_1570, NUCLEO_L486RG)

STM32WB specific information:

MD5_ALT, SHA1_ALT, SHA256_ALT are not supported (no HASH IP)
AES_ALT is enabled
GCM_ALT and CCM_ALT support will be added later (internal ticket on going)
NEW => MBEDTLS_CONFIG_HW_SUPPORT can be added in the default configuration for all STM32WB55xx (NUCLEO_WB55RG)
@donatieng

STM32L5 specific information:

AES_ALT, MD5_ALT, SHA1_ALT, SHA256_ALT are enabled
GCM_ALT and CCM_ALT support will be added later (internal ticket on going)
NEW => MBEDTLS_CONFIG_HW_SUPPORT can be added in the default configuration for all STM32L562xx (DISCO_L562QE)

@ARMmbed/team-st-mcd
@Patater

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

Verified with benchmark application from:
https://github.com/ARMmbed/mbed-os-example-tls/tree/master/benchmark

and MBEDTLS selftests

Few details from this benchmark application:

STM32L562 compared to STM32L552

	STM32L552xx	STM32L562xx
MD5	3637 KB/s	4772 KB/s
SHA-1	1713 KB/s	4744 KB/s
SHA-256	832 KB/s	4748 KB/s
AES-CBC-256	253 KB/s	1669 KB/s
AES-CTR-256	242 KB/s	784 KB/s
AES-GCM-256	170 KB/s	332 KB/s
AES-CCM-256	123 KB/s	403 KB/s
AES-CMAC-256	243 KB/s	831 KB/s

NUCLEO_WB55RG with full SW MBEDTLS implementation and with HW crypto

	WB55xx full SW	WB55xx with HW crypto
benchmark application
RAM memory (bytes)	11220	11240
Flash memory (bytes)	179308	170806
AES-CBC-256	141 KB/s	1024 KB/s
AES-CTR-256	146 KB/s	478 KB/s
AES-GCM-256	94 KB/s	182 KB/s
AES-CCM-256	67 KB/s	199 KB/s
AES-CMAC-256	134 KB/s	413 KB/s

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2020-04-02T13:00:44Z

@jeromecoutant, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-crypto please review.

mergify · 2020-04-14T09:01:10Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

jeromecoutant · 2020-04-15T08:58:55Z

@ARMmbed/mbed-os-crypto please review.

0xc0170 · 2020-04-17T13:51:06Z

@jeromecoutant will talk to the team to review this PR, I'll also review later today

0xc0170 · 2020-04-20T09:49:01Z

Started CI

0xc0170 · 2020-04-20T15:37:15Z

@ARMmbed/mbed-os-crypto please review.

The team was notified , will review

mbed-ci · 2020-04-20T16:06:10Z

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

Patater

LGTM other than some overly strong synchronization

Patater · 2020-04-20T16:42:37Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+        return;
+    }
+
+    __disable_irq();


Is disabling IRQs really necessary? Is there a weaker synchronization primitive we could use here instead?

This is a question of re-entrance.
Depending of the SW architecture, this code can be simultaneously used from different places (several threads, single thread, even within an interrupt).
Nevertheless, one cryptoprocessor HW instance is available and must be shared.
Even in a single thread, user can call mbedtls primitives in different sequences and all this processing can be interrupted.
For all these reasons, disabling IRQ seems to be the most appropriate solution to deal with all these situations.

If we are for sure using this from an interrupt, sure, I agree we need a stronger primitive. However, AES in an interrupt? What's the use case for that? I suspect just thread safety would be enough for crypto operations, which generally take a long time. As crypto operations do take a long time, you certainly don't want to either do them either within an interrupt context or to block all other interrupts during the crypto operation.

I wouldn't expect any mbed TLS crypto calls to be IRQ safe - not sure it's been defined anywhere.

With the current code, the "mutex init" inside the disable_irq would not be legal, if that was leading to an actual RTOS osMutexInit - that can't be called either from IRQ or with IRQs disabled, so the current form doesn't make sense.

I'd also be very much inclined to avoid the complexity by not attempting the juggling - just write in C++ and use a SingletonPtr<Mutex>. That would have the mutex be statically allocated (if referenced) and initialised on first use.

What can we use to ensure thread safety, knowing that we're trying to release the mutex here that is used at other places to ensure this thread safety ?
Also we're looking for a C implementation that can be used as well outside of mbed-os ...

-Of course, crypto code would not be called within an interrupt (it cannot because it is done with HAL cryp used in polling mode and this implementation will takes too long time in this case. There is no Use Case yet). Sorry for the noise.
-Anyway, you said “mutex_init” (or “mutex_free)” inside the “disable_irq/enable_irq” would not be legal. “mutex_init”/ “mutex_free” are functions with basic processing (chained lists, …). They would not cause a context switch. I do not see what is wrong in calling them with IRQs disabled. Could you explain please?
-“cryp_context_count” and “cryp_mutex_started” should remain valid variables (“atomicity”). I mean more than one instruction can be completed and a switch to another task should not occur until the instructions are executed. What is the right method? Is there a mbed-OS specific function to protect them?
-As pointed by @LMESTM, what would be the C implementation because this code is also used in mbed TLS environment.

osMutexNew from IRQ isn't legal because the CMSIS RTOS docs say so.

In practice, it's because RTX uses supervisor calls both for memory protection and serialisation, and for any call to be usable from interrupt or with interrupts disabled on Cortex-M they need an alternative non-supervisor-call form to be manually coded. That's only done for the calls that are needed from IRQ, like osSemaphoreRelease

It's a cute "feature" of the ARM-M architecture that the SVC instruction causes a Hard Fault if issued with interrupts disabled. Quite different to ARM-A.

An increment of cryp_context_count won't be atomic in hardware, because it's "load", "add", "store" - three instructions. Breaks if there's a task switch between the load and store, and someone else increments or decrements in the meantime. A plain load or store can be, but you still have potential compiler reordering issues, plus the fact you're performing a complete operation - the mutex init - it's not just one access.

If you're trying to write embedded-portable C code here, I don't think there's a portable construct for atomic operations. C11 and C++11 both have them, but implementations aren't always ideal in embedded. Mbed OS has mstd::atomic, which is a local implemenation of std::atomic, but other platforms won't have that. So I think atomics are out.

In Mbed OS I'd use singleton_lock() to protect your reference count and mutex creation. Is there any equivalent in mbed TLS porting layers? Or is there any place you could guarantee a central init call to init your master mutex?

std::mutex guarantees auto-init on first lock - without that you basically need a single init entry point, else you start needing a mutex to protect your mutex creation, and you're stuck in a loop.

Patater · 2020-04-20T16:54:55Z

features/mbedtls/targets/TARGET_STM/aes_alt.h

+
+    CRYP_HandleTypeDef hcryp_aes;   /* AES context                     */
+    uint32_t           ctx_save_cr; /* save context for multi-context  */
+}


I'm very happy not to see space reserved for VIA padlock here. Nicely done. :)

Patater · 2020-04-20T17:05:55Z

features/mbedtls/targets/TARGET_STM/ccm_alt.c

+        return;
+    }
+
+    __disable_irq();


Any weaker synchronization we can use here?

Here any any other places we use __disable_irq().

Patater · 2020-04-20T17:06:07Z

features/mbedtls/targets/TARGET_STM/aes_alt.c

+{
+    AES_VALIDATE(ctx != NULL);
+
+    __disable_irq();


Any weaker synchronization we can use here?

Patater · 2020-04-20T17:06:27Z

features/mbedtls/targets/TARGET_STM/ccm_alt.c

+{
+    CCM_VALIDATE(ctx != NULL);
+
+    __disable_irq();


Any weaker synchronization we can use here?

kjbracey · 2020-04-22T10:41:40Z

features/mbedtls/targets/TARGET_STM/aes_alt.c


-    mbedtls_zeroize(ctx, sizeof(mbedtls_aes_context));
+#if defined(MBEDTLS_THREADING_C)
+    if (cryp_mutex_started) {


Only if cryp_context_count is now zero, surely? But as with the mbedtls_mutex_init, this wouldn't be legal with interrupts disabled.

@tbistm as pointed by @kjbracey-arm - shouldn't you check that cryp_context_count is 0 ?

As pointed by @kjbracey-arm, mutex should be freed when it is no more used. This point is relevant and valid.

mergify · 2020-04-28T10:10:46Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

MarceloSalazar · 2020-05-06T08:50:41Z

@ARMmbed/mbed-os-maintainers waiting for Arm feedback

MarceloSalazar · 2020-05-08T13:56:53Z

@jeromecoutant what do we need to get this moving?

Pull request has been modified.

adbridge · 2020-06-11T11:21:57Z

@Patater could you re-review this please ?

jeromecoutant · 2020-06-11T11:27:22Z

@Patater could you re-review this please ?

Wait a little, I am implementing @kjbracey-arm comments :-)

jeromecoutant · 2020-06-12T08:25:07Z

@Patater @kjbracey-arm
could you check the updates ?
Thx

jeromecoutant · 2020-06-15T07:18:00Z

Let's start CI ?

mbed-ci · 2020-06-15T14:09:00Z

Test run: SUCCESS

Summary: 7 of 7 test jobs passed
Build number : 2
Build artifacts

jeromecoutant changed the title ~~Pr mbedtls~~ STM32 MBEDTLS support with HW crypto Apr 2, 2020

jeromecoutant mentioned this pull request Apr 2, 2020

Incorrect behavior working with Crypto HW accelerator on STM32F439xI chipset #10095

Closed

ciarmcom requested review from a team April 2, 2020 13:00

ciarmcom added the needs: review label Apr 2, 2020

mergify bot added the needs: work label Apr 14, 2020

mergify bot removed the needs: review label Apr 14, 2020

jeromecoutant force-pushed the PR_MBEDTLS branch from 3c0198c to 54f0d73 Compare April 15, 2020 08:58

0xc0170 added needs: review and removed needs: work labels Apr 20, 2020

0xc0170 approved these changes Apr 20, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Apr 20, 2020

Patater previously requested changes Apr 20, 2020

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Apr 20, 2020

0xc0170 previously approved these changes Apr 21, 2020

View reviewed changes

kjbracey reviewed Apr 22, 2020

View reviewed changes

jeromecoutant mentioned this pull request May 29, 2020

STM32WB55 Crypto Hardware Accelaration #13028

Closed

jeromecoutant added 6 commits June 8, 2020 13:51

STM32 MBEDTLS: create mbedtls_device.h for all F756 targets

f71b091

STM32 MBEDTLS: create mbedtls_device.h for all L443 and L486 targets

904ced2

STM32 MBEDTLS: create mbedtls_device.h for all WB55 targets

073bbf7

STM32 MBEDTLS: create mbedtls_device.h for all L562 targets

d0e5722

NUCLEO_WB55RG: enable MBEDTLS_CONFIG_HW_SUPPORT

bfadc65

DISCO_L562QE: enable MBEDTLS_CONFIG_HW_SUPPORT

59d2dd5

jeromecoutant force-pushed the PR_MBEDTLS branch from 54f0d73 to 59d2dd5 Compare June 8, 2020 12:19

adbridge added needs: review release-type: patch Indentifies a PR as containing just a patch and removed needs: work labels Jun 11, 2020

STM32 MBEDTLS_ALT use singleton

e3862d3

Patater approved these changes Jun 12, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jun 12, 2020

mergify bot added ready for merge and removed needs: CI labels Jun 15, 2020

0xc0170 merged commit 3333f41 into ARMmbed:master Jun 16, 2020

mergify bot removed the ready for merge label Jun 16, 2020

jeromecoutant deleted the PR_MBEDTLS branch June 16, 2020 12:34

adbridge added release-version: 6.1.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 24, 2020

STM32 MBEDTLS support with HW crypto #12747

STM32 MBEDTLS support with HW crypto #12747

Uh oh!

Conversation

jeromecoutant commented Apr 2, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Apr 2, 2020

Uh oh!

mergify bot commented Apr 14, 2020

Uh oh!

jeromecoutant commented Apr 15, 2020

Uh oh!

0xc0170 commented Apr 17, 2020

Uh oh!

0xc0170 commented Apr 20, 2020

Uh oh!

0xc0170 commented Apr 20, 2020

Uh oh!

mbed-ci commented Apr 20, 2020

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Apr 28, 2020

Uh oh!

MarceloSalazar commented May 6, 2020

Uh oh!

MarceloSalazar commented May 8, 2020

Uh oh!

adbridge commented Jun 11, 2020

Uh oh!

jeromecoutant commented Jun 11, 2020

Uh oh!

jeromecoutant commented Jun 12, 2020

Uh oh!

jeromecoutant commented Jun 15, 2020

Uh oh!

mbed-ci commented Jun 15, 2020

Uh oh!

Uh oh!