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

Add mbed os conf tests #255

Closed
RonEld wants to merge 2 commits from add_mbed_os_conf_tests into development
pull from: add_mbed_os_conf_tests
merge into: gh-ARMmbed:development
Conversation 17 Commits 2 Files changed 2 +69 -3
RonEld commented 2019-09-11 10:27:57 -05:00 (Migrated from github.com)

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 configures the default configuration in `Mbed OS` and fix test dependencies discovered from it.
👍 1
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-11 10:42:10 -05:00
tests/scripts/all.sh
@ -1122,1 +1122,4 @@
component_test_basic_mbed_os_conf () {
msg "build+test: Mbed OS default configuration"
scripts/config.pl unset MBEDTLS_NET_C
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 10:42:03 -05:00

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?

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?
tests/scripts/all.sh
@ -1123,0 +1128,4 @@
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
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 10:40:19 -05:00

Why is this line commented out?

Why is this line commented out?
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
gilles-peskine-arm (Migrated from github.com) commented 2019-09-11 10:40:37 -05:00

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

Maybe do a 32-bit build to be slightly closer to Mbed OS?
gilles-peskine-arm commented 2019-09-11 10:42:29 -05:00 (Migrated from github.com)

General question: where is the Mbed OS configuration defined?

General question: where is the Mbed OS configuration defined?
RonEld (Migrated from github.com) reviewed 2019-09-11 11:42:28 -05:00
tests/scripts/all.sh
@ -1123,0 +1128,4 @@
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
RonEld (Migrated from github.com) commented 2019-09-11 11:42:28 -05:00

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

Because we need some entropy sources, otherwise `psa_crypto_init()` will fail
RonEld (Migrated from github.com) reviewed 2019-09-11 11:43:15 -05:00
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
RonEld (Migrated from github.com) commented 2019-09-11 11:43:15 -05:00

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

ok. The idea was to check Mbed OS configuration, but this won't harm
RonEld commented 2019-09-11 11:44:58 -05:00 (Migrated from github.com)

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.

>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`](https://github.com/ARMmbed/mbed-os/blob/master/features/mbedtls/importer/adjust-config.sh) script.
gilles-peskine-arm commented 2019-09-11 11:50:50 -05:00 (Migrated from github.com)

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.

> 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 commented 2019-09-12 03:35:10 -05:00 (Migrated from github.com)

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 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 commented 2019-09-12 04:38:51 -05:00 (Migrated from github.com)

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

> > 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 commented 2019-09-12 04:52:40 -05:00 (Migrated from github.com)

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.

> 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 commented 2019-10-22 07:44:17 -05:00 (Migrated from github.com)

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.

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 commented 2019-10-22 10:56:53 -05:00 (Migrated from github.com)

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.

> 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 (Migrated from github.com) reviewed 2019-10-28 06:49:05 -05:00
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
Patater (Migrated from github.com) commented 2019-10-28 06:49:05 -05:00

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.

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.
Patater (Migrated from github.com) reviewed 2019-10-28 06:56:21 -05:00
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
Patater (Migrated from github.com) commented 2019-10-28 06:56:07 -05:00

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 )
     ^~~~~~~~~~~~~
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 ) ^~~~~~~~~~~~~ ```
gilles-peskine-arm (Migrated from github.com) reviewed 2019-10-28 08:09:32 -05:00
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
gilles-peskine-arm (Migrated from github.com) commented 2019-10-28 08:09:31 -05:00

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.

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.
Patater (Migrated from github.com) reviewed 2019-10-28 08:22:06 -05:00
tests/scripts/all.sh
@ -1123,0 +1181,4 @@
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
Patater (Migrated from github.com) commented 2019-10-28 08:22:06 -05:00

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 https://github.com/ARMmbed/mbed-crypto/pull/278 fixes don't rear their heads again, but adding such tests in this PR is out of scope.

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 https://github.com/ARMmbed/mbed-crypto/pull/278 fixes don't rear their heads again, but adding such tests in this PR is out of scope.
mpg commented 2020-03-26 05:55:25 -05:00 (Migrated from github.com)

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?

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 commented 2020-04-01 05:05:22 -05:00 (Migrated from github.com)

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

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

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
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!255
No description provided.