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.

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

STM32: TRNG: remove call to deprecated HAL_RNG_GetRandomNumber #5179

Merged
merged 2 commits into from Oct 23, 2017

Conversation

Projects
None yet
10 participants
@LMESTM
Contributor

LMESTM commented Sep 22, 2017

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 LMESTM force-pushed the LMESTM:trng_deprecated_call branch Sep 25, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 25, 2017

@adustm

This comment has been minimized.

Member

adustm commented Sep 25, 2017

LGTM

@LMESTM LMESTM force-pushed the LMESTM:trng_deprecated_call branch Sep 25, 2017

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

This comment has been minimized.

@0xc0170

0xc0170 Sep 26, 2017

Member

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

This comment has been minimized.

@LMESTM

LMESTM Sep 27, 2017

Contributor

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.

This comment has been minimized.

@0xc0170

0xc0170 Sep 27, 2017

Member

OK, can be as it is.

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

This comment has been minimized.

@LMESTM

LMESTM Sep 27, 2017

Contributor

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

This comment has been minimized.

Member

0xc0170 commented Sep 26, 2017

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Sep 29, 2017

ARM Internal Ref: IOTSSL-1790

@ciarmcom ciarmcom added the mirrored label Sep 29, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 10, 2017

bump for review

@adustm

adustm approved these changes Oct 10, 2017

@adustm

This comment has been minimized.

Member

adustm commented Oct 10, 2017

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

targets/TARGET_STM/trng_api.c Outdated
ret = -1;
} else {
for (uint8_t i =0; i < 4; i++) {
*output++ = random[i];

This comment has been minimized.

@RonEld

RonEld Oct 10, 2017

Contributor

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

This comment has been minimized.

@LMESTM

LMESTM Oct 10, 2017

Contributor

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

@LMESTM LMESTM force-pushed the LMESTM:trng_deprecated_call branch Oct 10, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Oct 10, 2017

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

@RonEld

RonEld approved these changes Oct 10, 2017

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

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Oct 11, 2017

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 0xc0170 added needs: CI and removed needs: review labels Oct 11, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 11, 2017

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

@yanesca

This comment has been minimized.

Contributor

yanesca commented Oct 11, 2017

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

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) {

This comment has been minimized.

@yanesca

yanesca Oct 11, 2017

Contributor

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

This comment has been minimized.

@LMESTM

LMESTM Oct 11, 2017

Contributor

HAL would return an error in such case.

/* 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;
}

This comment has been minimized.

@yanesca

yanesca Oct 11, 2017

Contributor

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

This comment has been minimized.

@LMESTM

LMESTM Oct 11, 2017

Contributor

sure - will do so and post an update

This comment has been minimized.

@LMESTM

LMESTM Oct 12, 2017

Contributor

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 11, 2017

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

This comment has been minimized.

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

@LMESTM LMESTM force-pushed the LMESTM:trng_deprecated_call branch Oct 12, 2017

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 12, 2017

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

targets/TARGET_STM/trng_api.c Outdated
}
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];

This comment has been minimized.

@yanesca

yanesca Oct 13, 2017

Contributor

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.

This comment has been minimized.

@LMESTM

LMESTM Oct 13, 2017

Contributor

yet another good point - thanks !
done

LMESTM added some commits Sep 21, 2017

STM32: TRNG: remove call to deprecated HAL_RNG_GetRandomNumber
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.
STM32: RNG: Ensure that no more than 1 instance is used
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 LMESTM force-pushed the LMESTM:trng_deprecated_call branch to 849749f Oct 13, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

/morph build

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

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 19, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 20, 2017

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

This comment has been minimized.

mbed-ci commented Oct 20, 2017

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

This comment has been minimized.

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

7 checks passed

AWS-CI uVisor Build & Test Success
Details
AWS-CI uvisor Test DIDNT RUN UVISOR TESTS
Details
Cam-CI uvisor Build & Test DIDNT RUN UVISOR TESTS
Details
ci-morph-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment