STM32: TRNG: remove call to deprecated HAL_RNG_GetRandomNumber #5179

LMESTM · 2017-09-22T14:44:52Z

Description

HAL_RNG_GetRandomNumber is a deprecated API and replaced here with a call to HAL_RNG_GenerateRandomNumber. Doing so, we also rework the driver to use the 4 bytes returned by a call to HAL_RNG_GenerateRandomNumber instead of 1 byte out of 4.
HAL_RNG_GenerateRandomNumber was not returning any error code, so now we can also check the return code.

A second commit ensures that there is a single user at a time.

Status

READY

Tests

+-----------------------+---------------+------------------------+--------+--------------------+-------------+
| target                | platform_name | test suite             | result | elapsed_time (sec) | copy_method |
+-----------------------+---------------+------------------------+--------+--------------------+-------------+
| NUCLEO_F091RC-ARM     | NUCLEO_F091RC | tests-mbedtls-selftest | OK     | 212.18             | default     |
| NUCLEO_F091RC-GCC_ARM | NUCLEO_F091RC | tests-mbedtls-selftest | OK     | 218.93             | default     |
| NUCLEO_F091RC-IAR     | NUCLEO_F091RC | tests-mbedtls-selftest | OK     | 257.7              | default     |
| NUCLEO_F103RB-ARM     | NUCLEO_F103RB | tests-mbedtls-selftest | OK     | 205.53             | default     |
| NUCLEO_F103RB-GCC_ARM | NUCLEO_F103RB | tests-mbedtls-selftest | OK     | 218.34             | default     |
| NUCLEO_F103RB-IAR     | NUCLEO_F103RB | tests-mbedtls-selftest | OK     | 209.21             | default     |
| NUCLEO_F207ZG-ARM     | NUCLEO_F207ZG | tests-mbedtls-selftest | OK     | 203.52             | default     |
| NUCLEO_F207ZG-GCC_ARM | NUCLEO_F207ZG | tests-mbedtls-selftest | OK     | 276.4              | default     |
| NUCLEO_F207ZG-IAR     | NUCLEO_F207ZG | tests-mbedtls-selftest | OK     | 200.14             | default     |
| NUCLEO_F303ZE-ARM     | NUCLEO_F303ZE | tests-mbedtls-selftest | OK     | 251.18             | default     |
| NUCLEO_F303ZE-GCC_ARM | NUCLEO_F303ZE | tests-mbedtls-selftest | OK     | 206.28             | default     |
| NUCLEO_F303ZE-IAR     | NUCLEO_F303ZE | tests-mbedtls-selftest | OK     | 202.71             | default     |
| NUCLEO_F446RE-ARM     | NUCLEO_F446RE | tests-mbedtls-selftest | OK     | 200.99             | default     |
| NUCLEO_F446RE-GCC_ARM | NUCLEO_F446RE | tests-mbedtls-selftest | OK     | 279.02             | default     |
| NUCLEO_F446RE-IAR     | NUCLEO_F446RE | tests-mbedtls-selftest | OK     | 197.36             | default     |
| NUCLEO_F767ZI-ARM     | NUCLEO_F767ZI | tests-mbedtls-selftest | OK     | 197.78             | default     |
| NUCLEO_F767ZI-GCC_ARM | NUCLEO_F767ZI | tests-mbedtls-selftest | OK     | 198.37             | default     |
| NUCLEO_F767ZI-IAR     | NUCLEO_F767ZI | tests-mbedtls-selftest | OK     | 201.52             | default     |
| NUCLEO_L073RZ-ARM     | NUCLEO_L073RZ | tests-mbedtls-selftest | OK     | 217.76             | default     |
| NUCLEO_L073RZ-GCC_ARM | NUCLEO_L073RZ | tests-mbedtls-selftest | OK     | 222.04             | default     |
| NUCLEO_L073RZ-IAR     | NUCLEO_L073RZ | tests-mbedtls-selftest | OK     | 253.9              | default     |
| NUCLEO_L152RE-ARM     | NUCLEO_L152RE | tests-mbedtls-selftest | OK     | 207.84             | default     |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-mbedtls-selftest | OK     | 212.86             | default     |
| NUCLEO_L152RE-IAR     | NUCLEO_L152RE | tests-mbedtls-selftest | OK     | 210.24             | default     |
| NUCLEO_L476RG-ARM     | NUCLEO_L476RG | tests-mbedtls-selftest | OK     | 210.63             | default     |
| NUCLEO_L476RG-GCC_ARM | NUCLEO_L476RG | tests-mbedtls-selftest | OK     | 205.5              | default     |
| NUCLEO_L476RG-IAR     | NUCLEO_L476RG | tests-mbedtls-selftest | OK     | 203.64             | default     |
+-----------------------+---------------+------------------------+--------+--------------------+-------------+
mbedgt: test suite results: 27 OK

All boards compiled OK, for which same tests is ongoing

LMESTM · 2017-09-25T08:50:04Z

@adustm

adustm · 2017-09-25T09:09:57Z

LGTM

0xc0170 · 2017-09-26T14:12:53Z

targets/TARGET_STM/trng_api.c

@@ -63,23 +66,31 @@ void trng_free(trng_t *obj)
    HAL_RNG_DeInit(&obj->handle);
    /* RNG Peripheral clock disable - assume we're the only users of RNG  */
    __HAL_RCC_RNG_CLK_DISABLE();
+
+    users = 0;


line 36 - uses atomic access, not here when you are setting it?

In the free case, I don't have to test the value, I just have to set it to unused, so I preferred to keep it simple: as there is only 1 user, once freed, it will always be 0.

An alternative is to throw an error in case of unbalanced, (like in sleep manager):

core_util_critical_section_enter(); if (users == 0) { core_util_critical_section_exit(); error("Unbalanced number of trng free (< 0)"); } core_util_atomic_decr_u16(&users, 1); core_util_critical_section_exit();

but that seemed overkill to me ...
I can do the change anyway if people think it's of added value.

OK, can be as it is.

Why init can be called only once - one owner limitation?

yes - one HW only and only one client in MBED in features/mbedtls/platform/src/mbed_trng.c, so I'll stick to it for now.

0xc0170 · 2017-09-26T14:13:39Z

cc @yanesca @andresag01 @RonEld

ciarmcom · 2017-09-29T15:50:38Z

ARM Internal Ref: IOTSSL-1790

0xc0170 · 2017-10-10T14:03:35Z

bump for review

adustm · 2017-10-10T14:21:00Z

Hello @0xc0170
The review is fine from my side, as I worked internally with @LMESTM on this PR.
I guess you meant @yanesca @RonEld or @andresag01 ?

Kind regards
Armelle

RonEld

I left one comment about a corner case where a buffer overflow could happen

RonEld · 2017-10-10T14:25:49Z

targets/TARGET_STM/trng_api.c

+                ret = -1;
+        } else {
+            for (uint8_t i =0; i < 4; i++) {
+                *output++ = random[i];


what about corner cases where output length will not be a multiple of 4? In this case you will overflow the buffer

I thought I'd covered it, but clearly missed here - will fix - thx Ron !

LMESTM · 2017-10-10T16:12:00Z

@RonEld thanks for review - I pushed an update to the code which should address the issue

RonEld

Thank you @LMESTM for the changes.
Looks good to me

LMESTM · 2017-10-11T12:53:47Z

thanks again @RonEld
Latest version retested ok

+-------------------+---------------+------------------------+--------+--------------------+-------------+
| target            | platform_name | test suite             | result | elapsed_time (sec) | copy_method |
+-------------------+---------------+------------------------+--------+--------------------+-------------+
| NUCLEO_F091RC-ARM | NUCLEO_F091RC | tests-mbedtls-selftest | OK     | 211.91             | default     |
| NUCLEO_F103RB-ARM | NUCLEO_F103RB | tests-mbedtls-selftest | OK     | 206.03             | default     |
| NUCLEO_F207ZG-ARM | NUCLEO_F207ZG | tests-mbedtls-selftest | OK     | 199.76             | default     |
| NUCLEO_F303ZE-ARM | NUCLEO_F303ZE | tests-mbedtls-selftest | OK     | 205.64             | default     |
| NUCLEO_F446RE-ARM | NUCLEO_F446RE | tests-mbedtls-selftest | OK     | 202.14             | default     |
| NUCLEO_F767ZI-ARM | NUCLEO_F767ZI | tests-mbedtls-selftest | OK     | 200.69             | default     |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-mbedtls-selftest | OK     | 222.38             | default     |
| NUCLEO_L152RE-ARM | NUCLEO_L152RE | tests-mbedtls-selftest | OK     | 210.55             | default     |
| NUCLEO_L476RG-ARM | NUCLEO_L476RG | tests-mbedtls-selftest | OK     | 209.27             | default     |
+-------------------+---------------+------------------------+--------+--------------------+-------------+

0xc0170 · 2017-10-11T13:47:09Z

@yanesca Waiting for your input here? or @RonEld review suffiecient

yanesca · 2017-10-11T14:27:15Z

@0xc0170 Could you please wait for my input? I'll have a look at it quickly.

yanesca

It looks good in general, I just have a minor request and a question.

yanesca · 2017-10-11T14:14:28Z

targets/TARGET_STM/trng_api.c

-    for( uint32_t i = 0; i < length; i++ ){
-        trng_get_byte(obj, output + i );
+    while ((*output_length < length) && (ret ==0)) {
+        if ( HAL_RNG_GenerateRandomNumber(&obj->handle, (uint32_t *)random ) != HAL_OK) {


What happens if we call HAL_RNG_GenerateRandomNumber() without calling trng_init() first?

HAL would return an error in such case.

yanesca · 2017-10-11T14:16:03Z

targets/TARGET_STM/trng_api.c

    /* Just be extra sure that we didn't do it wrong */
    if( ( __HAL_RNG_GET_FLAG(&obj->handle, (RNG_FLAG_CECS | RNG_FLAG_SECS)) ) != 0 ) {
        ret = -1;
-    } else {
-        ret = 0;
    }



I know that I am being paranoid, but could we please wipe the random[] buffer before returning.

sure - will do so and post an update

ok I made an update and now random is erased immediately after use.

mbed-ci · 2017-10-11T14:48:09Z

Build : SUCCESS

Build number : 119
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5179/

Triggering tests

/test mbed-os

mbed-ci · 2017-10-12T08:11:59Z

Test : SUCCESS

Build number : 50
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5179/50

mbed-ci · 2017-10-12T17:07:00Z

Build : SUCCESS

Build number : 157
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5179/

Triggering tests

/test mbed-os

yanesca · 2017-10-13T14:29:25Z

targets/TARGET_STM/trng_api.c

 }

 int trng_get_bytes(trng_t *obj, uint8_t *output, size_t length, size_t *output_length)
 {
-    int ret;
+    int ret = 0;
+    uint8_t random[4];


I suspect this way most of the compilers would optimise the out the instructions zeroising the array, declaring random[] to be volatile might be necessary to prevent it.

yet another good point - thanks !
done

HAL_RNG_GetRandomNumber is a deprecated API and replaced here with a call to HAL_RNG_GenerateRandomNumber. Doing so, we also rework the driver to use the 4 bytes returned by a call to HAL_RNG_GenerateRandomNumber instead of 1 byte out of 4. HAL_RNG_GenerateRandomNumber was not returning any error code, so now we can also check the return code.

There is only 1 RNG HW IP and we do not support more than one driver user at a time, so let's ensure this is the case and raise an error if needed.

0xc0170 · 2017-10-19T15:17:58Z

/morph build

studavekar · 2017-10-19T23:53:29Z

/morph build

mbed-ci · 2017-10-20T00:10:59Z

Build : SUCCESS

Build number : 289
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5179/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-10-20T00:58:21Z

Build : SUCCESS

Build number : 292
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5179/

Triggering tests

/morph test
/morph uvisor-test

mbed-ci · 2017-10-20T02:53:36Z

Test : SUCCESS

Build number : 131
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5179/131

LMESTM force-pushed the trng_deprecated_call branch from 6deaa96 to 73d9da1 Compare September 25, 2017 07:26

theotherjimmy added the needs: review label Sep 25, 2017

LMESTM force-pushed the trng_deprecated_call branch from 73d9da1 to b0854a3 Compare September 25, 2017 15:32

0xc0170 reviewed Sep 26, 2017

View reviewed changes

0xc0170 added the component: security label Sep 26, 2017

adbridge added the tracking label Sep 29, 2017

ciarmcom added the mirrored label Sep 29, 2017

adustm approved these changes Oct 10, 2017

View reviewed changes

RonEld suggested changes Oct 10, 2017

View reviewed changes

LMESTM force-pushed the trng_deprecated_call branch from b0854a3 to 91c6b42 Compare October 10, 2017 16:10

RonEld approved these changes Oct 10, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Oct 11, 2017

yanesca suggested changes Oct 11, 2017

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Oct 12, 2017

LMESTM force-pushed the trng_deprecated_call branch from 91c6b42 to 8251ff4 Compare October 12, 2017 16:49

yanesca reviewed Oct 13, 2017

View reviewed changes

LMESTM added 2 commits October 13, 2017 16:59

STM32: RNG: Ensure that no more than 1 instance is used

849749f

There is only 1 RNG HW IP and we do not support more than one driver user at a time, so let's ensure this is the case and raise an error if needed.

LMESTM force-pushed the trng_deprecated_call branch from 8251ff4 to 849749f Compare October 13, 2017 15:03

yanesca approved these changes Oct 13, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Oct 19, 2017

theotherjimmy merged commit 656aa82 into ARMmbed:master Oct 23, 2017

theotherjimmy added release-version: 5.6.4 and removed needs: CI labels Oct 23, 2017

ciarmcom added the closed_in_jira label Oct 23, 2017

STM32: TRNG: remove call to deprecated HAL_RNG_GetRandomNumber #5179

STM32: TRNG: remove call to deprecated HAL_RNG_GetRandomNumber #5179

Uh oh!

Conversation

LMESTM commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Tests

Uh oh!

LMESTM commented Sep 25, 2017

Uh oh!

adustm commented Sep 25, 2017

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!

0xc0170 commented Sep 26, 2017

Uh oh!

ciarmcom commented Sep 29, 2017

Uh oh!

0xc0170 commented Oct 10, 2017

Uh oh!

adustm commented Oct 10, 2017

Uh oh!

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

LMESTM commented Oct 10, 2017

Uh oh!

RonEld left a comment

Choose a reason for hiding this comment

Uh oh!

LMESTM commented Oct 11, 2017

Uh oh!

0xc0170 commented Oct 11, 2017

Uh oh!

yanesca commented Oct 11, 2017

Uh oh!

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

mbed-ci commented Oct 11, 2017

Triggering tests

Uh oh!

mbed-ci commented Oct 12, 2017

Uh oh!

mbed-ci commented Oct 12, 2017

Triggering tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Oct 19, 2017

Uh oh!

studavekar commented Oct 19, 2017

Uh oh!

mbed-ci commented Oct 20, 2017

Triggering tests

Uh oh!

LMESTM commented Sep 22, 2017 •

edited

Loading