gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

Merge mbedtls/development-psa, mbedtls/development into development #51

Merged
Patater merged 629 commits from update-dev-tls-dev-crypto-merge into development 2019-02-15 09:07:37 -06:00
Conversation 32 Commits 629 Files changed 211 +12185 -2581
Patater commented 2019-02-12 04:28:04 -06:00 (Migrated from github.com)

Merge Mbed TLS's merge of https://github.com/ARMmbed/mbedtls/tree/development and https://github.com/ARMmbed/mbedtls/tree/development-psa into Mbed Crypto. This finally aligns the crypto implementation that supports parameter validation with the crypto implementation that supports using TLS on PSA with the crypto implementation in Mbed Crypto which supports using null zero-length buffers in RSA. The source for this merge is the current version of https://github.com/ARMmbed/mbedtls/pull/2395.

This is a step along the way to aligning the crypto implementation in all three branches

https://github.com/ARMmbed/mbedtls/pull/2395 cannot be merged to the Mbed TLS development branch until Mbed Crypto has an up to date crypto implementation, which this PR brings.

After this PR is merged, Mbed TLS will update the submodule pointer in PR https://github.com/ARMmbed/mbedtls/pull/2395 and merge it to development. development-psa will no longer have a reason to exist.

See the merge commit description for an explanation of what merge conflicts were resolved and how. Additional commits are added on top to implement new features in Mbed Crypto like parameter validation support in examples and to remove use of recently deprecated constants.

Merge Mbed TLS's merge of https://github.com/ARMmbed/mbedtls/tree/development and https://github.com/ARMmbed/mbedtls/tree/development-psa into Mbed Crypto. This finally aligns the crypto implementation that supports parameter validation with the crypto implementation that supports using TLS on PSA with the crypto implementation in Mbed Crypto which supports using null zero-length buffers in RSA. The source for this merge is the current version of https://github.com/ARMmbed/mbedtls/pull/2395. This is a step along the way to aligning the crypto implementation in all three branches - https://github.com/ARMmbed/mbedtls/tree/development - https://github.com/ARMmbed/mbedtls/tree/development-psa - https://github.com/ARMmbed/mbed-crypto/tree/development https://github.com/ARMmbed/mbedtls/pull/2395 cannot be merged to the Mbed TLS development branch until Mbed Crypto has an up to date crypto implementation, which this PR brings. After this PR is merged, Mbed TLS will update the submodule pointer in PR https://github.com/ARMmbed/mbedtls/pull/2395 and merge it to development. development-psa will no longer have a reason to exist. See the merge commit description for an explanation of what merge conflicts were resolved and how. Additional commits are added on top to implement new features in Mbed Crypto like parameter validation support in examples and to remove use of recently deprecated constants.
Patater commented 2019-02-12 04:38:26 -06:00 (Migrated from github.com)

Rebase to add new error code handling in psa_crypto.c in commit psa: Use new generic error codes and complete updating the PSA examples for parameter validation support in commit psa: Enable use of PSA examples with CHECK_PARAMS

Rebase to add new error code handling in psa_crypto.c in commit [psa: Use new generic error codes](/ARMmbed/mbed-crypto/pull/51/commits/13fcfdcb8b9b660dc8aa6768e63c2ed4dff25d76) and complete updating the PSA examples for parameter validation support in commit [psa: Enable use of PSA examples with CHECK_PARAMS](/ARMmbed/mbed-crypto/pull/51/commits/f4742ceeb9df32a9ffb25aee75063045b4ff9d3f)
mpg commented 2019-02-12 06:28:43 -06:00 (Migrated from github.com)

I did the merge myself and found the following differences.

  1. In library/cipher.c I think https://github.com/ARMmbed/mbed-crypto/pull/44 was unintentionally reverted.

  2. In library/psa_crypto.c I get the following diff between this PR and my version:

diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index 74a762abcc91..ad7367b9cf0a 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -1227,7 +1227,7 @@ static psa_status_t psa_copy_key_material( const psa_key_slot_t *source,
     buffer_size = PSA_KEY_EXPORT_MAX_SIZE( source->type,
                                            psa_get_key_bits( source ) );
     buffer = mbedtls_calloc( 1, buffer_size );
-    if( buffer == NULL )
+    if( buffer == NULL && buffer_size != 0 )
         return( PSA_ERROR_INSUFFICIENT_MEMORY );
     status = psa_internal_export_key( source, buffer, buffer_size, &length, 0 );
     if( status != PSA_SUCCESS )
@@ -1235,7 +1235,8 @@ static psa_status_t psa_copy_key_material( const psa_key_slot_t *source,
     status = psa_import_key( target, source->type, buffer, length );
 
 exit:
-    mbedtls_platform_zeroize( buffer, buffer_size );
+    if( buffer_size != 0 )
+        mbedtls_platform_zeroize( buffer, buffer_size );
     mbedtls_free( buffer );
     return( status );
 }

The version in this PR diverges from the version in mbed-crypto/development in a way that I don't think is intended.

  1. I get a number of differences in configs/config-default.h and configs/config-psa-crypto.h where it's not immediately clear to me which version is correct, as I'm not sure how different or similar to include/mbedtls/config.h those files are supposed to be.
I did the merge myself and found the following differences. 1. In `library/cipher.c` I think https://github.com/ARMmbed/mbed-crypto/pull/44 was unintentionally reverted. 2. In `library/psa_crypto.c` I get the following diff between this PR and my version: ``` diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 74a762abcc91..ad7367b9cf0a 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1227,7 +1227,7 @@ static psa_status_t psa_copy_key_material( const psa_key_slot_t *source, buffer_size = PSA_KEY_EXPORT_MAX_SIZE( source->type, psa_get_key_bits( source ) ); buffer = mbedtls_calloc( 1, buffer_size ); - if( buffer == NULL ) + if( buffer == NULL && buffer_size != 0 ) return( PSA_ERROR_INSUFFICIENT_MEMORY ); status = psa_internal_export_key( source, buffer, buffer_size, &length, 0 ); if( status != PSA_SUCCESS ) @@ -1235,7 +1235,8 @@ static psa_status_t psa_copy_key_material( const psa_key_slot_t *source, status = psa_import_key( target, source->type, buffer, length ); exit: - mbedtls_platform_zeroize( buffer, buffer_size ); + if( buffer_size != 0 ) + mbedtls_platform_zeroize( buffer, buffer_size ); mbedtls_free( buffer ); return( status ); } ``` The version in this PR diverges from the version in mbed-crypto/development in a way that I don't think is intended. 3. I get a number of differences in `configs/config-default.h` and `configs/config-psa-crypto.h` where it's not immediately clear to me which version is correct, as I'm not sure how different or similar to `include/mbedtls/config.h` those files are supposed to be.
mpg commented 2019-02-12 06:39:37 -06:00 (Migrated from github.com)

@Patater There are a number of tests failing in the CI, which I did not really investigate, plus one test that seems to be hanging in both jobs (all_sh-test_use_psa_crypto_full_cmake_asan) and might require killing.

I still had a superficial look at the failures and here are my first impressions:

  • all_sh-test_check_params_silent seem to fail only in test_suite_psa_crypto so might be unrelated to that particular build, and perhaps more related to the change in library/psa_crypto.c that I mentioned above.

  • all_sh-test_check_params_without_platform build error:

psa_crypto_storage_file.c: In function 'key_id_to_location':

psa_crypto_storage_file.c:39:26: error: implicit declaration of function 'snprintf' [-Werror=implicit-function-declaration]

probably unrelated to parameter validation but more related to building with full config except MBEDTLS_PLATFORM_C, I suspect a missing #include in case the platform module is not enabled.

  • all_sh-test_full_cmake_clang, all_sh-test_m32_o0, all_sh-test_m32_o1, all_sh-test_mx32, all_sh-test_no_64bit_multiplication , all_sh-test_no_udbl_division also only fail in test_suite_psa_crypto, see above (hopefully same root cause for all of them)

  • all_sh-test_no_platform fails to build with:

psa/crypto_examples.c: In function 'mbedtls_param_failed':

psa/crypto_examples.c:335:5: error: implicit declaration of function 'mbedtls_exit' [-Werror=implicit-function-declaration]

     mbedtls_exit( MBEDTLS_EXIT_FAILURE );

probably missing a standard #include when the platform module is not enabled.

@Patater There are a number of tests failing in the CI, which I did not really investigate, plus one test that seems to be hanging in both jobs (`all_sh-test_use_psa_crypto_full_cmake_asan`) and might require killing. I still had a superficial look at the failures and here are my first impressions: - `all_sh-test_check_params_silent` seem to fail only in `test_suite_psa_crypto` so might be unrelated to that particular build, and perhaps more related to the change in `library/psa_crypto.c` that I mentioned above. - `all_sh-test_check_params_without_platform` build error: ``` psa_crypto_storage_file.c: In function 'key_id_to_location': psa_crypto_storage_file.c:39:26: error: implicit declaration of function 'snprintf' [-Werror=implicit-function-declaration] ``` probably unrelated to parameter validation but more related to building with full config except `MBEDTLS_PLATFORM_C`, I suspect a missing `#include` in case the platform module is not enabled. - `all_sh-test_full_cmake_clang`, `all_sh-test_m32_o0`, `all_sh-test_m32_o1`, `all_sh-test_mx32`, `all_sh-test_no_64bit_multiplication `, ` all_sh-test_no_udbl_division` also only fail in `test_suite_psa_crypto`, see above (hopefully same root cause for all of them) - `all_sh-test_no_platform` fails to build with: ``` psa/crypto_examples.c: In function 'mbedtls_param_failed': psa/crypto_examples.c:335:5: error: implicit declaration of function 'mbedtls_exit' [-Werror=implicit-function-declaration] mbedtls_exit( MBEDTLS_EXIT_FAILURE ); ``` probably missing a standard `#include` when the platform module is not enabled.
mpg commented 2019-02-12 06:53:45 -06:00 (Migrated from github.com)

I tool the liberty of force-stopping the CI jobs as both seemed stuck in all_sh-test_use_psa_crypto_full_cmake_asan and I didn't want to waste CI resources. I have no idea what the problem is so far.

I tool the liberty of force-stopping the CI jobs as both seemed stuck in `all_sh-test_use_psa_crypto_full_cmake_asan` and I didn't want to waste CI resources. I have no idea what the problem is so far.
Patater commented 2019-02-12 07:26:00 -06:00 (Migrated from github.com) 
1. In `library/cipher.c` I think #44 was unintentionally reverted.

Fixed

2. In `library/psa_crypto.c` I get the following diff between this PR and my version:

These are from https://github.com/ARMmbed/mbed-crypto/pull/48, which was merged after this branch was created. If you want to try to reproduce my merge, use 2a0f48ae1f as a base. When this PR is merged, those changes won't be removed.

3. I get a number of differences in `configs/config-default.h` and `configs/config-psa-crypto.h` where it's not immediately clear to me which version is correct, as I'm not sure how different or similar to `include/mbedtls/config.h` those files are supposed to be.

configs/config-psa-crypto.h should be identical to include/mbedtls/config.h in Mbed Crypto, as its the default we use in Mbed Crypto.

configs/config-default.h should be identical to include/mbedtls/config.h in Mbed TLS.

It's interesting to compare configs/config-default.h with configs/config-psa-crypto.h , to see what options we use differently between the libraries. Specifically, we enable persistent keys by default, where Mbed TLS doesn't yet. We also enable the presence of PSA APIs by default.

[lots of test failures]

We'll look into those. Thanks taking a look!

> 1. In `library/cipher.c` I think #44 was unintentionally reverted. Fixed > 2. In `library/psa_crypto.c` I get the following diff between this PR and my version: These are from https://github.com/ARMmbed/mbed-crypto/pull/48, which was merged after this branch was created. If you want to try to reproduce my merge, use 2a0f48ae1fe5 as a base. When this PR is merged, those changes won't be removed. > 3. I get a number of differences in `configs/config-default.h` and `configs/config-psa-crypto.h` where it's not immediately clear to me which version is correct, as I'm not sure how different or similar to `include/mbedtls/config.h` those files are supposed to be. `configs/config-psa-crypto.h` should be identical to `include/mbedtls/config.h` in Mbed Crypto, as its the default we use in Mbed Crypto. `configs/config-default.h` should be identical to `include/mbedtls/config.h` in Mbed TLS. It's interesting to compare `configs/config-default.h` with `configs/config-psa-crypto.h` , to see what options we use differently between the libraries. Specifically, we enable persistent keys by default, where Mbed TLS doesn't yet. We also enable the presence of PSA APIs by default. > [lots of test failures] We'll look into those. Thanks taking a look!
Patater commented 2019-02-12 11:14:16 -06:00 (Migrated from github.com)

Rebased on latest mbed-crypto/development branch.

Previous branch at https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-1

Rebased on latest mbed-crypto/development branch. Previous branch at https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-1
Patater commented 2019-02-12 11:14:49 -06:00 (Migrated from github.com)

Added a commit to fix "no platform" tests.

Tests verified working with new branch:

  • test_check_params_silent
  • test_check_params_without_platform
  • test_no_platform
  • test_m32_o0
  • test_m32_o1
  • test_mx32
  • test_no_64bit_multiplication
  • test_no_udbl_division
Added a commit to fix "no platform" tests. Tests verified working with new branch: - test_check_params_silent - test_check_params_without_platform - test_no_platform - test_m32_o0 - test_m32_o1 - test_mx32 - test_no_64bit_multiplication - test_no_udbl_division
Patater commented 2019-02-12 11:52:54 -06:00 (Migrated from github.com)

If you are attempting to reproduce the merge yourself, the current base is 1fb011f2a6.

If you are attempting to reproduce the merge yourself, the current base is 1fb011f2a61b.
mpg commented 2019-02-13 02:56:17 -06:00 (Migrated from github.com)

@Patater Thanks for fixing! I'm now unable to find any issues by just inspecting the code, but the CI is still failing in all_sh-test_use_psa_crypto_full_cmake_asan.

I only looked at a few failures in ssl-opt.sh but I suspect all are the same. The call to psa_crypto_init() returns 15 which is PSA_ERROR_INSUFFICIENT_ENTROPY.

I really have no idea why this is happening all the time in the USE_PSA_CRYPTO build but not the others. As the error seems to be coming from a call to a PSA API, I'm sure you guys will have more ideas than me about it.

@Patater Thanks for fixing! I'm now unable to find any issues by just inspecting the code, but the CI is still failing in `all_sh-test_use_psa_crypto_full_cmake_asan`. I only looked at a few failures in `ssl-opt.sh` but I suspect all are the same. The [call to `psa_crypto_init()`](https://github.com/ARMmbed/mbedtls/blob/development-psa/programs/ssl/ssl_server2.c#L1381) returns `15` which is `PSA_ERROR_INSUFFICIENT_ENTROPY`. I really have no idea why this is happening all the time in the `USE_PSA_CRYPTO` build but not the others. As the error seems to be coming from a call to a PSA API, I'm sure you guys will have more ideas than me about it.
Patater commented 2019-02-13 03:45:15 -06:00 (Migrated from github.com)

[test_use_psa_crypto_full_cmake_asan fails]

Yup, look into what's happening today and see if I can't make a fix.

> [test_use_psa_crypto_full_cmake_asan fails] Yup, look into what's happening today and see if I can't make a fix.
gilles-peskine-arm commented 2019-02-13 03:53:26 -06:00 (Migrated from github.com)

I only looked at a few failures in ssl-opt.sh but I suspect all are the same. The call to psa_crypto_init() returns 15 which is PSA_ERROR_INSUFFICIENT_ENTROPY.

I suspect this has to do with the way the entropy module and the PSA subsystem are initialized. Each works on its own, but in an application that calls both mbedtls_platform_setup and platform_crypto_init, depending on how you do it, it's possible to cause it to fail or to go into an infinite loop. See a discussion in https://github.com/ARMmbed/mbed-os-example-tls/pull/232#discussion_r249813184

> I only looked at a few failures in ssl-opt.sh but I suspect all are the same. The call to psa_crypto_init() returns 15 which is PSA_ERROR_INSUFFICIENT_ENTROPY. I suspect this has to do with the way the entropy module and the PSA subsystem are initialized. Each works on its own, but in an application that calls both `mbedtls_platform_setup` and `platform_crypto_init`, depending on how you do it, it's possible to cause it to fail or to go into an infinite loop. See a discussion in https://github.com/ARMmbed/mbed-os-example-tls/pull/232#discussion_r249813184
Patater commented 2019-02-13 04:56:25 -06:00 (Migrated from github.com)

One issue, which I think we need to resolve on the TLS side, is the change to main_test.function to make main() call psa_crypto_init() unconditionally. This will at least break PSA tests, where we check for psa_crypto_init() to fail in certain conditions (e.g. in our entropy injection tests). I'll leave a comment on the TLS PR.

One issue, which I think we need to resolve on the TLS side, is the change to main_test.function to make `main()` call `psa_crypto_init()` unconditionally. This will at least break PSA tests, where we check for `psa_crypto_init()` to fail in certain conditions (e.g. in our entropy injection tests). I'll leave a comment on the TLS PR.
gilles-peskine-arm (Migrated from github.com) requested changes 2019-02-13 07:35:09 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

I made the merge myself (https://github.com/gilles-peskine-arm/mbed-crypto/tree/prc_51-merge-gilles) and found a few differences, sometimes my mistake, sometimes yours, sometimes indifferent.

  • include/mbedtls/cipher.h: just whitespace, yours is better.
  • library/cipher.c: I missed a bugfix that was made in the crypto branch (remove dead code return( 0 ); in mbedtls_cipher_write_tag. Please indicate this in the commit message for the merge.
  • tests/scripts/all.sh: I missed removing USE_CRYPTO_SUBMODULE from component_test_use_psa_crypto_full_cmake_asan.
  • tests/scripts/run-test-suites.pl: you missed discarding the addition of a crypto submodule thing.
  • tests/suites/helpers.function: you missed some text when merging the two descriptions of TEST_ASSERT. Also some trivial whitespace differences.

This was a complicated merge and I'd like a more detailed commit message that contains sufficiently precise instructions to do the same merge (or at least up to trivial differences such as whitespace).

The post-merge commits up to 772ce79706 (I mistakenly posted the review from a tab that didn't have the latest commit) look good and I can't think of something else that would be missing.

I made this review under the assumption that https://github.com/ARMmbed/mbedtls/pull/2395 is correct. Since then you've found one bug (https://github.com/ARMmbed/mbedtls/pull/2395/commits/3ea2687053748e701f4d28a644c4c6ea28f3b251) which will need to be fixed here.

I made the merge myself (https://github.com/gilles-peskine-arm/mbed-crypto/tree/prc_51-merge-gilles) and found a few differences, sometimes my mistake, sometimes yours, sometimes indifferent. * `include/mbedtls/cipher.h`: just whitespace, yours is better. * `library/cipher.c`: I missed a bugfix that was made in the crypto branch (remove dead code `return( 0 );` in `mbedtls_cipher_write_tag`. Please indicate this in the commit message for the merge. * `tests/scripts/all.sh`: I missed removing `USE_CRYPTO_SUBMODULE` from `component_test_use_psa_crypto_full_cmake_asan`. * `tests/scripts/run-test-suites.pl`: you missed discarding the addition of a crypto submodule thing. * `tests/suites/helpers.function`: you missed some text when merging the two descriptions of `TEST_ASSERT`. Also some trivial whitespace differences. This was a complicated merge and I'd like a more detailed commit message that contains sufficiently precise instructions to do the same merge (or at least up to trivial differences such as whitespace). The post-merge commits up to 772ce797062c0114643aabb1390ac05f134f4e78 (I mistakenly posted the review from a tab that didn't have the latest commit) look good and I can't think of something else that would be missing. I made this review under the assumption that https://github.com/ARMmbed/mbedtls/pull/2395 is correct. Since then you've found one bug (https://github.com/ARMmbed/mbedtls/pull/2395/commits/3ea2687053748e701f4d28a644c4c6ea28f3b251) which will need to be fixed here.
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
gilles-peskine-arm (Migrated from github.com) commented 2019-02-13 06:09:03 -06:00

Which compiler requires this specific comment, as opposed to any comment or any comment containing “fallthrough”?

Which compiler requires this specific comment, as opposed to any comment or any comment containing “fallthrough”?
programs/psa/crypto_examples.c
@ -327,0 +335,4 @@
{
mbedtls_printf( "%s:%i: Input param failed - %s\n",
file, line, failure_condition );
mbedtls_exit( MBEDTLS_EXIT_FAILURE );
gilles-peskine-arm (Migrated from github.com) commented 2019-02-13 06:10:27 -06:00

Yuck, it's really ugly to have this in every example. But I guess that it's a design problem with MBEDTLS_CHECK_PARAMS that it's too late to fix.

Yuck, it's really ugly to have this in every example. But I guess that it's a design problem with `MBEDTLS_CHECK_PARAMS` that it's too late to fix.
tests/scripts/run-test-suites.pl
gilles-peskine-arm (Migrated from github.com) commented 2019-02-13 06:16:29 -06:00

Stray incursion of the crypto submodule.

Stray incursion of the crypto submodule.
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
gilles-peskine-arm (Migrated from github.com) commented 2019-02-13 06:19:06 -06:00

This is missing some useful text from the crypto branch:

+ *          Failing the test means:
+ *          - Mark this test case as failed.
+ *          - Print a message identifying the failure.
+ *          - Jump to the \c exit label.
+ *
+ *          This macro expands to an instruction, not an expression.
+ *          It may jump to the \c exit label.
+ *
This is missing some useful text from the crypto branch: ``` + * Failing the test means: + * - Mark this test case as failed. + * - Print a message identifying the failure. + * - Jump to the \c exit label. + * + * This macro expands to an instruction, not an expression. + * It may jump to the \c exit label. + * ```
Patater (Migrated from github.com) reviewed 2019-02-13 10:19:28 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
Patater (Migrated from github.com) commented 2019-02-13 10:19:27 -06:00 
$ gcc --version
gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
``` $ gcc --version gcc (Ubuntu 7.3.0-27ubuntu1~18.04) 7.3.0 Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ```
gilles-peskine-arm (Migrated from github.com) reviewed 2019-02-13 10:30:26 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
gilles-peskine-arm (Migrated from github.com) commented 2019-02-13 10:30:26 -06:00

Oh. Wait, I'm still confused: reading the documentation, I don't see a setting that recognizes /* fallthrough */ but not /* foo fallthrough bar */ — it should be /*-fallthrough*/ or /*@fallthrough@*/.

Oh. Wait, I'm still confused: reading the [documentation](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough_003d), I don't see a setting that recognizes `/* fallthrough */` but not `/* foo fallthrough bar */` — it should be `/*-fallthrough*/` or `/*@fallthrough@*/`.
Patater (Migrated from github.com) reviewed 2019-02-14 07:06:58 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
Patater (Migrated from github.com) commented 2019-02-14 07:06:58 -06:00

Maybe it's because of the surrounding text.

I've pasted the error into the commit message in https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-6

Maybe it's because of the surrounding text. I've pasted the error into the commit message in https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-6
gilles-peskine-arm (Migrated from github.com) reviewed 2019-02-14 07:27:19 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
gilles-peskine-arm (Migrated from github.com) commented 2019-02-14 07:27:19 -06:00

Ah, yes, got it. The last bullet point for -Wimplicit-fallthrough=3 allows fallthrough with optional surrounding whitespace, together with a few other variants. (And the previous two bullet points allow all-uppercase or titlecase variants.)

I'd prefer to change this comment to something like /*-fallthrough*/ to make it clear that it's a compiler-warning directive, as opposed to the comment above which is for humans.

Ah, yes, got it. The last bullet point for `-Wimplicit-fallthrough=3` allows `fallthrough` with optional surrounding whitespace, together with a few other variants. (And the previous two bullet points allow all-uppercase or titlecase variants.) I'd prefer to change this comment to something like `/*-fallthrough*/` to make it clear that it's a compiler-warning directive, as opposed to the comment above which is for humans.
Patater (Migrated from github.com) reviewed 2019-02-14 09:43:52 -06:00
tests/scripts/run-test-suites.pl
Patater (Migrated from github.com) commented 2019-02-14 09:43:52 -06:00

Removed

Removed
Patater (Migrated from github.com) reviewed 2019-02-14 09:44:13 -06:00
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
Patater (Migrated from github.com) commented 2019-02-14 09:44:13 -06:00

OK, re-added. We should consider adding this up to TLS to align if we want this text everywhere.

OK, re-added. We should consider adding this up to TLS to align if we want this text everywhere.
Patater commented 2019-02-14 10:33:50 -06:00 (Migrated from github.com)

Rebased to take in latest TLS PR and address code review feedback.

Previous branch at https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-pr-2

Diff against last branch at https://gist.github.com/Patater/5c8847915bd84b96725e6d3b0d322866

Rebased to take in latest TLS PR and address code review feedback. Previous branch at https://github.com/Patater/mbed-crypto/tree/update-dev-tls-dev-crypto-merge-pr-2 Diff against last branch at https://gist.github.com/Patater/5c8847915bd84b96725e6d3b0d322866
Patater commented 2019-02-14 10:37:03 -06:00 (Migrated from github.com)

The current base, if you want to reproduce my merge, is still github.com/ARMmbed/mbed-crypto@1fb011f2a6

The current base, if you want to reproduce my merge, is still https://github.com/ARMmbed/mbed-crypto/commit/1fb011f2a61b8ae817c0786d00b20adfefaf737e
Patater commented 2019-02-14 10:42:08 -06:00 (Migrated from github.com)

No more rebasing right now. Feel free to review.

No more rebasing right now. Feel free to review.
mpg commented 2019-02-15 01:50:12 -06:00 (Migrated from github.com)

The CI passed, except for know-fragile timing selftest on BSD in the Jenkins pr-merge run.

The CI passed, except for know-fragile timing selftest on BSD in the Jenkins pr-merge run.
mpg (Migrated from github.com) reviewed 2019-02-15 03:26:54 -06:00
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
mpg (Migrated from github.com) commented 2019-02-15 03:26:53 -06:00

@AndrzejKurek when you update the submodule in https://github.com/ARMmbed/mbedtls/pull/2395 could you also add a commit to include this text in our version as well?

@AndrzejKurek when you update the submodule in https://github.com/ARMmbed/mbedtls/pull/2395 could you also add a commit to include this text in our version as well?
mpg (Migrated from github.com) approved these changes 2019-02-15 03:29:11 -06:00
mpg (Migrated from github.com) left a comment

I'm approving based on my previous approval and the github-generated force-push diff since the version I previously reviewed.

I'm approving based on my previous approval and the github-generated force-push diff since the version I previously reviewed.
AndrzejKurek (Migrated from github.com) reviewed 2019-02-15 04:09:29 -06:00
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
AndrzejKurek (Migrated from github.com) commented 2019-02-15 04:09:28 -06:00

Sure, I wrote that down on my to-do list.

Sure, I wrote that down on my to-do list.
Patater (Migrated from github.com) reviewed 2019-02-15 04:14:24 -06:00
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
Patater (Migrated from github.com) commented 2019-02-15 04:14:24 -06:00

This can be done by cherry-picking 8954d0c ("Write documentation for TEST_ASSERT") and resolving the conflicts in the same way I did here, to give credit where credit is due and hopefully help avoid multiple implementation conflicts if we ever merge helpers.function between Mbed TLS and Mbed Crypto again.

This can be done by cherry-picking [8954d0c](https://github.com/ARMmbed/mbed-crypto/commit/8954d0c274fedb54f416d65255d61fb8b3e909f8) ("Write documentation for TEST_ASSERT") and resolving the conflicts in the same way I did here, to give credit where credit is due and hopefully help avoid multiple implementation conflicts if we ever merge helpers.function between Mbed TLS and Mbed Crypto again.
mpg (Migrated from github.com) reviewed 2019-02-15 04:46:15 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
mpg (Migrated from github.com) commented 2019-02-15 04:46:15 -06:00

@gilles-peskine-arm Or we could even go with

/* fallthrough */ /* for compilers */

(I'm not even kidding. (I think.))

@AndrzejKurek We'll need this side-ported to TLS. Perhaps this could be part of 2395? Either way, please let @k-stachowiak know as he's been assigned to fix this on the TLS side.

@gilles-peskine-arm Or we could even go with ``` /* fallthrough */ /* for compilers */ ``` (I'm not even kidding. (I think.)) @AndrzejKurek We'll need this side-ported to TLS. Perhaps this could be part of 2395? Either way, please let @k-stachowiak know as he's been assigned to fix this on the TLS side.
gilles-peskine-arm (Migrated from github.com) approved these changes 2019-02-15 09:04:47 -06:00
gilles-peskine-arm (Migrated from github.com) left a comment

I am happy with the new merge commit 67ea2c5e6d made with the updated development-psa PR https://github.com/ARMmbed/mbedtls/pull/2395 and the same mbed-crypto/development base as my previous review. I reviewed the new merge commit by diff with the previous one. I am also happy with the commit message. I am also happy with the additional commits on top of that (rebased from the previous iteration).

I am happy with the new merge commit 67ea2c5e6d9c624e23d8b104c35a019df34eaf5e made with the updated development-psa PR https://github.com/ARMmbed/mbedtls/pull/2395 and the same mbed-`crypto/development` base as my previous review. I reviewed the new merge commit by diff with the previous one. I am also happy with the commit message. I am also happy with the additional commits on top of that (rebased from the previous iteration).
gilles-peskine-arm commented 2019-02-15 09:05:44 -06:00 (Migrated from github.com)

2 approvals and CI is ok except for a FreeBSD timing test which is a known issue. This is ready for merge.

2 approvals and CI is ok except for a FreeBSD timing test which is a known issue. This is ready for merge.
AndrzejKurek (Migrated from github.com) reviewed 2019-02-15 09:25:42 -06:00
tests/suites/helpers.function
@ -83,1 +106,4 @@
* It may jump to the \c exit label.
*
* \param TEST The test expression to be tested.
*/
AndrzejKurek (Migrated from github.com) commented 2019-02-15 09:25:42 -06:00

Done.

Done.
AndrzejKurek (Migrated from github.com) reviewed 2019-02-15 09:25:53 -06:00
include/mbedtls/psa_util.h
@ -91,6 +91,7 @@ static inline psa_algorithm_t mbedtls_psa_translate_cipher_mode(
if( taglen == 0 )
return( PSA_ALG_CBC_NO_PADDING );
/* Intentional fallthrough for taglen != 0 */
/* fallthrough */
AndrzejKurek (Migrated from github.com) commented 2019-02-15 09:25:53 -06:00

Done.

Done.
Sign in to join this conversation.
Reviewers
No reviewers
mpg
gilles-peskine-arm
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!51
No description provided.