Add option to allocate large MLDSA objects from a global buffer#786
Add option to allocate large MLDSA objects from a global buffer#786bensze01 wants to merge 36 commits into
Conversation
bensze01
changed the title
bensze01
force-pushed
the
mldsa-buffer-alloc
branch
8 times, most recently
from
d3e85ab to
8b57228
Compare
bensze01
force-pushed
the
mldsa-buffer-alloc
branch
5 times, most recently
from
cfee876 to
b937dbf
Compare
bensze01
force-pushed
the
mldsa-buffer-alloc
branch
3 times, most recently
from
39a6e29 to
e46c386
Compare
bensze01
added
enhancement
There was a problem hiding this comment.
Pull request overview
Adds an experimental option TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC that makes mldsa-native allocate its large temporary objects from a single, global, statically-allocated bump buffer instead of the stack. When MBEDTLS_THREADING_C is enabled, access to the buffer is serialized by a new dedicated mutex.
Changes:
- New
pqcp_buffer_alloc.{h,c}defines the global buffer andMLD_CUSTOM_ALLOC/MLD_CUSTOM_FREEmacros plumbed into mldsa-native viapqcp-config.hand a per-call empty context stuffed by macros inwrap_mldsa_native.h. - A new global mutex
mbedtls_threading_pqcp_buffer_alloc_mutexis declared/initialised inplatform/threading.{c,h}to guard the buffer. - New configuration option (config metadata + scripts + two new CI components exercising the option with and without pthreads).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| include/psa/crypto_config.h | Adds docblock + define for TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC (currently enabled by default; module path also wrong). |
| scripts/data_files/config-options-current.txt | Registers the new option name. |
| scripts/config.py | Excludes the new option from blanket "full" configs. |
| drivers/pqcp/src/pqcp-config.h | Wires MLD_CONFIG_CONTEXT_PARAMETER, MLD_CONFIG_CUSTOM_ALLOC_FREE, and the alloc/free macros into mldsa-native. |
| drivers/pqcp/src/pqcp_buffer_alloc.h | Defines the global buffer, context struct and CUSTOM_ALLOC/CUSTOM_FREE bump-allocator macros. |
| drivers/pqcp/src/pqcp_buffer_alloc.c | Provides the storage for the global buffer. |
| drivers/pqcp/src/wrap_mldsa_native.h | Adds per-function macro wrappers that inject a zero-initialised context argument when context-stuffing is enabled. |
| drivers/pqcp/src/wrap_mldsa_native.c | Disables the new wrapper macros locally so the real function definitions are not affected, and tightens the guard with MBEDTLS_PSA_CRYPTO_C. |
| platform/threading.c, platform/threading_internal.h | Declares/initialises/destroys the new mutex when the option is enabled. |
| tests/scripts/components-configuration-crypto.sh | Adds two CI components covering the new option with and without pthread. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gilles-peskine-arm
added
the
needs-review
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
I've done a design review, not yet a full code review.
The design feels more complex and fragile than it needs to be. So please document the rationale. I especially want to understand why a context parameter is needed at all: can't we just make the offset a global variable, given that we end up having only one mldsa-native function active at time anyway?
gilles-peskine-arm
removed
the
needs-review
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
This only affects mldsa65, as all other parameter sets were already divisible by 32. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
We call the RNG rather than letting mldsa-native do so, so these non-internal functions are unused Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Extend the smoke test to allow poisoning the buffer allocator's state on locking or unlocking its mutex. This allows us to test the macros we use to calculate the memory usage of individual mldsa-native functions, as well as whether the mutex effectively controls access to the allocator. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
POISON_ON_UNLOCK would leave the allocator in a state that caused the following unit test to fail. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
These pointers need to be initialized before the first call to a macro that can jump to exit: Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
bjwtaylor
left a comment
bjwtaylor
left a comment
There was a problem hiding this comment.
Approved subject to Gilles comments.
bensze01
force-pushed
the
mldsa-buffer-alloc
branch
from
a23fdff to
1aab344
Compare
bensze01
force-pushed
the
mldsa-buffer-alloc
branch
from
1aab344 to
ef8131f
Compare
This reverts commit 3dda749. Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
Signed-off-by: Bence Szépkúti <bence.szepkuti@arm.com>
|
I've pushed the bulk of the rework - I'll add the multipart testcases, and |
There was a problem hiding this comment.
I have reviewed everything at ef8131f except the changes in .data files, i.e. basically everything except the new poisoning tests. I haven't reviewed that part partly because that's all I have time for today, and partly because as noted below I don't like adding more parameters to the existing functions, but I want to sleep on it.
Other than this aspect, I'm happy with the design and the execution except in a few minor places. (Also the CI is unhappy.)
| return status; | ||
| } | ||
|
|
||
| /* After this point, we may allocate memory, so we must go through |
There was a problem hiding this comment.
Minor: “we may allocate memory” originally only referred to using mbedtls_calloc inside our own code. Now it's both that and the allocator inside mldsa-native which requires us to call tf_psa_crypto_pqcp_alloc_done(). It would help to update this comment.
| psa_status_t status = tf_psa_crypto_pqcp_alloc_start(); | ||
| if (status != PSA_SUCCESS) { | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Minor: please add a comment reminding that from that point onwards, we must not return without calling tf_psa_crypto_pqcp_alloc_done().
Same in verify_finish.
| mbedtls_platform_zeroize(operation, sizeof(*operation)); | ||
| } | ||
| return status; | ||
| if (status == PSA_SUCCESS) { |
There was a problem hiding this comment.
Minor: most places prioritize alloc_status over the “normal” status, but here it's the opposite. That's not really important since apart from programming errors in mldsa-native or in our code, the only case where they're both nonzero should be when they're both INSUFFICIENT_MEMORY.
| cleanup: | ||
| mbedtls_free(public_key); | ||
| psa_status_t alloc_status = tf_psa_crypto_pqcp_alloc_done(); | ||
| if (status != PSA_SUCCESS) { |
There was a problem hiding this comment.
| if (status != PSA_SUCCESS) { | |
| if (status != PSA_SUCCESS || alloc_status != PSA_SUCCESS) { |
| ctest | ||
| } | ||
|
|
||
| component_test_pqcp_buffer_alloc_own_shake_no_builtin_pthread () { |
There was a problem hiding this comment.
The mldsa-native shake implementation doesn't use MLD_ALLOC. So, from a white-box perspective, we don't need to test the combination of TF_PSA_CRYPTO_PQCP_OWN_SHAKE and TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC. Given that we're trying to keep the number of all.sh components down, please remove this one, as well as the corresponding line in test_suite_config.crypto_combinations.data.
|
|
||
| PSA_INIT(); | ||
|
|
||
| PSA_ASSERT(poison_setup(poison_mode, poison_bytes)); |
There was a problem hiding this comment.
Missing poison_cleanup in sign_setup.
| return NULL; | ||
| } | ||
|
|
||
| /* Check that there is room. This shouldn't happen if the buffer size |
There was a problem hiding this comment.
Minor: this hasn't been addressed.
|
|
||
| #if defined(TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC) | ||
| /* Alignment sanity check*/ | ||
| TEST_EQUAL(TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE % MLD_DEFAULT_ALIGN, 0); |
There was a problem hiding this comment.
The CI points out that MLD_DEFAULT_ALIGN isn't available here.
|
|
||
| static void poison_cleanup(void) | ||
| { | ||
| #if defined(MBEDTLS_TEST_HOOKS) |
There was a problem hiding this comment.
| #if defined(MBEDTLS_TEST_HOOKS) | |
| #if defined(TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC) && defined(MBEDTLS_TEST_HOOKS) |
| psa_status_t expected_status) | ||
| psa_status_t expected_status, | ||
| int poison_mode, | ||
| int64_t poison_bytes) |
There was a problem hiding this comment.
I'm unsure about the test arrangements. I like the poisoning mechanism. What I don't like much is that it's added to all the test functions. It has quite a few downsides:
- More complex code in functions whose main goal is to test other aspects: data validation, normal operation or both.
- More complex test data.
- In particular the test generator needs to change, which is a bit of a hassle due to the repository split. (You need to make the new behavior an option to the test generator in
scripts/maintainer/generate_mldsa_tests.py, like what's already there withmultipart=True.)
I'm not fully convinced, but I think it would be better to have dedicated functions for the poisoning. These functions wouldn't need a variety of test data, and they don't need to test the invalid inputs that we reject before calling mldsa-native functions.
gilles-peskine-arm
added
needs-work
and removed
needs-review
Resolves #645
PR checklist