Fix built-in MAC operation initialization #897

2026-09-06T07:58:18-05:00

romart commented

2026-09-06 07:58:18 -05:00

(Migrated from github.com)

Description

Zero-initialize the full built-in MAC context. Add a regression test that fails before the fix and passes afterward across six GCC optimization levels, including -O0. The

PR checklist

changelog not required because: internal initializer fix; no public API behavior change demonstrated.
framework PR not required
TF-PSA-Crypto development PR provided
TF-PSA-Crypto 1.1 PR applicability pending review
mbedtls development PR applicability pending review
mbedtls 4.1 PR applicability pending review
mbedtls 3.6 PR applicability pending review
tests provided: regression fails before the fix and passes afterward with GCC at -O0, -O1, -O2, -O3, -Os, and -Og. The PSA crypto suite passes at -O0.

## Description Zero-initialize the full built-in MAC context. Add a regression test that fails before the fix and passes afterward across six GCC optimization levels, including -O0. The ## PR checklist - [x] **changelog** not required because: internal initializer fix; no public API behavior change demonstrated. - [x] **framework PR** not required - [x] **TF-PSA-Crypto development PR** provided - [ ] **TF-PSA-Crypto 1.1 PR** applicability pending review - [ ] **mbedtls development PR** applicability pending review - [ ] **mbedtls 4.1 PR** applicability pending review - [ ] **mbedtls 3.6 PR** applicability pending review - **tests** provided: regression fails before the fix and passes afterward with GCC at -O0, -O1, -O2, -O3, -Os, and -Og. The PSA crypto suite passes at -O0.

gilles-peskine-arm (Migrated from github.com) requested changes

2026-09-16 13:56:51 -05:00

gilles-peskine-arm (Migrated from github.com) left a comment

This breaks older clang.

This is just a minor robustness improvement, since there could be compilers where it doesn't help. (I'm not aware of one. Maybe Compcert, which has been only initializing the first member of unions for a long time?) So I'm not convinced it's worth making the code more complex over this.

This breaks older clang. This is just a minor robustness improvement, since there could be compilers where it doesn't help. (I'm not aware of one. Maybe Compcert, which has been only initializing the first member of unions for a long time?) So I'm not convinced it's worth making the code more complex over this.

drivers/builtin/include/mbedtls/private/crypto_builtin_composites.h

					
				@ -73,0 +73,4 @@

				 * (C17 6.7.9 paragraphs 10 and 21). An explicit { 0 } initializes only

				 * dummy (6.7.9 paragraph 17), leaving bytes beyond it unspecified

				 * (6.2.6.1 paragraph 7; see also DR 283). */

				#define MBEDTLS_PSA_MAC_OPERATION_INIT { 0 }

Unfortunately, leaving the union explicitly initialized doesn't help according to the C standard. ¶10 states:

if it is a union, the first named member is initialized (recursively) according to these
rules, and any padding is initialized to zero bits;

The content of the union that does not overlap with the first member remains uninitialized. (This content is not padding. Padding is memory content that is not part of the value of anything, not memory content that is part of an inactive union member.)

In practice, { 0 } vs { 0, { 0 } } does seem to make a difference with GCC 15 (the most popular compiler that actually only initializes the first (or designated) member of a union.) So, even though it isn't guaranteed, this does make the code more robust in case we forget to explicitly zero out the whole union somewhere. However, we can't just break the build with older Clang.

Unfortunately, leaving the union explicitly initialized doesn't help according to the C standard. ¶10 states: > if it is a union, the first named member is initialized (recursively) according to these rules, and any padding is initialized to zero bits; The content of the union that does not overlap with the first member remains uninitialized. (This content is not padding. Padding is memory content that is not part of the value of anything, not memory content that is part of an inactive union member.) In practice, `{ 0 }` vs `{ 0, { 0 } }` does seem to make a difference with GCC 15 (the most popular compiler that actually only initializes the first (or designated) member of a union.) So, even though it isn't guaranteed, this does make the code more robust in case we forget to explicitly zero out the whole union somewhere. However, we can't just break the build with older Clang.

drivers/builtin/include/mbedtls/private/crypto_builtin_composites.h

					
				@ -73,1 +74,4 @@

				 * dummy (6.7.9 paragraph 17), leaving bytes beyond it unspecified

				 * (6.2.6.1 paragraph 7; see also DR 283). */

				#define MBEDTLS_PSA_MAC_OPERATION_INIT { 0 }

We use full initializers because older versions of Clang don't like partial initializers with -Wextra (specifically -Wmissing-field-initializers). (See all the failures on the CI.)

We use full initializers because older versions of Clang don't like partial initializers with `-Wextra` (specifically `-Wmissing-field-initializers`). (See all the failures on the CI.)

This pull request is broken due to missing fork information.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin 'romart/development:romart/development'

git switch 'romart/development'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'development'

git merge --no-ff 'romart/development'

git switch 'romart/development'

git rebase 'development'

git switch 'development'

git merge --ff-only 'romart/development'

git switch 'romart/development'

git rebase 'development'

git switch 'development'

git merge --no-ff 'romart/development'

git switch 'development'

git merge --squash 'romart/development'

git switch 'development'

git merge --ff-only 'romart/development'

git switch 'development'

git merge 'romart/development'

git push origin 'development'

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!897

Rows
Columns