Skip to content

Conversation

@gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Feb 5, 2025

Fix PSA multipart or interruptible operations with compilers where union … foo = {0} only initializes the first member of the unions, such as the upcoming GCC 15. This affected:

  • MAC and interruptible sign-verify operations with our built-in implementation.
  • KDF operations, and one-shot MAC, due to their use of internal functions on multipart MAC operations.
  • Potentially other operations when using third-party drivers.

For testing:

  • Test with GCC 15, which validates that the fix is complete in builds without third-party drivers.
  • Add checks in the driver wrappers that we already have, which validates the fix for builds using drivers, if testing with an incomplete initializer.
  • Change many tests to use a short initializer, which validates the fix even on compilers where {0} initializes a union to all-bits-zero.

Status: I will rewrite the history a little, and rebase on top of #10151. This is not ready for review yet.

PR checklist

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
When initializing a multipart or interruptible operation structure, use an
auxiliary function that doesn't initialize union members to all-bits-zero.
Context: on most compilers, initializing a union to `{0}` initializes it to
all bits zero; but on some compilers, the trailing part of members other
than the first is left uninitialized. This way, we can run the tests on any
platform and validate that the code would work correctly on platforms where
union initialization is short.

This commit makes a systematic replacement in `test_suite_psa_crypto.function`
and `test_suite_psa_crypto_driver_wrappers.function`, which gives good
enough coverage.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Partially undo "Use short initializers for multipart operation structures",
only in test functions that specifically aim to test initializers. In these
functions, do try with the short initializers, but alongside the standard
ones.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
They can cause specific challenges when debugging, so move them out for
maintainers' convenience.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
They can cause specific challenges when debugging, so move them out for
maintainers' convenience.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
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>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Non-regression for Mbed-TLS#9814

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is a new warning in GCC 15 that our code base triggers in many places.
Silence it for the time being.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
The goal of testing with GCC 15 is to validate fixes for
Mbed-TLS#9814 . The bug is present in
multiple places, and some of them affect third-party drivers but not our
built-in implementation. (The bug is that driver contexts might not be
zero-initialized, but some of our built-in implementations happen not to
care about this.) Thus, enable the test drivers in the test component that
uses GCC 15, to gain the extra checks performed in the driver wrappers.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm
Copy link
Contributor Author

I've split off some preliminaries in #10151. Not much, but that will ease the review here a little, and also make the submodule transition a bit smoother.

@gilles-peskine-arm
Copy link
Contributor Author

Superseded by #10151, #10168 and #10179 with the same content but a different commit history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Development

Successfully merging this pull request may close these issues.

1 participant