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

RZ_A1LU: Fix TRNG function #5970

Merged
merged 1 commit into from Feb 8, 2018

Conversation

Projects
None yet
6 participants
@TomoYamanaka
Contributor

TomoYamanaka commented Jan 30, 2018

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?

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

This comment has been minimized.

@hanno-arm

hanno-arm Jan 30, 2018

Contributor

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

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

This comment has been minimized.

@hanno-arm

hanno-arm Jan 30, 2018

Contributor

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

@hanno-arm

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

This comment has been minimized.

Contributor

cmonr commented Jan 30, 2018

@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

This comment has been minimized.

Contributor

TomoYamanaka commented Jan 31, 2018

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

@TomoYamanaka TomoYamanaka force-pushed the TomoYamanaka:master branch from 96f51a4 to 334fd6b Jan 31, 2018

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Jan 31, 2018

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

This comment has been minimized.

Contributor

hanno-arm commented Feb 1, 2018

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

@cmonr cmonr requested a review from mazimkhan Feb 1, 2018

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

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 3, 2018

Contributor

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

This comment has been minimized.

@hanno-arm

hanno-arm Feb 3, 2018

Contributor

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

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 3, 2018

Contributor

1 -> sizeof( send_data )

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 3, 2018

Contributor

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

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

This comment has been minimized.

@mazimkhan

mazimkhan Feb 3, 2018

Contributor

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.

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

[RZ_A1LU] Fix TRNG function
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.
- 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 TomoYamanaka force-pushed the TomoYamanaka:master branch from 334fd6b to 412a79d Feb 6, 2018

@TomoYamanaka

This comment has been minimized.

Contributor

TomoYamanaka commented Feb 6, 2018

@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 0xc0170 added needs: CI and removed needs: work labels Feb 6, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 6, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

/morph uvisor-test

3 similar comments
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Feb 6, 2018

/morph uvisor-test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented Feb 6, 2018

/morph uvisor-test

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

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter 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
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment