Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement and use MBEDTLS_STATIC_ASSERT() #7229

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Implement and use MBEDTLS_STATIC_ASSERT() #7229

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ef9bb3
Implement and use MBEDTLS_STATIC_ASSERT()
tom-cosgrove-arm Mar 8, 2023
57f04b8
Have MBEDTLS_STATIC_ASSERT() match current development more closely
tom-cosgrove-arm Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions library/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "mbedtls/build_info.h"
#include "alignment.h"

#include <assert.h>
#include <stddef.h>
#include <stdint.h>
#include <stddef.h>
Expand Down Expand Up @@ -149,4 +150,34 @@ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned
#endif
/* *INDENT-ON* */

/* Always provide a static assert macro, so it can be used unconditionally.
* Note that it will expand to nothing on some systems.
* Can be used outside functions (but don't add a trailing ';' in that case:
* the semicolon is included here to avoid triggering -Wextra-semi when
* MBEDTLS_STATIC_ASSERT() expands to nothing).
* Can't use the C11-style `defined(static_assert)` on FreeBSD, since it
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you didn't find a way to bypass than warning 😞 . Maybe we could detect FreeBSD and use _Static_assert there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_Static_assert() is only available depending what you're building for. But yes, that could be added in a follow-up PR

* defines static_assert even with -std=c99, but then complains about it.
*/
#if defined(static_assert) && !defined(__FreeBSD__)
Copy link
Contributor

Choose a reason for hiding this comment

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

In C23, static_assert will no longer be a macro, it's a keyword. Should we add || __STDC_VERSION__ >= 202300L? Or just make a note to add this when C23 is finalized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've already decided not to pre-add stuff for C23 on other PRs, so let's not add the __STDC_VERSION__ check. Not sure of the benefit of adding a TODO into the code, vs coming back once C23 is a real thing

#define MBEDTLS_STATIC_ASSERT(expr, msg) static_assert(expr, msg);
#elif defined(__COUNTER__)
/* gcc will say "size of array ‘mbedtls_static_assert_failedN’ is negative"
* (and with -pedantic will complain further);
* clang will say "'mbedtls_static_assert_failedN' declared as an array with a
* negative size";
* Visual Studio will just say "error C2118: negative subscript" (without the
* mbedtls_static_assert_failedN part)
*/
#if defined(__GNUC__)
#define MBEDTLS_UNUSED __attribute__((unused))
#else
#define MBEDTLS_UNUSED
#endif
#define MBEDTLS_STATIC_ASSERT2(expr, count) extern int MBEDTLS_UNUSED mbedtls_static_assert_failed ## count [2 * !!(expr) - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need __COUNTER__? What's the benefit over the implementation in test/macros.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.

STATIC_ASSERT_EXPR() can only be used wherever an expression can be used, i.e. wouldn't work in some of the existing cases.

__COUNTER__ has been around in gcc for at least 10 years, so the systems without it (which this will still work on) should be fairly few.

Note I was aiming for "better (and we can build on it)" rather than "perfect"

#define MBEDTLS_STATIC_ASSERT1(expr, count) MBEDTLS_STATIC_ASSERT2(expr, count)
#define MBEDTLS_STATIC_ASSERT(expr, msg) MBEDTLS_STATIC_ASSERT1(expr, __COUNTER__)
#else
#define MBEDTLS_STATIC_ASSERT(expr, msg)
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also use this in tests/include/test/macros.h to replace STATIC_ASSERT_EXPR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, should be possible, but I haven't tested it. That would make an excellent follow-up PR

#endif /* MBEDTLS_LIBRARY_COMMON_H */
27 changes: 13 additions & 14 deletions library/psa_crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@

#include "psa_crypto_random_impl.h"

#include <assert.h>
#include <stdlib.h>
#include <string.h>
#include "mbedtls/platform.h"
Expand Down Expand Up @@ -1471,14 +1470,15 @@ psa_status_t psa_export_public_key(mbedtls_svc_key_id_t key,
return (status == PSA_SUCCESS) ? unlock_status : status;
}

#if defined(static_assert)
static_assert((MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
"One or more key attribute flag is listed as both external-only and dual-use");
static_assert((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
"One or more key attribute flag is listed as both internal-only and dual-use");
static_assert((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY) == 0,
"One or more key attribute flag is listed as both internal-only and external-only");
#endif
MBEDTLS_STATIC_ASSERT(
(MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
"One or more key attribute flag is listed as both external-only and dual-use")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annoyingly I couldn't find a way to have the macro work outside functions where it could have a ; after it, since if it expands to nothing, you end up with a stray ; that the compiler complains about

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it work to make it expand to a function declaration?

MBEDTLS_STATIC_ASSERT(
(PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0,
"One or more key attribute flag is listed as both internal-only and dual-use")
MBEDTLS_STATIC_ASSERT(
(PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY) == 0,
"One or more key attribute flag is listed as both internal-only and external-only")

/** Validate that a key policy is internally well-formed.
*
Expand Down Expand Up @@ -1742,11 +1742,10 @@ static psa_status_t psa_finish_key_creation(
psa_key_slot_number_t slot_number =
psa_key_slot_get_slot_number(slot);

#if defined(static_assert)
static_assert(sizeof(slot_number) ==
sizeof(data.slot_number),
"Slot number size does not match psa_se_key_data_storage_t");
#endif
MBEDTLS_STATIC_ASSERT(sizeof(slot_number) ==
sizeof(data.slot_number),
"Slot number size does not match psa_se_key_data_storage_t");

memcpy(&data.slot_number, &slot_number, sizeof(slot_number));
status = psa_save_persistent_key(&slot->attr,
(uint8_t *) &data,
Expand Down
8 changes: 3 additions & 5 deletions library/psa_crypto_se.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

#if defined(MBEDTLS_PSA_CRYPTO_SE_C)

#include <assert.h>
#include <stdint.h>
#include <string.h>

Expand Down Expand Up @@ -313,10 +312,9 @@ psa_status_t psa_register_se_driver(
}
/* Driver table entries are 0-initialized. 0 is not a valid driver
* location because it means a transparent key. */
#if defined(static_assert)
static_assert(PSA_KEY_LOCATION_LOCAL_STORAGE == 0,
"Secure element support requires 0 to mean a local key");
#endif
MBEDTLS_STATIC_ASSERT(PSA_KEY_LOCATION_LOCAL_STORAGE == 0,
"Secure element support requires 0 to mean a local key");

if (location == PSA_KEY_LOCATION_LOCAL_STORAGE) {
return PSA_ERROR_INVALID_ARGUMENT;
}
Expand Down
10 changes: 3 additions & 7 deletions library/ssl_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@

#if defined(MBEDTLS_SSL_TLS_C)

#include <assert.h>

#include "mbedtls/platform.h"

#include "mbedtls/ssl.h"
Expand Down Expand Up @@ -1196,11 +1194,9 @@ static int ssl_handshake_init(mbedtls_ssl_context *ssl)
size_t sig_algs_len = 0;
uint16_t *p;

#if defined(static_assert)
static_assert(MBEDTLS_SSL_MAX_SIG_ALG_LIST_LEN
<= (SIZE_MAX - (2 * sizeof(uint16_t))),
"MBEDTLS_SSL_MAX_SIG_ALG_LIST_LEN too big");
#endif
MBEDTLS_STATIC_ASSERT(MBEDTLS_SSL_MAX_SIG_ALG_LIST_LEN
<= (SIZE_MAX - (2 * sizeof(uint16_t))),
"MBEDTLS_SSL_MAX_SIG_ALG_LIST_LEN too big");

for (md = sig_hashes; *md != MBEDTLS_MD_NONE; md++) {
if (mbedtls_ssl_hash_from_md_alg(*md) == MBEDTLS_SSL_HASH_NONE) {
Expand Down
7 changes: 3 additions & 4 deletions library/ssl_tls12_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,10 +1510,9 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl)
MBEDTLS_TLS_SIG_NONE
};

#if defined(static_assert)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: since there was no #include <assert.h> at the top of this file, defined(static_assert) would always be false, so this check was never going to happen - just showing that we should have an "always-present" static assert like this

static_assert(sizeof(default_sig_algs) / sizeof(default_sig_algs[0]) <=
MBEDTLS_RECEIVED_SIG_ALGS_SIZE, "default_sig_algs is too big");
#endif
MBEDTLS_STATIC_ASSERT(sizeof(default_sig_algs) / sizeof(default_sig_algs[0])
<= MBEDTLS_RECEIVED_SIG_ALGS_SIZE,
"default_sig_algs is too big");

memcpy(received_sig_algs, default_sig_algs, sizeof(default_sig_algs));
}
Expand Down