Improve implementation of Mbed TLS timing #14772
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ciarmcom
added
the
release-type: patch
|
@LDong-Arm, thank you for your changes. |
LDong-Arm
force-pushed
the
mbedtls_timing
branch
from
c3bff22
to
20e533e
Compare
Patater
previously requested changes
Jun 14, 2021
LDong-Arm
force-pushed
the
mbedtls_timing
branch
from
dd98f02
to
ca7c4ab
Compare
LDong-Arm
force-pushed
the
mbedtls_timing
branch
from
ca7c4ab
to
0c76789
Compare
Patater
previously requested changes
Jun 14, 2021
Previously we used `gettimeofday()` for Mbed TLS timing, but its implementation provided by Mbed OS is only precise to seconds. The microsecond component of the output `struct timeval` is always set to zero. But Mbed TLS requires millisecond precision. To provide required timing precision, switch to use `LowPowerTicker` or (microsecond) `Ticker`. `LowPowerTicker` is preferred as it saves power and Mbed TLS does not require microsecond precision.
When MBEDTLS_TIMING_C and MBEDTLS_TIMING_ALT are enabled,
the Arm Compiler generates errors like the following (one for
each missing symbol):
Error: L6218E: Undefined symbol mbedtls_timing_get_delay
Reason:
The function `mbedtls_timing_self_test()` in the Mbed TLS default
`timing.c` always gets compiled, if MBEDTLS_SELF_TEST is defined.
And MBEDTLS_SELF_TEST is always defined, as we have a Greentea test
to run some of the Mbed TLS self tests. (In the future we should try
not to enable MBEDTLS_SELF_TEST except for tests, but it requires
a rework in our test flow.)
`mbedtls_timing_self_test()` tests (calls) the full API declared in
`timing.h`, and the ARM Compiler requires all symbols referenced by
all functions to be defined, even those not used by the final
application. This is unlike GCC_ARM which resolves what are required.
Solution:
To fix the "undefined symbol" errors, we add an implementation of
`mbedtls_timing_get_timer()` based on Mbed OS `LowPowerTimer` or
`Timer` (depending on which one is available), and copy Mbed TLS's
default `mbedtls_timing_set_delay()` and `mbedtls_timing_get_delay()`
which are built on top of `mbedtls_timing_get_timer()`. This will also
benefit user applications that need to enable timing in Mbed TLS.
Do not compile the Mbed implementation of Mbed TLS unless MBEDTLS_TIMING_ALT is defined. This prevents a macro check error on devices that do not have LPTICKER or USTICKER when Mbed TLS timing is not enabled.
The function `mbedtls_set_alarm()` is only precise to seconds, so `LowPowerTimeout` is enough and saves power.
This allows us to verify the support for Mbed TLS timing on Mbed OS. Note: The macros MBEDTLS_TIMING_C and MBEDTLS_TIMING_ALT are not enabled by default and need to be additionally enabled to run this test.
LDong-Arm
force-pushed
the
mbedtls_timing
branch
from
0c76789
to
da542bb
Compare
Patater
previously requested changes
Jun 15, 2021
We potentially save flash space by not enabling Mbed TLS self-tests by default. A new test config file, TESTS/configs/mbedtls.json, is provided to enable self tests. This newly created JSON file also enables timing in Mbed TLS so timing gets tested.
LDong-Arm
force-pushed
the
mbedtls_timing
branch
from
da542bb
to
49163f0
Compare
Mbed Core
automation
moved this from Review in progress (4)
to Reviewer approved & awaiting CI & merge (3)
Jun 15, 2021
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
Mbed Core
automation
moved this from Reviewer approved & awaiting CI & merge (3)
to Done
Jun 15, 2021
|
MBEDTLS seltests are not executed by default now. Before: Now: |
| "macros": [ | ||
| "MBEDTLS_SELF_TEST", | ||
| "MBEDTLS_TIMING_C", | ||
| "MBEDTLS_TIMING_ALT" |
There was a problem hiding this comment.
_ALT should be enabled only for HW crypto targets ?
There was a problem hiding this comment.
Mbed TLS has a number of _ALT macros, but MBEDTLS_TIMING_ALT is only to enable timing based on Mbed OS API (i.e. LowPowerTicker/Ticker). This one is not related to crypto capabilities.
This is expected. Now if you want to enable Mbed TLS self tests, you need to pass |
mbedmain
removed
release-type: patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
This PR makes the following fixes and improvements to the Mbed OS implementation of Mbed TLS timing:
LowPowerTimerorTimerfor time measurements. The Mbed OS implementation of gettimeofday() is only precise to seconds, but Mbed TLS timing requires milliseconds.LowPowerTimeout(instead ofTimeout) for mbedtls_set_alarm() if available, to save power.timing.h, to fix "undefined symbol" errors generated by the Arm Compiler. This will also benefits user applications if they need to enable timing in Mbed TLS.mbedtls_timing_self_testto the existing Greentea testconnectivity-mbedtls-tests-TESTS-mbedtls-selftest, ifMBEDTLS_TIMING_Cis available.TESTS/configs/mbedtls.jsonto enable self tests and timing.Please see the commit messages for reasoning.
To run the timing self test (e.g. on K64F, with the ARM toolchain):
Impact of changes
Now compilation failure with the Arm Compiler with
MBEDTLS_TIMING_Cis fixed. For example, the benchmark application in mbed-os-example-tls depends on that.Additionally, a full implementation of Mbed TLS timing becomes available if an application needs it, and it can be enabled by defining
MBEDTLS_TIMING_CandMBEDTLS_TIMING_ALT.Migration actions required
None.
Documentation
None.
Pull request type
Test results
Manually ran the self test (see description above) and got
Reviewers
@ARMmbed/mbed-os-core @Patater @0xc0170