gh-Mbed-TLS/TF-PSA-Crypto
Watch
1
0
Fork
You've already forked TF-PSA-Crypto
 0
Code Issues 167 Pull requests 92 Releases 5 Activity

Fix race condition in ECP self tests and improve testing with TSan #450

Open
gilles-peskine-arm wants to merge 8 commits from gilles-peskine-arm/ecp-self-test-race-crypto into development
pull from: gilles-peskine-arm/ecp-self-test-race-crypto
merge into: gh-Mbed-TLS:development
Conversation 7 Commits 8 Files changed 4 +198 -43
gilles-peskine-arm commented 2025-09-11 09:56:17 -05:00 (Migrated from github.com)

PR checklist

  • changelog not required because: self-tests are not an API feature in development (but 3.6 will need a changelog entry)
  • framework PR not required
  • mbedtls development PR TODO (all.sh components only)
  • mbedtls 3.6 PR TODO
  • tests provided
* Fix https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/443. * Test with TSan both with `MBEDTLS_TEST_HOOKS` enabled and with it disabled. ## PR checklist - [x] **changelog** not required because: self-tests are not an API feature in development (but 3.6 will need a changelog entry) - [x] **framework PR** not required - [ ] **mbedtls development PR** TODO (`all.sh` components only) - [ ] **mbedtls 3.6 PR** TODO - **tests** provided
mpg (Migrated from github.com) approved these changes 2025-09-15 03:05:24 -05:00
mpg (Migrated from github.com) left a comment

Looking good to me, I only have two questions to improve my understanding, but none of them is a blocker.

Looking good to me, I only have two questions to improve my understanding, but none of them is a blocker.
drivers/builtin/src/ecp.c
mpg (Migrated from github.com) commented 2025-09-15 02:56:54 -05:00

Question: is there a circumstance where mbedtls_ecp_trace_counts_live is going to be non-NULL but mbedtls_ecp_self_test_trace_counts_compare is going to be NULL?

I'm thinking mbedtls_ecp_self_test_trace_counts_compare() might not need to be a NULLable pointer, but could be a proper function just like reset(), so that we have only one source of truth for "are we collecting and checking trace counters", that is mbedtls_ecp_trace_counts_live != NULL.

Question: is there a circumstance where `mbedtls_ecp_trace_counts_live` is going to be non-NULL but `mbedtls_ecp_self_test_trace_counts_compare` is going to be NULL? I'm thinking `mbedtls_ecp_self_test_trace_counts_compare()` might not need to be a NULLable pointer, but could be a proper function just like `reset()`, so that we have only one source of truth for "are we collecting and checking trace counters", that is `mbedtls_ecp_trace_counts_live != NULL`.
tests/scripts/components-sanitizers.sh
@ -121,1 +131,4 @@
make
msg "test: full config, main suites (TSan)"
make test
mpg (Migrated from github.com) commented 2025-09-15 02:48:24 -05:00

I'm probably missing something obvious:

Restartable ECC is
not thread-safe, but this does not affect the thread safety of the ECC code
as long as you don't try restartable operations (including interruptible PSA
functions).

But surely make test below is going to try restartable operations when ECP_RESTARTABLE is enabled?

I never thought deeply about this, but it seems to me that restartable operations are just as thread-safe as normal ones as long as you don't share the restartable context between threads. So I guess the thing is make test below will run restartable/interruptible tests, but just nor share the context between threads?

I'm probably missing something obvious: > Restartable ECC is not thread-safe, but this does not affect the thread safety of the ECC code as long as you don't try restartable operations (including interruptible PSA functions). But surely `make test` below is going to try restartable operations when `ECP_RESTARTABLE` is enabled? I never thought deeply about this, but it seems to me that restartable operations are just as thread-safe as normal ones as long as you don't share the restartable context between threads. So I guess the thing is `make test` below will run restartable/interruptible tests, but just nor share the context between threads?
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 03:15:58 -05:00
tests/scripts/components-sanitizers.sh
@ -121,1 +131,4 @@
make
msg "test: full config, main suites (TSan)"
make test
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 03:15:58 -05:00

make test won't do concurrent restartable operations since we don't have a test that does that.

Likewise, make test doesn't run self-tests concurrently, so it doesn't matter that they're not thread-safe. The reason TSan was complaining was that some code gated by MBEDTLS_SELF_TEST that wasn't thread safe was getting run by all ECC operations.

`make test` won't do _concurrent_ restartable operations since we don't have a test that does that. Likewise, `make test` doesn't run self-tests concurrently, so it doesn't matter that they're not thread-safe. The reason TSan was complaining was that some code gated by `MBEDTLS_SELF_TEST` that wasn't thread safe was getting run by all ECC operations.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 03:27:30 -05:00
drivers/builtin/src/ecp.c
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 03:27:30 -05:00

It depends on what the test code does. Tests other than the self-tests set the live count pointer to non-null, but don't need to set the compare function. The self-tests set both.

The compare function needs to live in the test code because it calls TEST_EQUAL. Code in library can't call test assertions: this would result in linking failures when compiling sample programs that don't link against test objects.

We could have a different way of reporting assertion failures. We could have the compare function return an error code that makes the self-test fail (like it did before). I don't like that because test hooks are not supposed to change the behavior of the library, as much as possible. (It wouldn't violate the rules since you'd have to activate the compare hook by setting mbedtls_ecp_trace_counts_live, but I still think it's dodgy.) Setting a field in the mbedtls_ecp_trace_counts_live structure and checking that field in the test function would work, at the expense of requiring a little more code and having more indirect error reporting that's a little harder to investigate.

I don't think that's worth it, just to avoid having the possibility of a runtime test configuration that doesn't make sense (non-null compare hook with a null live counts pointer).

It depends on what the test code does. Tests other than the self-tests set the live count pointer to non-null, but don't need to set the compare function. The self-tests set both. The compare function needs to live in the test code because it calls `TEST_EQUAL`. Code in `library` can't call test assertions: this would result in linking failures when compiling sample programs that don't link against test objects. We could have a different way of reporting assertion failures. We could have the compare function return an error code that makes the self-test fail (like it did before). I don't like that because test hooks are not supposed to change the behavior of the library, as much as possible. (It wouldn't violate the rules since you'd have to activate the compare hook by setting `mbedtls_ecp_trace_counts_live`, but I still think it's dodgy.) Setting a field in the `mbedtls_ecp_trace_counts_live` structure and checking that field in the test function would work, at the expense of requiring a little more code and having more indirect error reporting that's a little harder to investigate. I don't think that's worth it, just to avoid having the possibility of a runtime test configuration that doesn't make sense (non-null compare hook with a null live counts pointer).
mpg (Migrated from github.com) reviewed 2025-09-15 03:44:25 -05:00
drivers/builtin/src/ecp.c
mpg (Migrated from github.com) commented 2025-09-15 03:44:25 -05:00

Tests other than the self-tests set the live count pointer to non-null, but don't need to set the compare function.

I can't see the point in collecting data if you aren't going to do anything with it.

The compare function needs to live in the test code because it calls TEST_EQUAL. [...]

Ah yes, good point. I agree the alternatives would be unpleasant, and that's a good enough reason to use a pointer here.

> Tests other than the self-tests set the live count pointer to non-null, but don't need to set the compare function. I can't see the point in collecting data if you aren't going to do anything with it. > The compare function needs to live in the test code because it calls `TEST_EQUAL`. [...] Ah yes, good point. I agree the alternatives would be unpleasant, and that's a good enough reason to use a pointer here.
gilles-peskine-arm (Migrated from github.com) reviewed 2025-09-15 03:57:08 -05:00
drivers/builtin/src/ecp.c
gilles-peskine-arm (Migrated from github.com) commented 2025-09-15 03:57:08 -05:00

I can't see the point in collecting data if you aren't going to do anything with it.

Sure, but the normal scenario is that the test code measures one operation, then either stores the result or compares it with a previous stored result. That's what happens in ecp_test_vect. In the self-test code, there's a loop that runs multiple iteration, so deciding what to measure and compare has to be done each time you go through the loop body. Hence we need to inject additional code into mbedtls_ecp_self_test. Most of that code can just be guarded by mbedtls_ecp_trace_counts_live, but not reporting the failed comparisons because that goes through the unit test infrastructure.

> I can't see the point in collecting data if you aren't going to do anything with it. Sure, but the normal scenario is that the test code measures one operation, then either stores the result or compares it with a previous stored result. That's what happens in `ecp_test_vect`. In the self-test code, there's a loop that runs multiple iteration, so deciding what to measure and compare has to be done each time you go through the loop body. Hence we need to inject additional code into `mbedtls_ecp_self_test`. Most of that code can just be guarded by `mbedtls_ecp_trace_counts_live`, but not reporting the failed comparisons because that goes through the unit test infrastructure.
bjwtaylor (Migrated from github.com) approved these changes 2026-09-17 09:47:30 -05:00
mpg commented 2026-09-18 02:02:53 -05:00 (Migrated from github.com)

This is now fully approved, but has a conflict, and probably needs a 1.1 backport too.

This is now fully approved, but has a conflict, and probably needs a 1.1 backport too.
This pull request has changes conflicting with the target branch.
  • drivers/builtin/src/ecp_invasive.h
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'gilles-peskine-arm/ecp-self-test-race-crypto:gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'gilles-peskine-arm/ecp-self-test-race-crypto'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'
git merge --no-ff 'gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'gilles-peskine-arm/ecp-self-test-race-crypto'
git rebase 'development'
git switch 'development'
git merge --ff-only 'gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'gilles-peskine-arm/ecp-self-test-race-crypto'
git rebase 'development'
git switch 'development'
git merge --no-ff 'gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'development'
git merge --squash 'gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'development'
git merge --ff-only 'gilles-peskine-arm/ecp-self-test-race-crypto'
git switch 'development'
git merge 'gilles-peskine-arm/ecp-self-test-race-crypto'
git push origin 'development'
Sign in to join this conversation.
Reviewers
No reviewers
mpg
bjwtaylor
Labels
Clear labels   
api-break
This issue/PR breaks the API and must wait for a new major version

  
approved
Design and code approved - may be waiting for CI or backports

  
bug
Something isn't working

  
component-docs
Docs / website issues filed here for tracking

  
component-platform
Portability layer and build scripts

  
component-test
Test framework and CI scripts

  
dependencies
Pull requests that update a dependency file

  
deprecation
needs deprecation announcement

  
DO-NOT-MERGE

   
enhancement
New feature or request

  
good first issue
Good for newcomers

  
help wanted
This issue is not being actively worked on, but PRs welcome.

  
needs-adoption
stalled PR that someone should pick up and complete

  
needs-backports
Backports are missing or are pending review and approval.

  
needs-ci
Needs to pass CI tests

  
needs-design-approval
Needs design discussion / approval

  
needs-info
An issue or PR which needs further info from the reporter / author

  
needs-preceding-pr
Requires another PR to be merged first

  
needs-review
Every commit must be reviewed by at least two team members

  
needs-reviewer
This PR needs someone to pick it up for review

  
needs-work

   
priority-high
High priority - will be reviewed soon

  
priority-low
Low priority - this may not receive review soon

  
priority-medium
Medium priority - this can be reviewed as time permits

  
priority-scheduled
This PR is big - it will require time to be scheduled for review

  
priority-very-high
Highest priority - prioritise this over other review work

  
resolved-after-1.1
The issue is resolved in development, but still present in 1.1 LTS.

  
single-reviewer
This PR qualifies for having only one reviewer

  
size-l
Estimated task size: large (2w+)

  
size-m
Estimated task size: medium (~1w)

  
size-optimisation

   
size-s
Estimated task size: small (~2d)

  
size-xs
Estimated task size: extra small (a few hours at most)

No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
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-Mbed-TLS/TF-PSA-Crypto!450
No description provided.