-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve implementation of Mbed TLS timing #14772
Conversation
@LDong-Arm, thank you for your changes. |
c3bff22
to
20e533e
Compare
dd98f02
to
ca7c4ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Patater Thanks for the review, I've addressed your comments.
ca7c4ab
to
0c76789
Compare
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.
0c76789
to
da542bb
Compare
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.
da542bb
to
49163f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ALT should be enabled only for HW crypto targets ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Summary of changes
This PR makes the following fixes and improvements to the Mbed OS implementation of Mbed TLS timing:
LowPowerTimer
orTimer
for 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_test
to the existing Greentea testconnectivity-mbedtls-tests-TESTS-mbedtls-selftest
, ifMBEDTLS_TIMING_C
is available.TESTS/configs/mbedtls.json
to 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_C
is 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_C
andMBEDTLS_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