Add option to allocate large MLDSA objects from a global buffer #786
No reviewers
Labels
No labels
api-break
approved
bug
component-docs
component-platform
component-test
dependencies
deprecation
DO-NOT-MERGE
enhancement
good first issue
help wanted
needs-adoption
needs-backports
needs-ci
needs-design-approval
needs-info
needs-preceding-pr
needs-review
needs-reviewer
needs-work
priority-high
priority-low
priority-medium
priority-scheduled
priority-very-high
resolved-after-1.1
single-reviewer
size-l
size-m
size-optimisation
size-s
size-xs
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-Mbed-TLS/TF-PSA-Crypto!786
Loading…
Reference in a new issue
No description provided.
Delete branch "mldsa-buffer-alloc"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/645
PR checklist
Pull request overview
Adds an experimental option
TF_PSA_CRYPTO_PQCP_BUFFER_ALLOCthat makes mldsa-native allocate its large temporary objects from a single, global, statically-allocated bump buffer instead of the stack. WhenMBEDTLS_THREADING_Cis enabled, access to the buffer is serialized by a new dedicated mutex.Changes:
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.mbedtls_threading_pqcp_buffer_alloc_mutexis declared/initialised inplatform/threading.{c,h}to guard the buffer.Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC(currently enabled by default; module path also wrong).MLD_CONFIG_CONTEXT_PARAMETER,MLD_CONFIG_CUSTOM_ALLOC_FREE, and the alloc/free macros into mldsa-native.CUSTOM_ALLOC/CUSTOM_FREEbump-allocator macros.MBEDTLS_PSA_CRYPTO_C.💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZEis hard-coded to123200. This appears to be specifically the maximum stack-replaced size for MLDSA-87 signing, but ifTF_PSA_CRYPTO_PQCP_MLDSA_87_ENABLEDis not set (only smaller parameter sets are enabled in the future), this size is larger than necessary; conversely it may need to grow if a larger parameter set is added. Consider deriving this from the enabled parameter set(s) (or at least introducing a named constant per parameter set summed/maxed appropriately), and/or letting the user override the size via configuration. At minimum, the inline comment should explain how the value was determined so a future maintainer can revalidate it.@ -0,0 +48,4 @@if ((N) > 0 && (N) <= TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE / sizeof(T)) { \(v) = tf_psa_crypto_pqcp_alloc_push(MLD_ALIGN_UP(sizeof(T) * (N))); \} \} while (0)There is a potential deadlock when threading is enabled. If the very first CUSTOM_ALLOC call in a function successfully acquires the mutex (so
alloc_offsetis set to 0 by the lock), but the requested allocation does not fit becauseMLD_ALIGN_UP(sizeof(T) * (N))exceedsTF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE(the prior(N) <= BUFFER_SIZE/sizeof(T)check only boundssizeof(T)*N, not its aligned-up size), the code takes theelsebranch and setsalloc_offset = PSA_ERROR_INSUFFICIENT_MEMORY. In that case no allocation was ever made, so no FREE call will fire to release the mutex (the FREE only unlocks whenv == buffer start, andvis NULL here). All subsequent ALLOCs seealloc_offset != 0and skip re-locking, so the mutex is held forever.The accompanying comment "If we ended up here, that implies (alloc_offset != 0) thus we don't need to call mutex_unlock()" is incorrect in this edge case —
alloc_offsetcould have been just set to 0 by a successful lock in this same invocation, with no prior successful allocation. The size check or the unlock logic needs to handle this case (e.g., unlock immediately when the first-and-only allocation fails, or tighten the upper bound to account forMLD_ALIGN_UP).@ -2065,6 +2065,50 @@*/This option is uncommented by default, which means it is enabled in the default configuration. All other experimental
TF_PSA_CRYPTO_PQCP_*options in this file (e.g.,TF_PSA_CRYPTO_PQCP_MLDSA_ENABLEDline 2026,TF_PSA_CRYPTO_PQCP_OWN_SHAKEline 2066) follow the//#define ...pattern so users explicitly opt in. This line should be commented out (//#define TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC) consistent with the surrounding options and the "Uncomment to ..." instruction in the docblock.The Module path in this docblock refers to
drivers/pqcp/src/mldsa_native_buffer_alloc.c, but the actual source file introduced in this PR isdrivers/pqcp/src/pqcp_buffer_alloc.c. The path should be updated to match.@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Adjusting the size of the buffer will be handled in a follow-up PR.
@ -0,0 +48,4 @@if ((N) > 0 && (N) <= TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE / sizeof(T)) { \(v) = tf_psa_crypto_pqcp_alloc_push(MLD_ALIGN_UP(sizeof(T) * (N))); \} \} while (0)TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZEis divisible by 32, the same granularity thatMLD_ALIGN_UP()uses. This guarantees that the edge case above cannot be hit (if an allocation sizeswould fit in the buffer, an allocation of sizeṀLD_ALIGN_UP(s)is also guaranteed to fit.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?
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Where does this number come from?
I would like to have some validation somewhere. Presumably as a static assertion that compares our value with some value defined by mldsa-native.
Can we write this whole macro as
T *v = tf_psa_crypto_pqcp_custom_alloc(sizeof(T), N, &context)wheretf_psa_crypto_pqcp_custom_allocis astatic inlinefunction? It would be more readable and easier to debug.I don't like having
alloc_offsetdouble as an error code. It's error-prone. Here we could use a local variable. Where else isalloc_offsetchecked for negative values?I'm trying to understand the concurrency behavior. If I understand correctly:
alloc_offset=0. Thus the first allocation using that context object locks the buffer, which is then for exclusive use inside this thread.This works, but why not instead have our code call
? That would be simpler and wouldn't rely on the FILO order of allocations. I believe that the FILO order is likely to remain true as mldsa-native evolves, but it isn't currently guaranteed by the mldsa-native documentation.
The value of
context.alloc_offsetis how the different allocations made from one mldsa-native call don't overlap, so we rely on the context not being copied.So why not make it a global variable?
@ -0,0 +21,4 @@#include "threading_internal.h"#endif#if defined(MBEDTLS_THREADING_C)I wouldn't mind just saying that
TF_PSA_CRYPTO_PQCP_BUFFER_ALLOCis incompatible with multithreading for now, since it's intended for constrained environments. But if we do claim that it's compatible, I would like it to be tested.@ -2066,2 +2066,4 @@//#define TF_PSA_CRYPTO_PQCP_OWN_SHAKE/*** \def TF_PSA_CRYPTO_PQCP_BUFFER_ALLOCPlease add interesting entries to
tests/suites/test_suite_config.crypto_combinations.data: at leastor maybe with/without threads.
Some initial comments, but still need to do a full review.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Is there a reason we are ignoring the return of mbedtls_mutex_unlock?
Does this add a new failure mode MLD_ERR_OUT_OF_MEMORY, which is then misclassified on line drivers/pqcp/src/psa_crypto_mldsa.c:181 due to a catch all
188 /* At the time of writing, invalid signature is the only possible 189 * error condition. But this will change when we update mldsa-native 190 * with support for heap allocation of intermediate values. 191 */ 192 return PSA_ERROR_INVALID_SIGNATURE;The comment suggest this is now out of date and needs updating?
Also one general comment, do we need to add testing to validate a simultaneous signing/verification test case that forces the simultaneous use of the PQCP allocation buffer? Or is this out of scope?
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */For code size. GCC doesn't know that the value of the global variable
alloc_offsetis guaranteed to be the same after a call to any function - even one that allocates from the buffer - since the default implementation uses on-stack allocation.@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Making
alloc_offseta pass-by-value parameter makes this clear for the compiler.@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Yeah, i guess now that I'm defining macros for each entry point, this approach makes more sense.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */We can't really do much with the return value as it is at the moment - no way to communicate it to mldsa-native, etc.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */I introduced the error-code usage to have a way to fail all further allocations, as that reduced code size considerably. Without that, the generated code has to check each allocation individually to see if it succeeded, then calls
mbedtls_zeroizefor allocations.@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */I tried making the offset global and I see a size reduction: (positive = size gain for my version)
https://github.com/gilles-peskine-arm/TF-PSA-Crypto/tree/mldsa-buffer-alloc-global
What downsides do you see to my version (in terms of general design, it would need some cleanup and documentation)?
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
@ -0,0 +1,82 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */These allocator state variables don't need external linkage; leaving them non-
staticexports additional global symbols from the library unnecessarily. Mark themstaticto keep the symbols private to this translation unit.Comment typo: this underflow condition is an error case, so the comment should say it "shouldn't happen".
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Typo in comment: "occured" should be "occurred".
@ -0,0 +21,4 @@#include "threading_internal.h"#endif#if defined(MBEDTLS_THREADING_C)The lock/unlock macros use
mbedtls_threading_pqcp_buffer_alloc_mutex, but that mutex is only declared/defined whenMBEDTLS_PSA_CRYPTO_Cis enabled (see platform/threading_internal.h). WithMBEDTLS_THREADING_Cenabled butMBEDTLS_PSA_CRYPTO_Cdisabled, this header will still define the locking macros and fail to compile due to an undeclared mutex symbol. Make the lock conditional onMBEDTLS_PSA_CRYPTO_Cas well (or provide an extern in the non-PSA case).@ -8,7 +8,7 @@TF_PSA_CRYPTO_PQCP_DISABLE_CONTEXT_STUFFINGis defined/undefined around#include "wrap_mldsa_native.h", but the macro isn't referenced anywhere, so this block has no effect and the comment is misleading. Please remove it (or add the corresponding conditional logic in the header, but currently there's nothing to disable).@ -0,0 +21,4 @@#include "threading_internal.h"#endif#if defined(MBEDTLS_THREADING_C)Nothing includes
wrap_mldsa_native.hifPSA_CRYPTO_Cis disabled, so this should be fine.Still need to do a full review, however an initial comment/question?
Currently the returns from this function are ignored. As drivers/pqcp/src/psa_crypto_mldsa.c:151 immediately overrides the return with 0, is this ok?
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */I've kept the overflow-prevention check as part of the macro, just so we don't have to figure out
always_inlinefor all the compilers we support. (GCC refused to inline the check otherwise)No, that doesn't sound correct - this looks like I messed up during my last rebase.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Looking around a bit, I see that we have
always_inlinesupport for GCC (, Clang?) and IAR inalignment.h, while mldsa-native has support for GCC, Clang and MSVC.If IAR supports
_Pragma(), we could define anALWAYS_INLINEmacro, and not have to mess around with#if/#else.Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Macro name has a typo ("VERFIY") which makes the allocator size helpers harder to search for and easy to misread. Rename it to "VERIFY" and update the one use accordingly.
The
#endifcomment doesn't match the opening#ifcondition (it mentionsMBEDTLS_THREADING_PTHREADbut the guard isTF_PSA_CRYPTO_PQCP_BUFFER_ALLOC && MBEDTLS_THREADING_C). This can mislead future edits when adjusting the conditionals.@ -64,6 +88,7 @@ void smoke_87(data_t *seed)NULL, 0, public_key), 0);exit:signature_lengthis initialized toSIZE_MAX, but in error pathstf_psa_crypto_pqcp_mldsa87_signature_internal()is not guaranteed to write to it. Since the test asserts it becomes 0 on failure, initialize it to 0 so the test doesn't depend on callee behavior in failure cases.@ -64,6 +88,7 @@ void smoke_87(data_t *seed)NULL, 0, public_key), 0);exit:I think the current code is fine. Please shout if you agree with Copilot.
@ -64,6 +88,7 @@ void smoke_87(data_t *seed)NULL, 0, public_key), 0);exit:Actually, thinking about this a bit further, this will become a problem if we ever increase the buffer size to be larger than what is required for
signature_internal, as the mldsa-native function will complete successfully, but we will getMLD_ERR_FAILas the return value, due to the poison being detected bytf_psa_crypto_pqcp_alloc_done()Mostly LGTM for the execution. I like that you're testing all the new failure conditions. But there is a design choice that I think doesn't work, as it's a recurring theme in my change requests: the allocator instrumentation should be tied to entering/leaving driver entry points, rather than entering/leaving mldsa-native functions.
Also, since https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/770 got merged first, there are now more functions to instrument.
@ -63,6 +63,13 @@/* Disable SUPERCOP interface. We don't care about that. */As noted below, we'll sometimes need the pattern
So putting start/stop-allocator in wrappers doesn't always work. So I think instead we should have the pattern of starting and stopping the allocator at the beginning/end of driver entry points. This would have the major benefit of not having the extra layer of wrapper functions and macros.
@ -0,0 +1,82 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Please follow the naming convention:
psa_status_t status = PSA_xxx,int ret = MLD_xxx(orint ret = MBEDTLS_ERR_xxx, but that's not relevant here).But why would this error be translated, anyway? Why not call
tf_psa_crypto_pqcp_alloc_done()directly frompsa_crypto_mldsa.cand use its status directly?We shouldn't use
MLD_ERR_FAIL. That's too vague. This is a “can't happen” error, so it should map toPSA_ERROR_CORRUPTION_DETECTEDor perhapsPSA_ERROR_BAD_STATE. This is especially problematic with verify, whereMLD_ERR_FAILmaps toPSA_ERROR_INVALID_SIGNATURE.Note that we'll want to use
tf_psa_crypto_pqcp_alloc_bufferin our own code, not just inside mldsa-native functions. Specifically, we have a request to avoid keys on the stack inseed_to_public_key()andtf_psa_crypto_mldsa_sign_message(), and at least inseed_to_public_key(), the plan is to usetf_psa_crypto_pqcp_alloc_buffer.“This shouldn't happen” → “This should be the case”? The way it's worded now, it seems like it should not happen that there is room.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */How did you determine these values? Through manual reading of the library source, or using tooling?
We need to know in case mldsa-native evolves and the memory usage changes.
This is supposed to be sufficient for all operations. It's very likely that verify will always be cheaper than sign, but I'm not so sure about keypair. Please at least add static asserts, or a chain of
#if, to ensure that the value is always the largest size, or at least that the compilation will fail if it isn't.Why not use
MLD_TOTAL_ALLOC_87and friends?Note: also
verify_internalsince https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/770.Can we have an extra mode
POISON_ON_STARTwhich just pre-allocates a certain number of bytes, and which is tested even when threading is disabled?I'm in two minds about injecting failures through mutex functions. On the one hand, it's handy that it works without any special requirements on the library code. On the other hand, it does mean that we only test the threading case, so we might miss if something critical is guarded by
MBEDTLS_THREADING_Cwhen it shouldn't. Also, the fact that mutex functions are called through function pointers is a legacy quirk that we may want to change at some point. Overall it's a code smell. We haveMBEDTLS_TEST_HOOKSfor this sort of stuff.@ -64,6 +88,7 @@ void smoke_87(data_t *seed)NULL, 0, public_key), 0);exit:In high-level APIs, we want to ensure that
*foo_length <= foo_sizeeven on error, so we initializefoo_length = SIZE_MAXin tests and assertfoo_length <= foo_sizeafter calling the function.Here, this is a low-level API, so we don't have to ensure that property, but it's still not a bad idea.
There's no reason to initialize to 0, but we could choose not to assert
signature_lengthon failure.Somewhat related to https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/786/changes#r3368000351 and https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/786/changes#r3364569741 : testing at the level of mldsa-native function calls, rather than pqcp driver entry points, reduces the test coverage. In particular, it doesn't test error propagation. It misses bugs such as the one fixed in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/778.
We should have this testing in
test_suite_psa_crypto_mldsain addition to here, or instead of here. I don't think we gain much by having them in two places, so I think “instead of” is the right way.I've pushed the bulk of the rework - I'll add the multipart testcases, and
POISON_ON_STARTlater tonight.I have reviewed everything at ef8131f74c1cd30410509ded6e14882ead0ae12d except the changes in
.datafiles, 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.)
@ -0,0 +1,82 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Minor: this hasn't been addressed.
@ -382,12 +416,17 @@ psa_status_t tf_psa_crypto_mldsa_sign_setup(Minor: most places prioritize
alloc_statusover 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 bothINSUFFICIENT_MEMORY.Minor: “we may allocate memory” originally only referred to using
mbedtls_callocinside our own code. Now it's both that and the allocator inside mldsa-native which requires us to calltf_psa_crypto_pqcp_alloc_done(). It would help to update 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.@ -514,3 +514,35 @@ component_test_pqcp_own_shake_no_builtin () {msg "test: TF_PSA_CRYPTO_PQCP_OWN_SHAKE, built-in SHA3/SHAKE disabled"The mldsa-native shake implementation doesn't use
MLD_ALLOC. So, from a white-box perspective, we don't need to test the combination ofTF_PSA_CRYPTO_PQCP_OWN_SHAKEandTF_PSA_CRYPTO_PQCP_BUFFER_ALLOC. Given that we're trying to keep the number ofall.shcomponents down, please remove this one, as well as the corresponding line intest_suite_config.crypto_combinations.data.We're trying to keep the number of
all.shcomponents down, so I think that three components for one new option is too much. Test hooks aren't supposed to change how the code works by design (they're supposed to need some runtime action to change the behavior), so I think we don't need to test both with and without hooks. Testing both with and without threads does make sense, mostly to check that we're protecting lock/unlock calls withdefined(MBEDTLS_THREADING_C).So how about just testing hooks+threads, and prod+singlethreaded? (Impacts both
all.shandtest_suite_config.crypto_combinations.data.)@ -19,0 +34,4 @@} while (0)#else#define PQCP_INIT()#define PQCP_DONE()The CI points out that
MLD_DEFAULT_ALIGNisn't available here.Minor: maybe we should put this boilerplate into a pair of macros
PQCP_INIT(); PQCP_DONE()(like we havePSA_INIT()andPSA_DONE())?@ -48,3 +71,4 @@/* Calculate the expanded key pair from the seed */TEST_EQUAL(tf_psa_crypto_pqcp_mldsa87_keypair_internal(public_key, secret_key,We can't use
TEST_EQUALafter theexit:label: that would cause an infinite loop on failure. We need to mark the test as failed, but keep running normally. See howPSA_DONE()does it.@ -12,0 +53,4 @@0x7a, 0x2d, 0xb1, 0xf4, 0x46, 0x00, 0x27, 0xa9, 0x16, 0x2b, 0xc9, 0x9b, 0xda, 0x74, 0x25, 0x5c,0xc3, 0x36, 0x20, 0x6b, 0xfa, 0x3d, 0x55, 0xff, 0x70, 0x66, 0x8b, 0xaf, 0x97, 0x57, 0xcf, 0x5a,0xd0, 0x42, 0x7e, 0x37, 0x85, 0x66, 0xb3, 0xe6, 0x5e, 0x5d, 0x5c, 0x8f, 0xf4, 0xe8, 0x21, 0x87,0xa6, 0xe1, 0xbe, 0xee, 0x53, 0x15, 0xc3, 0xaf, 0xdb, 0xd1, 0xf9, 0x0b, 0xcf, 0xe8, 0x97, 0xec,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:
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.
@ -12,0 +296,4 @@0xdc, 0x94, 0xf9, 0x48, 0xfc, 0xdc, 0x7a, 0xd7, 0x50, 0xf7, 0xb9, 0xe4, 0x1b, 0xb9, 0x6b, 0x91,0x42, 0xd4, 0x45, 0x5d, 0x19, 0x8d, 0xba, 0x08, 0xaa, 0xcd, 0x2b, 0xf1, 0x62, 0x6f, 0xb8, 0x85,0x3b, 0xff, 0xd6, 0x08, 0x36, 0x92, 0x50, 0x58, 0x9e, 0x77, 0x3e, 0x89, 0x3d, 0x25, 0xad, 0xe7,0x91, 0xef, 0xcc, 0xdb, 0x00, 0xfd, 0x98, 0x9c, 0x4a, 0x4d, 0x99, 0xa0, 0xff, 0xf7, 0x30, 0xe6,Missing
poison_cleanupinsign_setup.@ -12,0 +529,4 @@static void poison_cleanup(void){#if defined(MBEDTLS_TEST_HOOKS)I'm fully happy with the design. I want a few more explanations in some places, a little more test coverage, and there are a couple of points I'm unsure of. The rest of my comments are optional.
@ -0,0 +9,4 @@defined(TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC)#include "wrap_mldsa_native.h"#include "src/common.h"Non-blocker: It bothers me a little that we're including an internal header of mldsa-native, only to get the definition of
MLD_ALIGN. I'm not sure whether we really should rely on their definition ofMLD_ALIGN— for example, they don't support IAR.If we make our own equivalent macro, it won't affect uses of
MLD_ALIGNinside mldsa-native anyway, so we'd need to patch mldsa-native for IAR support. However, IAR tests have passed. I think that's because the internal uses ofMLD_ALIGNare for performance only.Here,
MLD_ALIGNis needed for correctness, not just performance, on architectures where 32-bit words need to be aligned on a 4-byte boundary. (TODO: test an IAR build. Note, if it doesn't work, that's an acceptable bug for now, although we should file it as a GitHub issue.) So arguably we should do our own stuff, which would have the benefit of not including a private header.Suggestion: instead of
MLD_ALIGN, usewhich is fully portable and avoids an external dependency.
@ -0,0 +1,88 @@/* A simple bump allocator for mldsa-native, allocating from a global buffer */Please at least add a comment explaining that this is sized for applications that use all the features, and that applications that don't use all operations could use a smaller size. In particular, applications that only do verification (e.g. a bootloader) could use
MLD_TOTAL_ALLOC_87_VERIFY.We don't yet have a way to declare “I only want verification” in the configuration. But maybe we should expose
TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZEas a config option for now?@ -16,6 +16,27 @@Please remove this redundant definition.
@ -45,6 +66,9 @@ void smoke_87(data_t *seed)TEST_CALLOC(public_key, public_key_length);TEST_CALLOC(signature, signature_size);Non-blocker: That's a lot of hassle: having to extend the include path, and directly including an internal header that may change. All for a single check with limited value.
This check only depends on compile-time values, so it would be better implemented as a static assert. We can do it in the definition of
TF_PSA_CRYPTO_PQCP_CUSTOM_ALLOC:@ -7,8 +7,535 @@* validation.Minor: we're already in an
#if defined(TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC)block. It doesn't really matter, but it's weird thatpoison_setupandpoison_cleanupare inconsistent.Minor: this looks like a job for
switch.Shouldn't we assert
poison_mode == 0when the poisoning code wouldn't work?@ -10,2 +11,4 @@#include <pqcp_buffer_alloc.h>#include <psa_crypto_mldsa.h>enum poison_mode {Minor: it's often convenient to have a defined value for “go through the poisoning code, but don't actually disrupt operations”, conventionally 0. I don't see an immediate use for it though, since the poisoning test functions just wrap around the non-poisoning test functions.
@ -12,0 +33,4 @@* before we reach the method being tested.** Requires MBEDTLS_TEST_HOOKS */POISON_ON_UNLOCK,Please document what the poisoning modes do. For example,
POISON_LOCKallocatespoison_bytesbytes on allocation start in the driver entry point (which could be subtly different from reserving at the beginning of each test case, or before each call to an mldsa-native function).It's not clear to me what problem
POISON_ON_UNLOCKcatches, thatPOISON_ON_LOCKwouldn't catch.Here and in the other functions, why not point to
seed_1and so on directly?Maybe we should take a copy systematically for inputs that we tweak. But right now none of the inputs are tweaked.
For test coverage, we should test some of the failure cases of verification: some error paths might not be deallocating properly. At least we should try a public key that's too short.
Either by calling
test_export_public_keymultiple times (withmbedtls_test_set_stepfor good failure reporting), or with separate test cases with an extra parameter to indicate the input variant.Also applies to multipart verification.
For export-public and sign, the error cases should not be reachable from the API, so testing them isn't important.
@ -0,0 +1,74 @@MLDSA-87 export public key: poison on lock, sufficient memoryNon-blocker: this file is written in a very systematic way, so it would be nice to have a script.
@ -0,0 +37,4 @@# The setup step only requires MLD_TOTAL_ALLOC_87_KEYPAIR bytes of buffer memory, the signature generation happens in finish()MLDSA-87 sign setup: poison on lock, sufficient memorydepends_on:TF_PSA_CRYPTO_PQCP_MLDSA_87_ENABLED:TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC:MBEDTLS_TEST_HOOKSpoison_sign_setup:POISON_ON_LOCK:TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE-MLD_TOTAL_ALLOC_87_KEYPAIR:PSA_SUCCESSNon-blocker: The fact that
sign_setuprequiresMLD_TOTAL_ALLOC_87_KEYPAIRbytes rather thanMLD_TOTAL_ALLOC_87_SIGNwarrants a comment.@ -0,0 +53,4 @@# We don't test with full-buffer poisoning, because it would cause a failure in tf_psa_crypto_mldsa_sign_setup(), before the test gets to finish()# MLD_CONFIG_REDUCE_RAM makes keypair() take more memory than sign(), so we must disable this test.MLDSA-87 sign deterministic multipart finish: poison on lock, insufficient memoryWhy test insufficient memory for this function, but only full buffer for the others?
@ -0,0 +53,4 @@# We don't test with full-buffer poisoning, because it would cause a failure in tf_psa_crypto_mldsa_sign_setup(), before the test gets to finish()# MLD_CONFIG_REDUCE_RAM makes keypair() take more memory than sign(), so we must disable this test.MLDSA-87 sign deterministic multipart finish: poison on lock, insufficient memoryA full buffer would fail at the
setup()step - that's why I've modified it, so it only fails atfinish()@ -12,0 +33,4 @@* before we reach the method being tested.** Requires MBEDTLS_TEST_HOOKS */POISON_ON_UNLOCK,Hmm. I can only think of a few edge cases. Eg: in https://github.com/Mbed-TLS/TF-PSA-Crypto/pull/786#discussion_r3512945190 I could choose a smaller buffer size that passed
setup(), but if the memory required forsetup()was larger, I might not be able to choose a small enough poison size that only fails onfinish().@ -7,8 +7,535 @@* validation.Poisoning with a size of
0bytes is equivalent to doing nothing with any mode, which is why I only asserted on that parameter.I couldn't define the buffers as
const, because the test system rejectsconstparameters.My other options were casting away the
constmodifier, or using mutable global variables, and I thought a failure or segfault would be harder to debug. But I'm willing to go for either solution, if you'd prefer that.@ -0,0 +9,4 @@defined(TF_PSA_CRYPTO_PQCP_BUFFER_ALLOC)#include "wrap_mldsa_native.h"#include "src/common.h"For the record, AFAIU IAR supports
_Pragma("data_alignment=32")Ah, good point.
I don't like making the globals mutable, because a problem in a test case could contaminate subsequent test cases.
I don't like casting away the const either, but less so. I think it would be valid C since the pointer is only temporarily stored in an object with a pointer-to-non-const type, and then passed to a function that takes it with a pointer-to-const type.
Ideally
data_t::xwould be a pointer-to-const, but that would probably require changing a lot of code.So I lean towards casting away the const. But I'm not opposed to copying, as long as there's a comment explaining why.
(reviewed up to
8942ba92ae, no new comments)@ -45,6 +66,9 @@ void smoke_87(data_t *seed)TEST_CALLOC(public_key, public_key_length);TEST_CALLOC(signature, signature_size);The include path had to be modified so that I could include
wrap_mldsa_native.handMLD_TOTAL_ALLOC_87_*intests/suites/test_suite_psa_crypto_mldsa.poison.dataI think the existing tests should catch this in
tf_psa_crypto_pqcp_alloc_done, as long as we verify thattf_psa_crypto_pqcp_alloc_done()detects memory leaks in the buffer.Ah, I guess some failure cases might potentially fail to call
tf_psa_crypto_pqcp_alloc_done().For the multipart case, the key size is checked in setup(), but that step doesn't use the buffer allocator.
@ -12,0 +33,4 @@* before we reach the method being tested.** Requires MBEDTLS_TEST_HOOKS */POISON_ON_UNLOCK,The edge case I described above happens if
MLD_CONFIG_REDUCE_RAMis enabled.LGTM once the build failure is resolved and some documentation improvements are made
@ -0,0 +14,4 @@#if !defined(TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE)#define TF_PSA_CRYPTO_PQCP_ALLOC_BUFFER_SIZE MLD_TOTAL_ALLOC_87#endifWe need to keep
since users don't have to define the option if they're happy with the default value.
@ -2068,0 +2074,4 @@* the stack.** \warning This option is experimental. It may change or be removed without* notice.We should mention that the default value is generally suitable, and the only reason to deviate from the default is if your application doesn't use all the operations.
Also, the plausible values are
MLD_TOTAL_ALLOC_xxxmacros which are not defined by the TF-PSA-Crypto code and are not in scope when the config file is parsed. We should explain where to find the available macros in the source tree.@ -9,2 +9,4 @@#include <wrap_mldsa_native.h>#include <pqcp_buffer_alloc.h>#include <psa_crypto_mldsa.h>That's how it works internally, but not why you'd use it. The useful high-level description is that it consumes
poison_bytesextra bytes for the duration of the operation.This is somewhat similar to reducing the buffer size, but not quite. It would be useful to explain how.
@ -12,0 +33,4 @@* before we reach the method being tested.** Requires MBEDTLS_TEST_HOOKS */POISON_ON_UNLOCK,Future maintainers should be able to understand this by reading the comments on
POISON_ON_xxx. Please expand on these comments.I'd like an improvement to the public documentation, then LGTM
@ -2065,6 +2065,50 @@*/@ -2068,0 +2077,4 @@* notice.** Module: drivers/pqcp/src/pqcp_buffer_alloc.c*@ -2068,0 +2102,4 @@* notice.** Module: drivers/pqcp/src/pqcp_buffer_alloc.c*LGTM
We're under a merge ban for the release. We'll merge this PR as soon as the merge ban ends.