[2.28] Enable the timing.c selftest with MBEDTLS_TIMING_ALT #6931

AndrzejKurek · 2023-01-14T00:05:17Z

Description

This caused trouble for users that were using the selftest feature along with an alternative timing implementation. They were forced to provide their own version of a selftest. Since it was not mentioned in the define description, it should not be required, and is provided roughly as it was before breaking changes in 77daaad were introduced.
Bonus change: the FAIL macro no longer uses a field that exists only without MBEDTLS_TIMING_ALT.

Fixes #6923

Gatekeeper checklist

changelog
test - provided

backport not required

This caused trouble for users that were using the selftest feature along with an alternative implementation. They were forced to provide their own version of a selftest. Since it was not mentioned in the define description, it should not be required, and is provided roughly as it was before breaking changes in 77daaad were introduced. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm · 2023-01-16T10:05:16Z

This is for 2.28 - should there be a forward port? And since it's a bug fix, I guess there ought to be a ChangeLog entry.

Is there a way of catching something like this in the CI for next time?

AndrzejKurek · 2023-01-16T19:35:11Z

This is for 2.28 - should there be a forward port? And since it's a bug fix, I guess there ought to be a ChangeLog entry.

Is there a way of catching something like this in the CI for next time?

There should be no forward port, as there's no timing.c selftest on development (removed here).
As for the test on the CI... we could add a configuration that tests it with an external copy of Mbed TLS implementation in an additional all.sh module. Do you think that this will be enough?

gilles-peskine-arm · 2023-01-16T19:47:51Z

we could add a configuration that tests it with an external copy of Mbed TLS implementation in an additional all.sh module. Do you think that this will be enough?

We currently have build_module_alt and build_dhm_alt. They only test the compilation, not linking. Providing an external copy of the mbedtls implementation would allow testing linking, but it would also mean that the context types are the same, which wouldn't catch cases where the library makes assumptions on context types. So we'd either want to have both build-with-different-context-types and test-with-same-implementation, or just test-with-tweaked-implementation where the tweak changes the context type (e.g. renaming all the fields, or embedding the actual context into a wrapper struct — either way requiring a nontrivial change to the module code rather than a simple copy).

tom-cosgrove-arm · 2023-01-16T19:55:50Z

either way requiring a nontrivial change to the module code

This seems to me not worth it for the LTS then

gilles-peskine-arm · 2023-01-16T19:58:48Z

either way requiring a nontrivial change to the module code

This seems to me not worth it for the LTS then

The same technique should work for all modules (except ECP which has the additional complication that some context fields are public and need to keep their name). So it would be for the ALT modules in development as well.

But far too much work for this bug fix.

tom-cosgrove-arm · 2023-01-16T20:30:11Z

So it would be for the ALT modules in development as well

But at some point we are going to retire the ALT interface in favour of PSA drivers, so we should really not be putting more effort into it now

Copy the original implementation Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

AndrzejKurek · 2023-01-17T10:22:29Z

Updated with a changelog entry and a test with an alternate, but copied implementation.

ChangeLog.d/fix_timing_alt.txt

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tests/src/external_timing/timing_alt.h

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm

LGTM

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm

LGTM

tom-cosgrove-arm · 2023-01-18T07:28:16Z

CI all green

mprse

Looks good, but got question regarding testing in comments.

mprse · 2023-01-24T14:38:51Z

ChangeLog.d/fix_timing_alt.txt

@@ -0,0 +1,5 @@
+Bugfix
+   * Fix a build issue when defining MBEDTLS_TIMING_ALT and MBEDTLS_SELF_TEST.
+     The library would not link if the user didn't provide an external selftest


Suggested change

The library would not link if the user didn't provide an external selftest

The library would not link if the user didn't provide an external self-test

mprse · 2023-01-24T15:08:53Z

library/timing.c

@@ -383,9 +383,8 @@ static void busy_msleep(unsigned long msec)
            mbedtls_printf(" cycles=%lu ratio=%lu millisecs=%lu secs=%lu hardfail=%d a=%lu b=%lu\n", \
                           cycles, ratio, millisecs, secs, hardfail,   \
                           (unsigned long) a, (unsigned long) b);     \
-            mbedtls_printf(" elapsed(hires)=%lu elapsed(ctx)=%lu status(ctx)=%d\n", \


Commit mentions about enabling self-test when MBEDTLS_TIMING_ALT is defined and that is ok, but why information about delay time is removed?

Because it was expecting a certain internal field from the context (ctx.timer), and the selftest should be implementation-agnostic.

mprse · 2023-01-24T21:51:31Z

tests/scripts/all.sh

+    make lib TEST_TIMING_ALT_IMPL=1 CFLAGS="-I../tests/src/external_timing"
+
+    msg "test: MBEDTLS_TIMING_ALT - test suites"
+    make test TEST_TIMING_ALT_IMPL=1 CFLAGS="-I../tests/src/external_timing"


We are building external_timing_for_test.c and we have MBEDTLS_TIMING_ALT set, so now we will have mbedtls_timing_self_test that will use external timing functions. Is that correct?

We want to test that mbedtls_timing_self_test works in this configuration, but I don't see test suite that execute mbedtls_timing_self_test.

I see that mbedtls_timing_self_test is executes in mbedtls/programs/test/selftest.c.

Normally we have a unit test that runs the self-test function. This is not the case for timing. It used to, then in #1136 we replaced the self-test by unit tests that were faster (the self-test takes 1 second of CPU time) and more reliable (the self-test can fail on a heavily loaded machine). The reasons for not running the timing self-test function from the unit tests still apply, so we should build and run the selftest program here.

Sure, the addition of test suite run was unnecessary, but the problem we tried to solve here was linking the library with a certain set of defines, not building / running programs. There are other problems when we're trying to build them with an alternate timing implementation (fuzzer doesn't build, probably some Makefile shenanigans). Should we resolve this here too? I wanted to opt for a minimal solution that fixes an existing problem.

I would say let's fix the existing problem, then raise an issue to capture the other concerns

Added it here after all, it was just a matter of the fuzz programs having one level of directories nesting more, so not a big deal. Edit: I also went with leaving the test suite run in, as there are some tests from the timing test suite that will be run with the alternate implementation too.

The fuzz programs require one layer of directories more when adding include directories. Also remove an unnecessary include directory in the Makefile. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm

LGTM, pending CI pass

mprse

LGTM!

AndrzejKurek added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review priority-high High priority - will be reviewed soon labels Jan 14, 2023

AndrzejKurek mentioned this pull request Jan 14, 2023

selftest.c doesn't working anymore with < 2.28.2 : MBEDTLS_SELF_TEST and MBEDTLS_TIMING_ALT #6923

Closed

Add a test for external alternate timing implementation

c890b22

Copy the original implementation Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm reviewed Jan 17, 2023

View reviewed changes

ChangeLog.d/fix_timing_alt.txt Outdated Show resolved Hide resolved

Add a changelog entry

b36fa91

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

AndrzejKurek force-pushed the timeless-selftest-waz-bad branch from 1ee8bda to b36fa91 Compare January 17, 2023 10:28

tom-cosgrove-arm reviewed Jan 17, 2023

View reviewed changes

tests/src/external_timing/timing_alt.h Outdated Show resolved Hide resolved

Fix a copy-paste error in define comment

721cff1

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm previously approved these changes Jan 17, 2023

View reviewed changes

Shorten lines of changelog entry

4136a9e

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

AndrzejKurek dismissed tom-cosgrove-arm’s stale review via 4136a9e January 17, 2023 11:09

tom-cosgrove-arm previously approved these changes Jan 17, 2023

View reviewed changes

tom-cosgrove-arm removed the needs-ci Needs to pass CI tests label Jan 18, 2023

mprse self-requested a review January 24, 2023 14:36

mprse removed the needs-reviewer This PR needs someone to pick it up for review label Jan 24, 2023

mprse reviewed Jan 25, 2023

View reviewed changes

Add a selftest run for the TIMING_ALT test

3a261a4

The fuzz programs require one layer of directories more when adding include directories. Also remove an unnecessary include directory in the Makefile. Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

AndrzejKurek dismissed tom-cosgrove-arm’s stale review via 3a261a4 January 26, 2023 09:34

AndrzejKurek requested review from mprse and tom-cosgrove-arm January 26, 2023 09:37

Be consistent with self-test naming

26d3839

Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>

tom-cosgrove-arm added the needs-ci Needs to pass CI tests label Jan 26, 2023

tom-cosgrove-arm approved these changes Jan 26, 2023

View reviewed changes

mprse approved these changes Jan 26, 2023

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jan 26, 2023

gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Jan 26, 2023

gilles-peskine-arm merged commit f57f3db into Mbed-TLS:mbedtls-2.28 Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.28] Enable the timing.c selftest with MBEDTLS_TIMING_ALT #6931

[2.28] Enable the timing.c selftest with MBEDTLS_TIMING_ALT #6931

AndrzejKurek commented Jan 14, 2023 •

edited

Loading

tom-cosgrove-arm commented Jan 16, 2023

AndrzejKurek commented Jan 16, 2023

gilles-peskine-arm commented Jan 16, 2023

tom-cosgrove-arm commented Jan 16, 2023 •

edited

Loading

gilles-peskine-arm commented Jan 16, 2023

tom-cosgrove-arm commented Jan 16, 2023

AndrzejKurek commented Jan 17, 2023

tom-cosgrove-arm left a comment

tom-cosgrove-arm left a comment

tom-cosgrove-arm commented Jan 18, 2023

mprse left a comment

mprse Jan 24, 2023

AndrzejKurek Jan 26, 2023

mprse Jan 24, 2023

AndrzejKurek Jan 25, 2023

mprse Jan 24, 2023

gilles-peskine-arm Jan 25, 2023

AndrzejKurek Jan 25, 2023 •

edited

Loading

tom-cosgrove-arm Jan 25, 2023

AndrzejKurek Jan 26, 2023 •

edited

Loading

tom-cosgrove-arm left a comment

mprse left a comment

	The library would not link if the user didn't provide an external selftest
	The library would not link if the user didn't provide an external self-test

[2.28] Enable the timing.c selftest with MBEDTLS_TIMING_ALT #6931

[2.28] Enable the timing.c selftest with MBEDTLS_TIMING_ALT #6931

Conversation

AndrzejKurek commented Jan 14, 2023 • edited Loading

Description

Gatekeeper checklist

tom-cosgrove-arm commented Jan 16, 2023

AndrzejKurek commented Jan 16, 2023

gilles-peskine-arm commented Jan 16, 2023

tom-cosgrove-arm commented Jan 16, 2023 • edited Loading

gilles-peskine-arm commented Jan 16, 2023

tom-cosgrove-arm commented Jan 16, 2023

AndrzejKurek commented Jan 17, 2023

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

tom-cosgrove-arm commented Jan 18, 2023

mprse left a comment

Choose a reason for hiding this comment

mprse Jan 24, 2023

Choose a reason for hiding this comment

AndrzejKurek Jan 26, 2023

Choose a reason for hiding this comment

mprse Jan 24, 2023

Choose a reason for hiding this comment

AndrzejKurek Jan 25, 2023

Choose a reason for hiding this comment

mprse Jan 24, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Jan 25, 2023

Choose a reason for hiding this comment

AndrzejKurek Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

tom-cosgrove-arm Jan 25, 2023

Choose a reason for hiding this comment

AndrzejKurek Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

mprse left a comment

Choose a reason for hiding this comment

AndrzejKurek commented Jan 14, 2023 •

edited

Loading

tom-cosgrove-arm commented Jan 16, 2023 •

edited

Loading

AndrzejKurek Jan 25, 2023 •

edited

Loading

AndrzejKurek Jan 26, 2023 •

edited

Loading