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

Fix #5079. Support of call to mbedtls_x_finish without calling mbedtls_x_update #5630

Merged
merged 4 commits into from Jan 31, 2018

Conversation

Projects
None yet
8 participants
@adustm
Member

adustm commented Dec 1, 2017

Description

Fix #5079
Support of call to mbedtls_x_finish without calling mbedtls_x_update.

Status

READY
Before the PR: failure occurs in case of an empty input and mbedtls_xx_update is not called between xxx_start and xxx_finish

After this PR:

  • SHA1 / SHA256 / MD5 hardware acceleration is activated for STM32F439xI
  • An empty input can be encrypted correctly even is the call to mbedtls_xxx_update has been omited

[] To do: test MD5, as it is not compiled in mbed-os (I could not find a way to activate it, can someone help me for this ?) [edit 12.14.2017 -> MD5 link issue is now fixed.]

Steps to test or reproduce

Use the test available in #5079

@0xc0170 0xc0170 requested a review from hanno-arm Dec 1, 2017

@0xc0170 0xc0170 assigned mazimkhan and unassigned mazimkhan Dec 1, 2017

@0xc0170 0xc0170 requested a review from mazimkhan Dec 1, 2017

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Dec 1, 2017

ARM Internal Ref: IOTSSL-1930

@ciarmcom ciarmcom added the mirrored label Dec 1, 2017

@adustm

This comment has been minimized.

Member

adustm commented Dec 12, 2017

Hello @andresag01
Do you have any feedback for this bug fix ?
Kind regards
Armelle

@andresag01

This comment has been minimized.

Contributor

andresag01 commented Dec 12, 2017

Hi @adustm, thanks for the fix! I have not done a thorough review, but after a quick look the changes look good. However, @mazimkhan and @hanno-arm will be doing the reviews on hardware acceleration-related changes from now on and they need to approve this.

In the meantime, I have replied to your question here regarding the linking of MD5. Perhaps it would be good to include the change I suggested in my comment in this PR to ensure the linking problem does not persist? What are your thoughts?

@adustm

This comment has been minimized.

Member

adustm commented Dec 14, 2017

Hi,
The MD5 link issue is fixed in the last commit 2c85c29.

The test is now 100% tested and OK.
This PR is ready for review.
@mazimkhan and @hanno-arm, could you proceed to the review, please ?

Kind regards
Armelle

@adustm

This comment has been minimized.

Member

adustm commented Dec 19, 2017

bump...

@mazimkhan

LGTM

@@ -175,6 +175,12 @@ void mbedtls_md5_finish( mbedtls_md5_context *ctx, unsigned char output[16] )
return; // Return error code here
}
}
/* The following test can happen when the input is empty, and mbedtls_md5_update has never been called */

This comment has been minimized.

@hanno-arm

hanno-arm Dec 22, 2017

Contributor

@adustm

  1. This seems to introduce a redundancy with the pre-existing code https://github.com/ARMmbed/mbed-os/pull/5630/files#diff-00438dce44385c38da7359547815b2daR135 handling the case of an update call with an empty buffer.
  2. Why are we not unconditionally calling HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, ctx->sbuf, ctx->sbuf_len) here, even if ctx->sbuf_len == 0? Looking at the code for HAL_HASH_MD5_Accumulate, it always performs the same check for hhash_md5.Phase and updates HASH->CR if it finds HAL_HASH_PHASE_READY. Is there any problem with calling HAL_HASH_MD5_Accumulate with an empty buffer?

This comment has been minimized.

@adustm

adustm Jan 15, 2018

Member

Hello @hanno-arm ,
Thank you for this finding.
I've tested your suggested modification and it works fine. I applied this suggestion also for SHA1 and SHA256. You can find it in the next commit.
Kind regards

@0xc0170 0xc0170 added needs: work and removed needs: review labels Dec 22, 2017

@adustm

This comment has been minimized.

Member

adustm commented Jan 15, 2018

Dear all,
I applied a suggestion from the code review:

diff --git a/features/mbedtls/targets/TARGET_STM/md5_alt.c b/features/mbedtls/targets/TARGET_STM/md5_alt.c
index 281e6f5..b390391 100644
--- a/features/mbedtls/targets/TARGET_STM/md5_alt.c
+++ b/features/mbedtls/targets/TARGET_STM/md5_alt.c
@@ -170,17 +170,12 @@ void mbedtls_md5_finish( mbedtls_md5_context *ctx, unsigned char output[16] )
     if (st_md5_restore_hw_context(ctx) != 1) {
         return; // Return HASH_BUSY timout error here
     }
-    if (ctx->sbuf_len > 0) {
-        if (HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, ctx->sbuf, ctx->sbuf_len) != 0) {
-            return; // Return error code here
-        }
-    }
-    /* The following test can happen when the input is empty, and mbedtls_md5_update has never been called */
-    if(ctx->hhash_md5.Phase == HAL_HASH_PHASE_READY) {
-        /* Select the MD5 mode and reset the HASH processor core, so that the HASH will be ready to compute
-            the message digest of a new message */
-        HASH->CR |= HASH_ALGOSELECTION_MD5 | HASH_CR_INIT;
+    /* Last accumulation for extra bytes in sbuf_len */
+    /* This allows the HW flags to be in place in case mbedtls_sha256_update has not been called yet */
+    if (HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, ctx->sbuf, ctx->sbuf_len) != 0) {
+        return; // Return error code here
     }
+

I did the same for SHA1 and SHA256.
I passed -n tests-mbedtls* for NUCLEO_F439ZI / NUCLEO_L486RG / NUCLEO_F756ZG on IAR / ARM / GCC_ARM
I also passed the test suggested in #5079 for those platforms and toolchains.

Kind regards
Armelle

}
/* Last accumulation for extra bytes in sbuf_len */
/* This allows the HW flags to be in place in case mbedtls_sha256_update has not been called yet */
if (HAL_HASH_MD5_Accumulate(&ctx->hhash_md5, ctx->sbuf, ctx->sbuf_len) != 0) {

This comment has been minimized.

@hanno-arm

hanno-arm Jan 16, 2018

Contributor

@adustm Thank you for the changes! As far as I see, there's still a redundancy in xxx_update: given the current version of xxx_finish, which always calls HAL_HASH_MD5_Accumulate, it should be fine for xxx_update to be a no-op on an empty buffer.

This comment has been minimized.

@adustm

adustm Jan 18, 2018

Member

Hello @hanno-arm
If I remove this 2nd redundancy, the md5 mbedtls-selftest is crashing, that's why I've kept it.

This comment has been minimized.

@hanno-arm

hanno-arm Jan 18, 2018

Contributor

@adustm To be sure we understand each other correctly: As far as I understand the code, it should be fine to remove the code-path for currentlen == 0 from xxx_update - but leaving xxx_finish as it is now. In particular, it shouldn't yield any change in behavior for the testing of starts-finish without update.

This comment has been minimized.

@adustm

adustm Jan 30, 2018

Member

Hello @hanno-arm
Thanks for insisting, I have made more tests.
The updated version of the code avoids the remaining redundancy and passes md5_selftest successfully.

Kind regards

This comment has been minimized.

@hanno-arm

hanno-arm Jan 31, 2018

Contributor

@adustm Thank you for the rework!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 29, 2018

@adustm, has any progress been made on this PR?

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 29, 2018

@adustm, thank you for your changes. You should get some feedback from the following people shortly: @RonEld @hanno-arm @mazimkhan please review.

@adustm adustm force-pushed the adustm:fix5079_sha1_md5_sha256_hwcrypto branch from 7e18ea4 to 88c3b3e Jan 30, 2018

@adustm

This comment has been minimized.

Member

adustm commented Jan 30, 2018

Hi,
Last fix was implemented (commit 88c3b3e)+ tests are OK + rebase on master done.

Kind regards
Armelle

@0xc0170 0xc0170 removed the needs: work label Jan 30, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 30, 2018

Building whilst waiting on the last review.

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 30, 2018

@cmonr cmonr referenced this pull request Jan 30, 2018

Merged

Let libraries, targets configure bootloader #5909

1 of 1 task complete
@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 30, 2018

Failure unrelated to this PR. In the process of resolving.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 31, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 31, 2018

Build : SUCCESS

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

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 Jan 31, 2018

@hanno-arm Can you review the latest update. All your comments should have been addressed.

@cmonr cmonr merged commit c06a42b into ARMmbed:master Jan 31, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment