-
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
[Backport 3.6] psa_util.c included in builds without PSA, which can break the build #9463
[Backport 3.6] psa_util.c included in builds without PSA, which can break the build #9463
Conversation
… when PSA enabled Signed-off-by: Sam Berry <sam.berry@arm.com>
Signed-off-by: Sam Berry <sam.berry@arm.com>
This is missing the changes to the test suite. |
@gilles-peskine-arm I think |
Ah. So it does. The difference between 3.6 and development still doesn't quite make sense to me, but I now think that something's wrong with the development PR because things changed on the development branch after you started working. Let me look closer. |
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.
Looking at this independently on the mbedtls-3.6
branch:
We want to resolve a bug in 3.6.0 whereby the ECDSA conversion functions don't work when PSA is disabled (due to a buffer size that only works in builds with PSA). Since the functions are not very useful without PSA and didn't work without PSA, we want to disable them in builds without PSA. This pull request does the requisite change.
Incidentally, it has come up that the tests of these functions were only running in builds with PSA. The preprocessor guard on the test functions is the same as the guard on the library functions: MBEDTLS_PSA_UTIL_HAVE_ECDSA
, so there's nothing to fix there. The lack of test coverage came from guards on the test cases, which depend on PSA support for curve sizes. Since the library functions are now enabled only in builds with PSA, the test cases do run in all the builds that have the library functions. So we don't need to change anything else in the tests.
Therefore this pull request correctly fixes the bug.
There is no non-regression test, but that would come through stricter compiler flags and we'll handle that in a separate pull request (backport of #9456, addressing #9317).
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. As pointed out previously, the patch differs from the development patch, but in ways that have been explained in the meantime, so it's all good.
70658db
Backport of #9313
PR checklist