RZ_A1LU: Fix TRNG function #5970

TomoYamanaka · 2018-01-30T09:23:20Z

Related to the review of #5857, I fixed the TRNG function for GR-LYCHEE.

I modified to zeroize "recv_data" before the function return.
I added the processing that check the return value of I2C.read function. If return value is error, "output" is zeroized before function return.

Related PRs

#5857

Request

@cmonr
Could you revert #5967 when this PR passed?

hanno-becker · 2018-01-30T11:57:16Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

+    if (idx == 0) {
+        memset(output, 0, length);
+    }
+    memset(recv_data, 0, sizeof(recv_data));


This might be optimized away by the compiler, please consider using something similar to mbedtls_zeroize.

hanno-becker · 2018-01-30T11:58:18Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

@@ -65,13 +65,18 @@ extern "C" int trng_get_bytes_esp32(uint8_t *output, size_t length, size_t *outp
        send_data[0] = 0;
        ret = mI2c.write(ESP32_I2C_ADDR, send_data, 1);
        if (ret == 0) {
-            mI2c.read(ESP32_I2C_ADDR, recv_data, sizeof(recv_data));
+            ret = mI2c.read(ESP32_I2C_ADDR, recv_data, sizeof(recv_data));


Why are we maintaining an error counter for the write calls, but none for read?

hanno-becker

The zeroization of recv_data is not reliable. Also, the error handling seems to be inconsistent at the moment (though there might be reasons for this - left a comment in the review).

cmonr · 2018-01-30T21:10:28Z

@TomoYamanaka @hanno-arm Thank you for being so prompt with this PR!

@TomoYamanaka I'm holding off on merging in #5967 for now, and won't merge unless this PR takes too long to get into 5.7.5 (two-ish weeks from now).

TomoYamanaka · 2018-01-31T01:02:52Z

@cmonr
Thank you for your cooperation. I will address the above comments asap.

TomoYamanaka · 2018-01-31T06:05:01Z

@hanno-arm
I rebased the commit due to address your two comments.

This might be optimized away by the compiler, please consider using something similar to mbedtls_zeroize.

I changed the way of zeroization of buffer from memset to mbedtls_zeroise by referring to other vendors.

Why are we maintaining an error counter for the write calls, but none for read?

err_cnt is used as a retry counter. There is a time lag in the period from ESP32 reset to start working, error may occur when Write is called.
Thus, I added a retry counter due to address this concern. there is not this counter for Read since it is called after Write.
I renamed err_cnt because this name does not express behavior.

hanno-becker · 2018-02-01T15:50:44Z

@0xc0170 Could you also request a review from @mazimkhan please? Thanks!

mazimkhan · 2018-02-03T20:15:53Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

+            ret = mI2c.read(ESP32_I2C_ADDR, recv_data, sizeof(recv_data));
+            if (ret != 0) {
+                idx = 0;
+                break;


No retry in case of read error. How is it different than a write error?

@mazimkhan There has already been some discussion on this. See above.

mazimkhan · 2018-02-03T20:17:28Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

@@ -59,19 +64,24 @@ extern "C" int trng_get_bytes_esp32(uint8_t *output, size_t length, size_t *outp
    char recv_data[4];
    size_t idx = 0;
    int i;
-    int err_cnt = 0;
+    int retry_cnt = 0;

    while (idx < length) {
        send_data[0] = 0;
        ret = mI2c.write(ESP32_I2C_ADDR, send_data, 1);


1 -> sizeof( send_data )

mazimkhan · 2018-02-03T20:28:20Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

                output[idx++] = recv_data[i];
            }
        } else {
-            err_cnt++;
-            if (err_cnt >= 20) {
+            retry_cnt++;


Mixing retry logic within this loop is not clean and it does not cover read error (if that is also required to be covered). I suggest having a supervising retry loop with condition for ( retry = 0; retry < 20 && idx < length; retry++ ).

mazimkhan · 2018-02-03T20:31:04Z

targets/TARGET_RENESAS/TARGET_RZ_A1XX/TARGET_GR_LYCHEE/trng_api_esp32.cpp

@@ -80,6 +90,10 @@ extern "C" int trng_get_bytes_esp32(uint8_t *output, size_t length, size_t *outp
    if (output_length != NULL) {


If output_length is a required out parameter then output_length == NULL is an invalid input. In that case it should be tested in the beginning of the function.

Related to the review of ARMmbed#5857, I fixed the TRNG function for GR-LYCHEE. - I modified to zeroize "recv_data" before the function return. - I added the processing that check the return value of I2C.read function. If return value is error, "output" is zeroized before function return. - In trng_get_bytes_esp32 function, there is a time lag in the period from ESP32 reset to start working, error may occur when "Write" is called. Thus, I added a retry counter due to address this concern. There is not this counter for "Read" since it is called after "Write".

TomoYamanaka · 2018-02-06T06:56:37Z

@mazimkhan
I rebased the commit due to address your comments.

ret = mI2c.write(ESP32_I2C_ADDR, send_data, 1);

1 -> sizeof( send_data )

I modified from 1 to sizeof( send_data ) .

Mixing retry logic within this loop is not clean and it does not cover read error (if that is also required to be covered). I suggest having a supervising retry loop with condition for ( retry = 0; retry < 20 && idx < length; retry++ ).

This retry loop is not required to cover read error, I think that read error does not occur usually in this situation. However, just in case, I accepted your suggestion and modified this retry logic.

If output_length is a required out parameter then output_length == NULL is an invalid input. In that case it should be tested in the beginning of the function.

I added the NULL check process at the beginning of the function.

mazimkhan

@TomoYamanaka thanks for addressing my comments.

0xc0170 · 2018-02-06T12:07:13Z

/morph build

mbed-ci · 2018-02-06T12:38:32Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

mbed-ci · 2018-02-06T13:59:39Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-02-06T14:14:45Z

Test : SUCCESS

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

0xc0170 · 2018-02-06T15:39:39Z

/morph uvisor-test

0xc0170 · 2018-02-06T16:21:14Z

/morph uvisor-test

cmonr · 2018-02-06T17:36:59Z

/morph uvisor-test

cmonr · 2018-02-06T19:09:02Z

/morph uvisor-test

cmonr · 2018-02-06T20:25:00Z

Waiting on #6026 to be merged to retest.
PR removes failing device in failing test.

cmonr · 2018-02-06T21:37:59Z

/morph uvisor-test

cmonr · 2018-02-06T22:24:47Z

/morph uvisor-test

TomoYamanaka mentioned this pull request Jan 30, 2018

Registration GR-LYCHEE board as a new mbed board #5857

Merged

0xc0170 requested a review from hanno-becker January 30, 2018 10:04

0xc0170 added component: security tracking labels Jan 30, 2018

hanno-becker reviewed Jan 30, 2018

View reviewed changes

hanno-becker suggested changes Jan 30, 2018

View reviewed changes

0xc0170 added the needs: work label Jan 30, 2018

cmonr mentioned this pull request Jan 30, 2018

Removed TRNG peripheral from GR_LECHEE target #5967

Closed

TomoYamanaka force-pushed the master branch from 96f51a4 to 334fd6b Compare January 31, 2018 06:01

hanno-becker approved these changes Feb 1, 2018

View reviewed changes

cmonr requested a review from mazimkhan February 1, 2018 16:31

cmonr added needs: review and removed needs: work labels Feb 1, 2018

mazimkhan suggested changes Feb 3, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Feb 5, 2018

TomoYamanaka force-pushed the master branch from 334fd6b to 412a79d Compare February 6, 2018 06:45

mazimkhan approved these changes Feb 6, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Feb 6, 2018

0xc0170 changed the title ~~[RZ_A1LU] Fix TRNG function~~ RZ_A1LU: Fix TRNG function Feb 6, 2018

cmonr added ready for merge and removed needs: CI labels Feb 6, 2018

0xc0170 added release-version: 5.7.5 and removed component: security tracking labels Feb 7, 2018

cmonr merged commit f8cc426 into ARMmbed:master Feb 8, 2018

cmonr removed the ready for merge label Feb 8, 2018

		@@ -80,6 +90,10 @@ extern "C" int trng_get_bytes_esp32(uint8_t output, size_t length, size_t outp
		if (output_length != NULL) {

RZ_A1LU: Fix TRNG function #5970

RZ_A1LU: Fix TRNG function #5970

Uh oh!

Conversation

TomoYamanaka commented Jan 30, 2018

Related PRs

Request

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanno-becker left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Jan 30, 2018

Uh oh!

TomoYamanaka commented Jan 31, 2018

Uh oh!

TomoYamanaka commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hanno-becker commented Feb 1, 2018

Uh oh!

mazimkhan Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

TomoYamanaka commented Feb 6, 2018

Uh oh!

mazimkhan left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Feb 6, 2018

Uh oh!

mbed-ci commented Feb 6, 2018

Triggering tests

Uh oh!

mbed-ci commented Feb 6, 2018

Uh oh!

mbed-ci commented Feb 6, 2018

Uh oh!

0xc0170 commented Feb 6, 2018

Uh oh!

0xc0170 commented Feb 6, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

cmonr commented Feb 6, 2018

Uh oh!

Uh oh!

TomoYamanaka commented Jan 31, 2018 •

edited

Loading

mazimkhan Feb 3, 2018 •

edited

Loading