-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add testing for concurrently loading/using/destroying the same key #8924
Add testing for concurrently loading/using/destroying the same key #8924
Conversation
This will allow us to use this smoke test to ensure that key slot content reads are only performed when we are registered to read a full slot. We will destroy the key on another thread while the key is being exercised, and fail the test if an unexpected error code is hit. Future commits will incrementally implement this new parameter. All current usages of this function have this parameter set to 0, in which case the new behaviour must be the same as the old behaviour Signed-off-by: Ryan Everett <ryan.everett@arm.com>
This function is currently only used in the exercise_key smoke test. Signed-off-by: Ryan Everett <ryan.everett@arm.com>
If the key has been destroyed (and the new parameter is 1) then we test that psa_mac_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>
If the key has been destroyed (and the new parameter is 1), we test that psa_cipher_abort succeeds in this scenario. Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
All current usages have this parameter set to 0 (in this case the behaviour of the test is unchanged) Signed-off-by: Ryan Everett <ryan.everett@arm.com>
All current usages have this parameter set to 0 (meaning the behaviour of these tests hasn't changed). We also now return the actual error code, not GENERIC_ERROR Signed-off-by: Ryan Everett <ryan.everett@arm.com>
All current usages have this parameter set to 0 (this means the tests are unchanged). Remove the GENERIC_ERROR return behaviour, in favour of returning the actual status. Signed-off-by: Ryan Everett <ryan.everett@arm.com>
These are only called from mbedtls_test_psa_exercise_key Signed-off-by: Ryan Everett <ryan.everett@arm.com>
The thread functions can also be used in future tests for other key types and other test scenarios Signed-off-by: Ryan Everett <ryan.everett@arm.com>
There is a 1-1 correlation between these test cases and the test cases for import_and_exercise_key. Signed-off-by: Ryan Everett <ryan.everett@arm.com>
d4d5808
to
f111f35
Compare
In its current form this PR does not handle the potential INSUFFICIENT_MEMORY failure which can occur in exercise key when lots of threads attempt to load into a key slot. It shouldn't be merged before this is changed. |
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.
I did a first pass to get more familiar with the PR and it looks almost OK to me. I only found very few comments. I will do a second pass tomorrow.
/* The key has been destroyed. */ | ||
status = PSA_SUCCESS; | ||
goto exit; | ||
} |
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.
I think that PSA_ASSERT(status)
is missing after this line in order to keep the same behavior as before in case the key has not been destroyed.
same_key_context *skc = (struct same_key_context *) ctx; | ||
psa_key_attributes_t got_attributes = PSA_KEY_ATTRIBUTES_INIT; | ||
|
||
/* Import the key, exactly one thread must succceed. */ |
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.
Typo: succeed
if (skc->key_loaded) { | ||
mbedtls_mutex_unlock(&skc->key_loaded_mutex); | ||
/* More than one thread has succeeded, report a failure. */ | ||
TEST_EQUAL(skc->key_loaded, 0); |
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.
Why not directly TEST_FAIL()
in this case? I think it would be more explicit, but I might be missing something.
If you accept this change, then there is a very similar case also in thread_use_and_destroy_key()
below
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.
I wasn't aware of TEST_FAIL()
, good suggestion!
Upon revisiting this PR I realize that I was mistaken here. My concern was of two (what I thought where) possible scenarios:
To summarize: no extra resources are ever required when calling |
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
Signed-off-by: Ryan Everett <ryan.everett@arm.com>
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
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.
Thanks for addressing my comments. LGTM :)
Description
There are two parts to this PR. The first part is repetitive work which changes the key smoke tests to support the possibility that the key may be destroyed at any time. The second part is adding tests for various key behaviours.
We first add a new parameter
key_destroyable
to theexercise_X
functions. Whenkey_destroyable == 0
the behaviour is the same as before these changes, whenkey_destroyable == 1
the smoke tests will ignore any valid failures caused by the key suddenly not existing; unexpected failures like corruption will be reported.The reason we are doing this is that it provides us a framework which we can use to test that we abide by our key access rules set in
psa-thread-safety.md
:By running
mbedtls_test_psa_exercise_key
on the key in one thread, and concurrently destroying the key in another, TSan/internal errors will spot if these properties are being violated.This PR adds testing for the following:
Disclaimer: this doesn't test every API call or every code path. Exercise key can do a lot, but it doesn't cover all types of operation or all API calls. We also only do not cover all methods of loading a key, or all types of keys. Future work should add more functionality to exercise key, add more test data to this function, and add more test functions which use the other key management functions.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")