Backport 3.6: Fix insufficient union initialization in contexts #10168
Conversation
In API functions that set up a multipart or interruptible operation, make
sure to initialize the driver-specific part of the context. This is a union,
and initializing the union to `{0}` only guarantees that the first member of
the union is initialized, not necessarily the member used by the driver.
Most compilers do initialize the whole union to all-bits-zero, but some
don't. With compilers that don't, the lack of initialization caused failures
of built-in MAC, interruptible-sign and interruptible-verify. It could also
cause failures for other operations with third-party drivers: we promise
that drivers' setup entry points receive a zero-initialized operation
structure, but this promise was not kept.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In functions that bypass the API functions and call the MAC driver wrapper
`psa_driver_wrapper_mac_sign_setup()` directly, make
sure to initialize the driver-specific part of the context. This is a union,
and initializing the union to `{0}` only guarantees that the first member of
the union is initialized, not necessarily the member used by the driver.
Most compilers do initialize the whole union to all-bits-zero, but some
don't. With compilers that don't, the lack of initialization caused failures
of the affected operations. This affected several key derivation operations.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In functions that bypass the API functions and call an internal MAC setup
function directly, make sure to initialize the driver-specific part of the
context. This is a union, and initializing the union to `{0}` only
guarantees that the first member of the union is initialized, not
necessarily the member used by the driver. Most compilers do initialize the
whole union to all-bits-zero, but some don't. With compilers that don't, the
lack of initialization caused failures of the affected operations. This
affected one-shot MAC operations using the built-in implementation.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is a non-regression test for Mbed-TLS#9814 Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
bug
needs-backports
github-project-automation
bot
moved this to In Development
in Roadmap pull requests (new board)
gilles-peskine-arm
added
component-crypto
|
When I build this branch using fc42 as follows:
I get the following build error: /home/mbedtls.gilles-peskine-arm.union-initialization-gcc15-basic-fix-3.6/framework/tests/src/psa_exercise_key.c:187:36: error: initializer-string for array of ‘unsigned char’ truncates NUL terminator but destination lacks ‘nonstring’ attribute (33 chars into 32 available) [-Werror=unterminated-string-initialization] Not sure this is related to the original issue, though also seems an issue with gcc 15. So maybe we fix it here or in another PR? Or is it some issue with my test method? gcc version FYI: [root@1f02bf6b65be home]# gcc --version Let me know what you think? |
|
@bjwtaylor That's #9944 which is out of scope here. We currently disable this warning in |
gabor-mezei-arm
requested review from
gabor-mezei-arm and
ronald-cron-arm
and removed request for
ronald-cron-arm
gabor-mezei-arm
left a comment
gabor-mezei-arm
left a comment
There was a problem hiding this comment.
Only minor things found. Otherwise looks good to me.
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
|
@gabor-mezei-arm What do you think about test coverage? Do you think that Mbed-TLS/mbedtls-framework#135 is enough, or do we need more? (Just in terms of what it claims to do, it's not yet ready for a code review.) |
mpg
left a comment
mpg
left a comment
There was a problem hiding this comment.
LGTM!
More precisely, I think the changes that are here are correct. Regarding the question of whether they're complete, I'm relying on the tests (present and upcoming).
github-project-automation
bot
moved this from In Development
to Has Approval
in Roadmap pull requests (new board)
mpg
added
the
approved
|
I've made pull requests for the test follow-ups. For 3.6:
The crypto and mbedtls pull requests will need a few rebase cycles after their precedessors are merged, but I don't intend to rewrite the existing history otherwise. I'll make new commits to handle review comments and CI failures. |
I think these tests give enough coverage. |
mpg
removed
needs-review
github-project-automation
bot
moved this from Has Approval
to Done
in Roadmap pull requests (new board)
4 participants
Fix issues with some PSA operation contexts not being properly initialized to all-bits-zero:
union foo x = {0}does not initialize bytes that are not used by the first member ofunion foo— resolvesHMAC-SHA-256test failures on upcominggcc-15(after partial union initialization changes) #9814;In this pull request, the practical definition of done is that the unit tests now pass with GCC 15. I don't think this is sufficient non-regression testing for #9814 and #9975. I've added a changelog entry, but I'm open to removing it until we have better testing.
I have written further tests which are feature-complete but may need a little rework to pass the CI, consisting of:
Beyond that, what we may want to test better — done in Mbed-TLS/mbedtls-framework#168 and #10179:
PR checklist