Changes after code review

Signed-off-by: TRodziewicz <tomasz.rodziewicz@mobica.com>

ChangeLog.d/issue4313.txt

@@ -1,25 +1,4 @@
 Removals
    * Remove the following macros: MBEDTLS_CHECK_PARAMS,
      MBEDTLS_CHECK_PARAMS_ASSERT, MBEDTLS_PARAM_FAILED,
-     MBEDTLS_PARAM_FAILED_ALT, TEST_INVALID_PARAM, TEST_INVALID_PARAM_RET,
+     MBEDTLS_PARAM_FAILED_ALT. Fixes #4313.
-     the following macros have been inactivated MBEDTLS_INTERNAL_VALIDATE_RET
-     and MBEDTLS_INTERNAL_VALIDATE, structures: param_failed_ctx_t,
-     mbedtls_test_param_failed_location_record_t, functions:
-     mbedtls_test_param_failed_get_location_record(),
-     mbedtls_test_param_failed_expect_call(),
-     mbedtls_test_param_failed_check_expected_call(),
-     mbedtls_test_param_failed_get_state_buf(),
-     mbedtls_test_param_failed_reset_state(),
-     mbedtls_param_failed(). Remove the following functions from all.sh:
-     component_test_check_params_functionality(),
-     component_test_check_params_without_platform(),
-     component_test_check_params_silent().
-     Remove the following test functions from test_suite_*.function files:
-     aes_check_params(), aria_invalid_param(), blowfish_invalid_param(),
-     camellia_invalid_param(), ccm_invalid_param(), chacha20_bad_params(),
-     chachapoly_bad_params(), cipher_invalid_param_conditional(),
-     dhm_invalid_params(), ecdh_invalid_param(), ecdsa_invalid_param(),
-     ecjpake_invalid_param(), ecp_invalid_param(), gcm_invalid_param(),
-     mpi_invalid_param(), invalid_parameters() (pk), poly1305_bad_params(),
-     rsa_invalid_param(), sha1_invalid_param(), sha256_invalid_param(),
-     sha512_invalid_param(). Fixes #4313.

docs/3.0-migration-guide.d/remove_mbedtls_check_params_option.md

@@ -1,10 +1,11 @@
 Remove MBEDTLS_CHECK_PARAMS option
 ----------------------------------
 
-This change affects the way of how parameters are validated.
+This change does not affect users who use the default configuration; it only
+affects users who enabled that option.
 
-The option `MBEDTLS_CHECK_PARAMS` (disabled by default) enables certain kinds of
+The option `MBEDTLS_CHECK_PARAMS` (disabled by default) enabled certain kinds
-“parameter validation”. It covers two kinds of validations:
+of “parameter validation”. It covered two kinds of validations:
 
 - In some functions that require a valid pointer, “parameter validation” checks
 that the pointer is non-null. With the feature disabled, a null pointer is not
@@ -14,34 +15,17 @@ runtime crash. 90% of the uses of the feature are of this kind.
 checks that the value is a valid one. With the feature disabled, an invalid
 value causes a silent default to one of the valid values.
 
-The default reaction to a failed check is to call a function mbedtls_param_failed
+The default reaction to a failed check was to call a function
-which the application must provide. If this function returns, its caller returns
+`mbedtls_param_failed()` which the application had to provide. If this function
-an error `MBEDTLS_ERR_xxx_BAD_INPUT_DATA`.
+returned, its caller returned an error `MBEDTLS_ERR_xxx_BAD_INPUT_DATA`.
 
-This feature is only used in some classic (non-PSA) cryptography modules. It is
+This feature was only used in some classic (non-PSA) cryptography modules. It was
-not used in X.509, TLS or in PSA crypto, and it has not been implemented in all
+not used in X.509, TLS or in PSA crypto, and it was not implemented in all
 classic crypto modules.
 
-Removal of `MBEDTLS_CHECK_PARAMS` and all dependent features means changing
+This feature has been removed. The library no longer checks for NULL pointers;
-code that does something like this:
+checks for enum-like arguments will be kept or re-introduced on a case-by-case
-```
+basis, but their presence will no longer be dependent on a compile-time option.
-#if MBEDTLS_CHECK_PARAMS
-#define VALIDATE(cond) do {if(cond) return BAD_INPUT_DATA;} while (0)
-#else
-#define VALIDATE(cond) do {} while (0)
-#endif
-...
-VALIDATE(coin == HEADS || coin == TAILS);
-VALIDATE(data != NULL);
-if (coin == HEADS) heads();
-else tails();
-```
-to something like this:
-```
-if (coin == HEADS) heads();
-else if (coin == TAILS) tails();
-else return BAD_INPUT_DATA;
-```
 
 Validation of enum-like values is somewhat useful, but not extremely important,
 because the parameters concerned are usually constants in applications.

library/psa_crypto.c

@@ -1021,10 +1021,7 @@ psa_status_t psa_wipe_key_slot( psa_key_slot_t *slot )
 
     /*
      * As the return error code may not be handled in case of multiple errors,
-     * do our best to report an unexpected lock counter: if available
+     * do our best to report an unexpected lock counter.
-     * call MBEDTLS_PARAM_FAILED that may terminate execution (if called as
-     * part of the execution of a test suite this will stop the test suite
-     * execution).
      */
     if( slot->lock_count != 1 )
     {

tests/include/test/macros.h

@@ -177,14 +177,6 @@
         }                                                   \
     } while( 0 )
 
-#if defined(MBEDTLS_CHECK_PARAMS) && !defined(MBEDTLS_PARAM_FAILED_ALT)
-#define TEST_INVALID_PARAM_RET( PARAM_ERR_VALUE, TEST )     \
-    do { if( ( TEST ) != ( PARAM_ERR_VALUE ) ) goto exit; } while( 0 )
-
-#define TEST_INVALID_PARAM( TEST )                          \
-    do { TEST; } while( 0 )
-#endif /* MBEDTLS_CHECK_PARAMS && !MBEDTLS_PARAM_FAILED_ALT */
-
 /**
  * \brief   This macro tests the statement passed to it as a test step or
  *          individual test in a test case. The macro assumes the test will not fail.

tests/suites/test_suite_aes.function

@@ -206,9 +206,6 @@ void aes_crypt_xts_size( int size, int retval )
     mbedtls_aes_xts_init( &ctx );
     memset( data_unit, 0x00, sizeof( data_unit ) );
 
-
-    /* Valid pointers are passed for builds with MBEDTLS_CHECK_PARAMS, as
-     * otherwise we wouldn't get to the size check we're interested in. */
     TEST_ASSERT( mbedtls_aes_crypt_xts( &ctx, MBEDTLS_AES_ENCRYPT, length, data_unit, src, output ) == retval );
 }
 /* END_CASE */
@@ -359,7 +356,7 @@ exit:
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void aes_invalid_mode( )
 {
     mbedtls_aes_context aes_ctx;

tests/suites/test_suite_aria.function

@@ -23,7 +23,7 @@ void aria_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void aria_invalid_param( )
 {
     mbedtls_aria_context ctx;

tests/suites/test_suite_blowfish.function

@@ -14,7 +14,7 @@ void blowfish_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void blowfish_invalid_param( )
 {
     mbedtls_blowfish_context ctx;

tests/suites/test_suite_camellia.function

@@ -14,7 +14,7 @@ void camellia_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void camellia_invalid_param( )
 {
     mbedtls_camellia_context ctx;

tests/suites/test_suite_cipher.function

@@ -207,7 +207,7 @@ exit:
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void cipher_invalid_param_conditional( )
 {
     mbedtls_cipher_context_t valid_ctx;

tests/suites/test_suite_ecdh.function

@@ -50,7 +50,7 @@ void ecdh_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void ecdh_invalid_param( )
 {
     mbedtls_ecdh_context ctx;

tests/suites/test_suite_ecjpake.function

@@ -98,7 +98,7 @@ cleanup:
  * END_DEPENDENCIES
  */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void ecjpake_invalid_param( )
 {
     mbedtls_ecjpake_context ctx;

tests/suites/test_suite_ecp.function

@@ -38,7 +38,7 @@ exit:
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void ecp_invalid_param( )
 {
     mbedtls_ecp_group grp;

tests/suites/test_suite_gcm.function

@@ -181,7 +181,7 @@ exit:
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:NOT_DEFINED */
 void gcm_invalid_param( )
 {
     mbedtls_gcm_context ctx;

tests/suites/test_suite_shax.data

@@ -1,9 +1,6 @@
 SHA-1 - Valid parameters
 sha1_valid_param:
 
-SHA-1 - Invalid parameters
-sha1_invalid_param:
-
 # Test the operation of SHA-1 and SHA-2
 SHA-1 Test Vector NIST CAVS #1
 depends_on:MBEDTLS_SHA1_C

tests/suites/test_suite_shax.function

@@ -11,46 +11,6 @@ void sha1_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_SHA1_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
-void sha1_invalid_param( )
-{
-    mbedtls_sha1_context ctx;
-    unsigned char buf[64] = { 0 };
-    size_t const buflen = sizeof( buf );
-
-    TEST_INVALID_PARAM( mbedtls_sha1_init( NULL ) );
-
-    TEST_INVALID_PARAM( mbedtls_sha1_clone( NULL, &ctx ) );
-    TEST_INVALID_PARAM( mbedtls_sha1_clone( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_starts_ret( NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_update_ret( NULL, buf, buflen ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_update_ret( &ctx, NULL, buflen ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_finish_ret( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_finish_ret( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_internal_sha1_process( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_internal_sha1_process( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_ret( NULL, buflen, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA,
-                            mbedtls_sha1_ret( buf, buflen, NULL ) );
-
-exit:
-    return;
-}
-/* END_CASE */
-
 /* BEGIN_CASE depends_on:MBEDTLS_SHA1_C */
 void mbedtls_sha1( data_t * src_str, data_t * hash )
 {
@@ -72,7 +32,7 @@ void sha256_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_SHA256_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:MBEDTLS_SHA256_C:NOT_DEFINED */
 void sha256_invalid_param( )
 {
     mbedtls_sha256_context ctx;
@@ -81,38 +41,10 @@ void sha256_invalid_param( )
     int valid_type = 0;
     int invalid_type = 42;
 
-    TEST_INVALID_PARAM( mbedtls_sha256_init( NULL ) );
+    TEST_EQUAL( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-
-    TEST_INVALID_PARAM( mbedtls_sha256_clone( NULL, &ctx ) );
-    TEST_INVALID_PARAM( mbedtls_sha256_clone( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_starts_ret( NULL, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
                             mbedtls_sha256_starts_ret( &ctx, invalid_type ) );
 
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
+    TEST_EQUAL( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_update_ret( NULL, buf, buflen ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_update_ret( &ctx, NULL, buflen ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_finish_ret( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_finish_ret( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_internal_sha256_process( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_internal_sha256_process( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_ret( NULL, buflen,
-                                                buf, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
-                            mbedtls_sha256_ret( buf, buflen,
-                                                NULL, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA,
                             mbedtls_sha256_ret( buf, buflen,
                                                 buf, invalid_type ) );
 
@@ -156,7 +88,7 @@ void sha512_valid_param( )
 }
 /* END_CASE */
 
-/* BEGIN_CASE depends_on:MBEDTLS_SHA512_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */
+/* BEGIN_CASE depends_on:MBEDTLS_SHA512_C:NOT_DEFINED */
 void sha512_invalid_param( )
 {
     mbedtls_sha512_context ctx;
@@ -165,38 +97,10 @@ void sha512_invalid_param( )
     int valid_type = 0;
     int invalid_type = 42;
 
-    TEST_INVALID_PARAM( mbedtls_sha512_init( NULL ) );
+    TEST_EQUAL( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-
-    TEST_INVALID_PARAM( mbedtls_sha512_clone( NULL, &ctx ) );
-    TEST_INVALID_PARAM( mbedtls_sha512_clone( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_starts_ret( NULL, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
                             mbedtls_sha512_starts_ret( &ctx, invalid_type ) );
 
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
+    TEST_EQUAL( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_update_ret( NULL, buf, buflen ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_update_ret( &ctx, NULL, buflen ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_finish_ret( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_finish_ret( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_internal_sha512_process( NULL, buf ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_internal_sha512_process( &ctx, NULL ) );
-
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_ret( NULL, buflen,
-                                                buf, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
-                            mbedtls_sha512_ret( buf, buflen,
-                                                NULL, valid_type ) );
-    TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA,
                             mbedtls_sha512_ret( buf, buflen,
                                                 buf, invalid_type ) );