-
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
psa_util.c included in builds without PSA, which can break the build #9313
psa_util.c included in builds without PSA, which can break the build #9313
Conversation
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.
This is correct but incomplete.
Although it's correct, I'm not sure this is the right approach, between restricting the function definition and fixing the array sizes. Let's discuss this further with Manuel in the issue.
library/psa_util.c
Outdated
@@ -365,6 +365,14 @@ int mbedtls_psa_get_random(void *p_rng, | |||
|
|||
#endif /* MBEDTLS_PSA_CRYPTO_CLIENT */ | |||
|
|||
#if defined(MBEDTLS_PSA_CRYPTO_CLIENT) |
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.
It doesn't really matter since these are just macros, but the new macros are only used in code guarded by #if defined(MBEDTLS_PSA_UTIL_HAVE_ECDSA)
, so the macro definitions should be after the #if defined(MBEDTLS_PSA_UTIL_HAVE_ECDSA)
line.
library/psa_util.c
Outdated
#define MAX_COORDINATE_BYTES PSA_BITS_TO_BYTES(PSA_VENDOR_ECC_MAX_CURVE_BITS) | ||
#define ECDSA_SIGNATURE_MAX_SIZE PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE |
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.
When MBEDTLS_PSA_CRYPTO_CLIENT
is defined, ECDSA can be implemented either through mbedtls_ecdsa_xxx
or through psa_xxx
. So naively, one might expect that ECDSA_SIGNATURE_MAX_SIZE
should be the max of PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE
and MBEDTLS_ECDSA_MAX_LEN
, and similarly for MAX_COORDINATE_BYTES
.
It is in fact guaranteed that PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE >= MBEDTLS_ECDSA_MAX_LEN
and so on, because it is guaranteed that all curves supported through the legacy API are also supported through PSA. We should document that we rely on this guarantee here.
We should have added a comment when we wrote the original code here, to justify why PSA_VENDOR_ECDSA_SIGNATURE_MAX_SIZE
was enough. But we didn't. So please write this comment now.
library/psa_util.c
Outdated
@@ -365,6 +365,14 @@ int mbedtls_psa_get_random(void *p_rng, | |||
|
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.
Since this is a user-impacting bug, we need a changelog entry.
A changelog entry goes into a file in the ChangeLog.d
directory. See https://github.com/Mbed-TLS/mbedtls/blob/development/ChangeLog.d/00README.md for explanations and existing entries in ChangeLog.d/*.txt
and ChangeLog
for examples.
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.
Note that the bug isn't only “Prevents 0-size array compiler warning”: in configurations with this warning, calling this function would never work since it would try to store data in the 0-size array.
library/psa_util.c
Outdated
@@ -365,6 +365,14 @@ int mbedtls_psa_get_random(void *p_rng, | |||
|
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.
We need either a non-regression test (i.e. a test that fails before applying the fix and passes after), or a good reason not to have one. Sometimes, “it's too much effort to write a test” can be a good reason.
Here I see two ways in which a test could catch the bug.
One way is at compile time: either we are never compiling in an affected configuration, or we are compiling in some affected configurations but only with permissive compiler flags (GCC and Clang allow 0-size array by defaults). According to the outcome file from #9301 (I'm looking at the 3.6 outcomes, because in the development
we're moving to PSA-only configurations where this bug can't happen), and with the help of search_outcomes.py
from Mbed-TLS/mbedtls-framework#20, I can see:
$ framework/scripts/search_outcomes_config.py -f outcomes-3.6.csv MBEDTLS_ECDSA_C \!MBEDTLS_PSA_CRYPTO_C
component_test_default_psa_crypto_client_without_crypto_provider
component_test_full_no_cipher_no_psa_crypto
component_test_no_psa_crypto_full_cmake_asan
config-no-entropy.h
config-suite-b.h
The first one has MBEDTLS_PSA_CRYPTO_C
disabled but MBEDTLS_PSA_CRYPTO_CLIENT
enabled, so it's actually not relevant here. The other 4 are relevant. The two configuration names ending with .h
are handled by tests/scripts/test-ref-configs.pl
, and they're just using the config files in the configs
subdirectory. All of these builds use Clang (or maybe GCC) without -std=c99 -pedantic
, so they don't complain about 0-length arrays. Ideally we should pass -std=c99 -pedantic
, but I haven't checked if it's feasible without too much effort: it might cause a bunch of other errors that we don't want to deal with.
The second way to catch the bug would be through tests. After all, if we run a test that tries to use a 0-size array, the test would surely overflow that array. We do most test builds with Asan, which detects buffer overflows. Why wasn't there a test failure? We do have tests for these functions. They're in test_suite_psa_crypto_util
. The test function is only guarded by MBEDTLS_PSA_UTIL_HAVE_ECDSA
, but the test data is guarded by a minimum value for PSA_VENDOR_ECC_MAX_CURVE_BITS
for all test cases. If the functions are available in configurations where PSA is disabled, we need to change the guards on these tests, since PSA_VENDOR_ECC_MAX_CURVE_BITS
is 0 when PSA is disabled.
So we now understand that we didn't catch this bug due to a test gap. And we need to fix the test gap here.
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.
Note that in builds with PSA disabled, PSA_WANT_xxx
symbols might still be set, so PSA_VENDOR_ECC_MAX_CURVE_BITS
might still end up being nonzero. I haven't figured out exactly when it ends up being 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.
Ideally we should pass
-std=c99 -pedantic
, but I haven't checked if it's feasible without too much effort: it might cause a bunch of other errors that we don't want to deal with.
In the past we there were indeed some portions of our code that threw a lot of errors with those flags, but looking at all.sh
it looks like nowadays we can use them with config.py
full and build everything. So, I'm tempted to suggest we use -std=c99 -pedantic
as our new default in all.sh
(like, add it to $ASAN_CFLAG
and cmake's ASan flags), and only disable it locally if there are a few components that don't work with it. Wdyt?
(@sezrab That would of course be out of scope for this PR and be a separate piece of work.)
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'm tempted to suggest we use -std=c99 -pedantic as our new default in all.sh (like, add it to $ASAN_CFLAG and cmake's ASan flags), and only disable it locally if there are a few components that don't work with it
That's absolutely a good idea. The thing is, I have no idea how far we are from it, so I can't estimate how long it'll take, which means it's currently not feasible.
Unfortunately this grew a conflict. @sezrab Can you resolve the conflict, then ping Gilles and me, and we'll re-review shortly? |
72da87e
to
8b52269
Compare
Aw, very unlucky, got a new conflict right after your rebased. Can you rebase again? Thanks! |
8b52269
to
528526d
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.
I think this fixes the issue, but also does extra work that's no longer needed as the chosen fix was to remove those functions from non-PSA builds.
@sezrab Now that this is approved, can you create the 3.6 backport? Thanks! |
I am dismissing my approval because I think something's wrong due to a change in development that happened after the work here started.
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.
This pull request has a semantic conflict with changes in the development
branch: it changes the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA
, but MBEDTLS_PSA_UTIL_HAVE_ECDSA
is no longer used in development
.
In develompent
now, we're removing support for builds without PSA. The library functions are now guarded by PSA_HAVE_ALG_SOME_ECDSA
, no longer by MBEDTLS_PSA_UTIL_HAVE_ECDSA
. The test functions are also guarded by PSA_HAVE_ALG_SOME_ECDSA
. So the bug is resolved in development
, although we hadn't noticed that there was a bug so we should still announce it in a changelog entry. (This changelog entry will need to change because it's referring to non-PSA builds, which will no longer be a thing. But I expect that we'll handle these changes to changelog entries in bulk at the end of the de-non-PSA-ification work.)
In addition to the changelog entry, we should remove the unused definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA
. Having this unused definition created confusion. For example, if the definition had been removed, there would have been a git conflict with this pull request, which would have alerted us that there was a conflict.
@@ -428,7 +428,7 @@ | |||
|
|||
/* psa_util file features some ECDSA conversion functions, to convert between | |||
* legacy's ASN.1 DER format and PSA's raw one. */ | |||
#if defined(MBEDTLS_ECDSA_C) || (defined(MBEDTLS_PSA_CRYPTO_C) && \ | |||
#if (defined(MBEDTLS_PSA_CRYPTO_CLIENT) && \ | |||
(defined(PSA_WANT_ALG_ECDSA) || defined(PSA_WANT_ALG_DETERMINISTIC_ECDSA))) | |||
#define MBEDTLS_PSA_UTIL_HAVE_ECDSA |
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.
In development
today, MBEDTLS_PSA_UTIL_HAVE_ECDSA
is still defined but never used. So there's a semantic conflict between what this pull request is doing and what has changed in development
.
As a general principle I would agree. But in this case, since the bug was only when PSA is disabled, and in 4.0 PSA will always be on, does it really make sense to have a ChangeLog entry for this in 4.0? The following would seem weird to me:
Agreed, though I think that's a job for 9384's author. So overall, I'd be inclined to just close this development PR, and keep the "backport" as a 3.6-only PR, changing the description to add "PR development: not required because: bug defined out of existence in 4.0". Wdyt? |
I expect that many changelog entries for bug fixes will not look right for the 4.0 release because they've been overridden. Sometimes they'll be overridden in ways that we don't anticipate yet because we don't know all the details of 4.0. We'll need to go over changelog entries and reword the ones that need rewording, including but not limited to removing the ones that are completely obsolete. That's a lot easier than going through the whole lot of commits and figuring out everything we need to announce. So no, please don't skip changelog entries for bug fixes just because you think they'll go away later.
I don't care who does it. We're a team. If there's a reason for it to stay for now, then of course it should stay. But then we should add a comment to explain why it's staying! |
Ok then. In that specific instance I think we don't just think it'll go away, we know it, because PSA always on is one of the defining features of 4.0. OTOH, you're right that in other cases things will be less clear, and it's better to have a uniform process with no exception, so let's add the ChangeLog now even though it's extremely likely to be removed later. (Btw, do we have an issue to track that step when preparing the 4.0 release?)
Sure. I meant for another PR. My line of thinking was whether we still needed this PR at all. Since we're doing the ChangeLog entry anyway, then the question is moot. |
Not yet. So far I've avoided creating 4.0 issues that aren't ready for work yet, to avoid noise during planning. I've only created a few because they were direct follow-ups to tasks that are started or ready to start. |
@@ -3,7 +3,7 @@ | |||
#include <mbedtls/psa_util.h> | |||
/* END_HEADER */ | |||
|
|||
/* BEGIN_CASE depends_on:PSA_HAVE_ALG_SOME_ECDSA */ | |||
/* BEGIN_CASE depends_on:MBEDTLS_PSA_UTIL_HAVE_ECDSA */ |
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.
#9384 has been replacing and we shouldn't be undoing that here. Please remove the changes to the test files (which were correct at the time you made them, btw, but development is moving fast these days - now the functions are guarded by PSA_HAVE_ALG_SOME_ECDSA
so that magically became the right macros for the tests too).
Just to summarize, I think this PR should be ChangeLog only:
(For the avoidance of doubt: that's just for the development PR. The 3.6 PR is fine as it is and keeps the fix to the definition in addition to the ChangeLog.) |
Signed-off-by: Sam Berry <sam.berry@arm.com>
1f65599
to
26769f1
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.
Approved with just the changelog because we need the 3.6 PR in the upcoming release, but I would strongly prefer to either remove the definition of MBEDTLS_PSA_UTIL_HAVE_ECDSA
or edit its comment to state that it shouldn't be used anymore and explain why it's still there.
8067879
Introduced max size/bytes macros in psa_util.c. Prevents 0-size array compiler warning in psa-util.c when MBEDTLS_ECDSA_C is enabled but MBEDTLS_PSA_CRYPTO_C is disabled. Fixes #9311
PR checklist