Add mbed os conf tests #255

RonEld · 2019-09-11T15:27:57Z

Add a test in all.sh that configures the default configuration in Mbed OS and fix test dependencies discovered from it.

Add a test in `all.sh` that tests the deafuolt configuration in Mbed OS, as adjusted in the Mbed OS importer script, except setting `MBEDTLS_NO_PLATFORM_ENTROPY` as the tests need entropy source.

1. Add a dependency on `MBEDTLS_GENPRIME` for the test: "PSA generate key: RSA, 1024 bits, good, encrypt (OAEP SHA-256)". 2. Run the import RSA tests only if `MBEDTLS_MPI_MAXSIZE` is larger than `PSA_VENDOR_RSA_MAX_KEY_BITS+8` in bytes.

gilles-peskine-arm · 2019-09-11T15:40:19Z

tests/scripts/all.sh

+    scripts/config.pl unset MBEDTLS_FS_IO
+    scripts/config.pl unset MBEDTLS_PSA_ITS_FILE_C # requires a filesystem
+    scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA ITS
+    #scripts/config.pl set MBEDTLS_NO_PLATFORM_ENTROPY Need some entropy


Why is this line commented out?

Because we need some entropy sources, otherwise psa_crypto_init() will fail

gilles-peskine-arm · 2019-09-11T15:40:37Z

tests/scripts/all.sh

+    scripts/config.pl unset MBEDTLS_SSL_TRUNCATED_HMAC
+    scripts/config.pl unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO
+    scripts/config.pl set MBEDTLS_MPI_MAX_SIZE     512
+    make CC=gcc CFLAGS='-Werror -O1' all test


Maybe do a 32-bit build to be slightly closer to Mbed OS?

ok.
The idea was to check Mbed OS configuration, but this won't harm

gilles-peskine-arm · 2019-09-11T15:42:03Z

tests/scripts/all.sh

@@ -1120,6 +1120,70 @@ component_test_cmake_out_of_source () {
    unset MBEDTLS_ROOT_DIR
 }

+component_test_basic_mbed_os_conf () {
+msg "build+test: Mbed OS default configuration"
+    scripts/config.pl unset MBEDTLS_NET_C


Rather than call config.pl a bunch of times, how about defining the Mbed OS configuration in a file in configs? Or as a filter in config.pl, if we want to define it by comparison to the default configuration?

gilles-peskine-arm · 2019-09-11T15:42:29Z

General question: where is the Mbed OS configuration defined?

RonEld · 2019-09-11T16:44:58Z

General question: where is the Mbed OS configuration defined?

When Mbed TLS is imported to Mbed OS, the import script is run, adjusting the Mbed TLS config, in the adjust_config.sh script.

gilles-peskine-arm · 2019-09-11T16:50:50Z

When Mbed TLS is imported to Mbed OS, the import script is run, adjusting the Mbed TLS config, in the adjust_config.sh script.

Thanks. Could we move that script to the Mbed TLS repository, and use that from all.sh? Looking at its commit history, it's mostly modified by Mbed TLS team members.

RonEld · 2019-09-12T08:35:10Z

Could we move that script to the Mbed TLS repository, and use that from all.sh? Looking at its commit history, it's mostly modified by Mbed TLS team members.

I think it's doable, and in this case, preferable, but I think we should hear what @Patater has to say on this, since he is more familiar with the script

Patater · 2019-09-12T09:38:51Z

Could we move that script to the Mbed TLS repository, and use that from all.sh? Looking at its commit history, it's mostly modified by Mbed TLS team members.

I think it's doable, and in this case, preferable, but I think we should hear what @Patater has to say on this, since he is more familiar with the script

Could we make a "reference configuration" for Mbed OS (to live in Mbed TLS's reference configs folder)? We can make the importer just copy over the Mbed OS reference configuration instead of shell-script-modifying it. This means we can delete some code. :)

gilles-peskine-arm · 2019-09-12T09:52:40Z

Could we make a "reference configuration" for Mbed OS (to live in Mbed TLS's reference configs folder)?

It depends whether we want to define the Mbed OS configuration by enumerating its contents or relative to the default configuration. If we add a new option which is enabled by default, should it be also enabled in Mbed OS by default?

Furthermore, if we make a reference configuration, how do we document it? It needs to be documented so that people can make informed choices when making custom builds of Mbed OS.

RonEld · 2019-10-22T12:44:17Z

As Gilles mentioned,
If we have a reference configuration for Mbed OS, that will be used by the importer script, it means that every new configuration in the main config.h file, will be need to be added to the reference configuration.
In my opinion we should have the importer script take the main configuration file, and script modify it to Mbed OS related features, like it does now, otherwise we will encounter numerous maintenance issues.

Patater · 2019-10-22T15:56:53Z

As Gilles mentioned,
If we have a reference configuration for Mbed OS, that will be used by the importer script, it means that every new configuration in the main config.h file, will be need to be added to the reference configuration.
In my opinion we should have the importer script take the main configuration file, and script modify it to Mbed OS related features, like it does now, otherwise we will encounter numerous maintenance issues.

What's the best way to ship a differential reference configuration?

Is it a header file which would be included via MBEDTLS_USER_CONFIG_FILE?
Is it a shell script that modifies the default configuration via a series of config.py calls?

I feel like the first option would be more portable and slightly easier to maintain, but interested to hear your thoughts as well.

Patater · 2019-10-28T11:49:05Z

tests/scripts/all.sh

+    scripts/config.pl unset MBEDTLS_SSL_TRUNCATED_HMAC
+    scripts/config.pl unset MBEDTLS_PLATFORM_TIME_TYPE_MACRO
+    scripts/config.pl set MBEDTLS_MPI_MAX_SIZE     512
+    make CC=gcc CFLAGS='-Werror -O1' all test


Should we also add -Wstack-usage=2048 -Werror to ensure we don't overflow the Mbed OS default main thread stack?

We should also use -Os, if that's what Mbed OS will default to using.

Gah, seems we can't enforce that stack limit until we can fit within that stack limit.

$ make CFLAGS:="-Os -Wstack-usage=2048 -Werror" check CC bignum.c bignum.c: In function ‘mbedtls_mpi_exp_mod’: bignum.c:1926:5: error: stack usage is 3312 bytes [-Werror=stack-usage=] int mbedtls_mpi_exp_mod( mbedtls_mpi *X, const mbedtls_mpi *A, ^~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Makefile:150: recipe for target 'bignum.o' failed make[2]: *** [bignum.o] Error 1 Makefile:158: recipe for target 'libmbedcrypto.a' failed make[1]: *** [libmbedcrypto.a] Error 2 Makefile:19: recipe for target 'lib' failed make: *** [lib] Error 2

What stack size should we test that we don't exceed and recommend Mbed OS users configure if they want to use Mbed TLS? We appear to fit within 6000 bytes.

Gen test_suite_version.c CC test_suite_version.c suites/host_test.function: In function ‘execute_tests’: suites/host_test.function:494:5: error: stack usage might be 5968 bytes [-Werror=stack-usage=] int execute_tests( int argc , const char ** argv ) ^~~~~~~~~~~~~

Stack consumption in mbedtls_mpi_exp_mod increases exponentially with MBEDTLS_MPI_WINDOW_SIZE. It's a space/time compromise, with ~1.5kB difference in stack usage between the min and max value on a 32-bit machine. Maybe we could reduce this value in Mbed OS: currently it uses the best-performance value except on TARGET_STM32F439xI.

The library and the test suite are written in a very different style when it comes to stack consumption. execute_tests uses a char buf[5000] to store a line of the .data file. I think we should -Wstack-usage only for the library, not for the test suite.

I agree we should run with -Wstack-usage only for the library for this PR.

In the future, we should build the tests as they'd be used on target, perhaps with our actual on-target testing capability, (so, likely excluding host_test.function): this enables us to catch when on-target testing would run into stack issues. Some test suites allocate unnecessarily large buffers on the stack. We should have a regression test in place to ensure issues like what #278 fixes don't rear their heads again, but adding such tests in this PR is out of scope.

mpg · 2020-03-26T10:55:25Z

It seems to me that with Mbed TLS recently becoming more independent from Mbed OS, it's no longer clear if this test should live in the mbedtls tree, especially if that requires having (an equivalent of) the Mbed OS config living in the Mbed TLS tree. @Patater @danh-arm wdyt?

mpg · 2020-04-01T10:05:22Z

Considering this PR does not appear to be close to be ready (still requires design discussions) or recently active, and we moving away from the mbed-crypto repo for new work, I'm closing this PR and encouraging you to re-open a similar one in mbedtls if you're still interested (perhaps after discussing if this is still appropriate now that Mbed TLS is less strongly connected to Mbed OS).

Ron Eldor added 2 commits September 11, 2019 18:26

Add a test for Mbed OS configuration

19a7e6c

Add a test in `all.sh` that tests the deafuolt configuration in Mbed OS, as adjusted in the Mbed OS importer script, except setting `MBEDTLS_NO_PLATFORM_ENTROPY` as the tests need entropy source.

Fix test dependencies in test_psa_crypto

ddc2fed

1. Add a dependency on `MBEDTLS_GENPRIME` for the test: "PSA generate key: RSA, 1024 bits, good, encrypt (OAEP SHA-256)". 2. Run the import RSA tests only if `MBEDTLS_MPI_MAXSIZE` is larger than `PSA_VENDOR_RSA_MAX_KEY_BITS+8` in bytes.

gilles-peskine-arm added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 11, 2019

gilles-peskine-arm reviewed Sep 11, 2019

View reviewed changes

Patater reviewed Oct 28, 2019

View reviewed changes

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Oct 29, 2019

mpg closed this Apr 1, 2020

Add mbed os conf tests #255

Add mbed os conf tests #255

Uh oh!

Conversation

RonEld commented Sep 11, 2019

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!

gilles-peskine-arm commented Sep 11, 2019

Uh oh!

RonEld commented Sep 11, 2019

Uh oh!

gilles-peskine-arm commented Sep 11, 2019

Uh oh!

RonEld commented Sep 12, 2019

Uh oh!

Patater commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Sep 12, 2019

Uh oh!

RonEld commented Oct 22, 2019

Uh oh!

Patater commented Oct 22, 2019

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!

Patater Oct 28, 2019 • 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!

mpg commented Mar 26, 2020

Uh oh!

mpg commented Apr 1, 2020

Uh oh!

Uh oh!

Patater commented Sep 12, 2019 •

edited

Loading

Patater Oct 28, 2019 •

edited

Loading