Skip to content

Move the inclusion of crypto_sizes.h and crypto_struct.h in crypto.h #260

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

Conversation

ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm commented Apr 17, 2025

Description

Move the inclusion of crypto_sizes.h and crypto_struct.h in crypto.h
That way when API are declared, the types they used are defined.
This should resolve the issues related to psa_xyz_init functions returning a structure described in Mbed-TLS/mbedtls#7087.

PR checklist

@ronald-cron-arm ronald-cron-arm added the needs-ci Needs to pass CI tests label Apr 17, 2025
@@ -39,6 +39,18 @@ extern "C" {
* algorithms, key types, policies, etc. */
#include "crypto_types.h"

/* The file "crypto_sizes.h" contains definitions for size calculation
* macros whose definitions are implementation-specific. */
#include "crypto_sizes.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crypto_sizes.h uses macros from crypto_values.h, so it would make more sense to include crypto_values.h first. I don't think it actually matters for the sake of the C language, because crypto_sizes.h itself only defines macros, it doesn't expand them. But then crypto_struct.h does expand macros from crypto_sizes.h, and that can result in expanding macros from crypto_values.h. It works because crypto_struct.h (indirectly) includes crypto_driver_common.h which includes crypto_values.h. But it would make more sense to include crypto_values.h at the logical place in crypto.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I missed that, thanks. I've changed it and while at it I've tried to fix the similar issues in crypto_extra.h pointed to by #7087.

@ronald-cron-arm ronald-cron-arm force-pushed the move-crypto-struct-inclusion branch from 037a7ac to be9d9f0 Compare April 17, 2025 14:51
@gilles-peskine-arm gilles-peskine-arm added priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) needs-work labels Apr 17, 2025
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the code change. I don't know if it actually fixes Mbed-TLS/mbedtls#7087, but at worst it's harmless.

If this does fix Mbed-TLS/mbedtls#7087 then we need a changelog entry, and preferably a non-regression test (which would resolve Mbed-TLS/mbedtls#1767).

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members needs-backports Backports are missing or are pending review and approval. and removed needs-ci Needs to pass CI tests labels May 7, 2025
@gilles-peskine-arm
Copy link
Contributor

We have a report that this does fix 7087. Can you please add a changelog entry and make a backport to 3.6?

@aslze
Copy link

aslze commented May 11, 2025

We have a report that this does fix 7087. Can you please add a changelog entry and make a backport to 3.6?

Can confirm. I had the same issue and just replacing "crypto_extra.h" and "crypto.h" in version 3.6.3 fixed it.

@irwir
Copy link
Contributor

irwir commented May 22, 2025

@ronald-cron-arm
Since you already rearranging declarations, would it be possible to improve consistency as requested in the issue #7841?

That way when API are declared, the types they use are defined.

This should resolve the issues related to psa_xyz_init functions
returning a structure described in #7087.

Signed-off-by: Ronald Cron <[email protected]>
Signed-off-by: Ronald Cron <[email protected]>
@irwir
Copy link
Contributor

irwir commented May 27, 2025

Rolling back changes in crypto_extra.h gives errors at the lines:

static psa_pake_cipher_suite_t psa_pake_cipher_suite_init(void);
static psa_pake_operation_t psa_pake_operation_init(void);

No other lines in the file are using these forward declarations; hence both lines could be safely deleted.
The corresponding definitions at the bottom of the file could be changed as suggested in my previous message:

/** Return an initial value for a PAKE cipher suite object.
 */
static inline psa_pake_cipher_suite_t psa_pake_cipher_suite_init(void)
{
    const psa_pake_cipher_suite_t v = PSA_PAKE_CIPHER_SUITE_INIT;
    return v;
}

/** Return an initial value for a PAKE operation object.
 */
static inline psa_pake_operation_t psa_pake_operation_init(void)
{
    const psa_pake_operation_t v = PSA_PAKE_OPERATION_INIT;
    return v;
}

This compiles and builds successfully.

@ronald-cron-arm
Copy link
Contributor Author

I've removed the changes related to crypto_extra.h as they are going to be done in #258.

@irwir
Copy link
Contributor

irwir commented May 27, 2025

Apparentely, there is no need for any extensive changes; deleting the declarations already resolves the issue.

@ronald-cron-arm
Copy link
Contributor Author

Apparentely, there is no need for any extensive changes; deleting the declarations already resolves the issue.

I've added a comment in #258, thanks.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronald-cron-arm ronald-cron-arm added needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels May 27, 2025
@ronald-cron-arm ronald-cron-arm moved this from In Development to In Review in Non-roadmap pull requests May 27, 2025
@davidhorstmann-arm davidhorstmann-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 27, 2025
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@github-project-automation github-project-automation bot moved this from In Review to Has Approval in Non-roadmap pull requests May 27, 2025
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members labels May 28, 2025
@davidhorstmann-arm davidhorstmann-arm removed the needs-backports Backports are missing or are pending review and approval. label May 28, 2025
@davidhorstmann-arm davidhorstmann-arm added this pull request to the merge queue May 28, 2025
Merged via the queue into Mbed-TLS:development with commit d7bc3bb May 28, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Has Approval to Done in Non-roadmap pull requests May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants