ML-DSA driver: support the expanded private key format #834

2026-07-08T13:57:12-05:00

gilles-peskine-arm commented

2026-07-08 13:57:12 -05:00

(Migrated from github.com)

In the ML-DSA driver, support export-public and signing where the key input is the concatenation of the seed and the expanded private key, in addition to supporting the seed only. Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/774.

Needs preceding PR: framework prerequisite https://github.com/Mbed-TLS/mbedtls-framework/pull/312

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required because: not exposed in the API yet
framework PR provided https://github.com/Mbed-TLS/mbedtls-framework/pull/312
TF-PSA-Crypto development PR here
TF-PSA-Crypto 1.1 PR not required because: new feature
mbedtls development PR not required because: new feature
mbedtls 4.1 PR not required because: new feature
mbedtls 3.6 PR not required because: new feature
tests provided

In the ML-DSA driver, support export-public and signing where the key input is the concatenation of the seed and the expanded private key, in addition to supporting the seed only. Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/774. Needs preceding PR: framework prerequisite https://github.com/Mbed-TLS/mbedtls-framework/pull/312 ## PR checklist Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line. If the provided content is part of the present PR remove the # symbol. - [x] **changelog** not required because: not exposed in the API yet - [x] **framework PR** provided https://github.com/Mbed-TLS/mbedtls-framework/pull/312 - [x] **TF-PSA-Crypto development PR** here - [x] **TF-PSA-Crypto 1.1 PR** not required because: new feature - [x] **mbedtls development PR** not required because: new feature - [x] **mbedtls 4.1 PR** not required because: new feature - [x] **mbedtls 3.6 PR** not required because: new feature - **tests** provided

bjwtaylor (Migrated from github.com) reviewed

2026-07-08 13:57:12 -05:00

bjwtaylor (Migrated from github.com) reviewed

2026-07-09 07:31:43 -05:00

bjwtaylor (Migrated from github.com) left a comment

One question on the scope, though otherwise looks good.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-07-09 08:18:44 -05:00

gilles-peskine-arm (Migrated from github.com) reviewed

2026-07-24 15:46:02 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

https://github.com/pq-code-package/mldsa-native/pull/1309 will remove the changing meaning of MLD_ERR_FAIL. So we can revert this change once that patch reaches our mldsa-native submodule.

https://github.com/pq-code-package/mldsa-native/pull/1309 will remove the changing meaning of `MLD_ERR_FAIL`. So we can revert this change once that patch reaches our mldsa-native submodule.

bjwtaylor (Migrated from github.com) reviewed

2026-07-27 03:24:07 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

Does this need to be mapped to the PSA error codes?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-07-27 03:29:23 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

I'm not sure what you mean. psa_status_t status is a PSA error code.

I'm not sure what you mean. `psa_status_t status` is a PSA error code.

bjwtaylor (Migrated from github.com) reviewed

2026-07-27 04:15:52 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

So, this PR adds sign_from_expanded() which returns a raw mldsa RC. It is correctly converted to a PSA error at psa_crypto_mldsa.c:282 using pqcp_to_psa_error. However in this case the return of sign_from_expanded is assigned directly to status which is returned here. Does this need a similar conversion, or have I missed something?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-07-27 08:56:12 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

Got it, fixed.

This alerted me to a test gap: I hadn't added poisoning tests with the private key in joined format (now done). Though it actually doesn't catch this specific mistake, because for an out-of-memory error, we return the allocator error code in preference to the mistakenly unconverted error code.

Got it, fixed. This alerted me to a test gap: I hadn't added poisoning tests with the private key in joined format (now done). Though it actually doesn't catch this specific mistake, because for an out-of-memory error, we return the allocator error code in preference to the mistakenly unconverted error code.

gilles-peskine-arm commented

2026-07-27 10:18:44 -05:00

(Migrated from github.com)

I'll push another commit to fix the code style. I'm waiting for the CI to complete, to see if there's anything else to fix.

bjwtaylor (Migrated from github.com) reviewed

2026-07-29 01:51:29 -05:00

minosgalanakis (Migrated from github.com) requested changes

2026-07-31 11:18:41 -05:00

minosgalanakis (Migrated from github.com) left a comment

Looks good and functionally does what it should. I have only two concerns one about the library changes, and one for adding testing coverage.

drivers/pqcp/src/psa_crypto_mldsa.c

					
				@ -186,0 +236,4 @@

				    } else {

				        return export_public_from_expanded(psa_get_key_bits(attributes),

				                                           key_buffer, key_buffer_size,

				                                           data, data_size, data_length);

Could we add a test covering the MLD_ERR_FAIL path from pk_from_sk() here? The non-matching-seed test only changes the leading seed, which this path skips, so the expanded private key remains valid.

We could achieve that using the current tests by corrupting a byte in tr, and checking that the test returns PSA_ERROR_INVALID_ARGUMENT

Could we add a test covering the MLD_ERR_FAIL path from pk_from_sk() here? The non-matching-seed test only changes the leading seed, which this path skips, so the expanded private key remains valid. We could achieve that using the current tests by corrupting a byte in tr, and checking that the test returns `PSA_ERROR_INVALID_ARGUMENT`

drivers/pqcp/src/psa_crypto_mldsa.c

Checking for key_size after we have called the tf_psa_crypto_pqcp_alloc_start. Effectively we permit input to unnecessarily acquire a shared allocator/mutex. Not sure if that can be exploited but I would make it to closely align the multipart setup. which reject invalid key lengths before allocating.
e.g


# Capture invalid private-key length
if (key_buffer_size != SEED_SIZE &&
      key_buffer_size != SEED_SIZE + MLDSA87_SECRETKEYBYTES) {
      return PSA_ERROR_INVALID_ARGUMENT;
  }

  if (signature_size < actual_signature_length) {
      return PSA_ERROR_BUFFER_TOO_SMALL;
  }

# Start the PQCP allocator
  psa_status_t status = tf_psa_crypto_pqcp_alloc_start();

#Then we can then assume the key has a supported length:

  if (key_buffer_size == SEED_SIZE) {
      status = sign_from_seed(...);
  } else {
      status = pqcp_to_psa_error(
          sign_from_expanded(key_buffer + SEED_SIZE, ...),
          PSA_ERROR_HARDWARE_FAILURE);
  }

So ideally we should be doing the following:

Validate algorithm, key type and parameter set.
Check signature output size and private-key length.
Acquire the PQCP allocator.
Sign or return PSA_ERROR_INVALID_ARGUMENT.

Checking for key_size after we have called the `tf_psa_crypto_pqcp_alloc_start`. Effectively we permit input to unnecessarily acquire a shared allocator/mutex. Not sure if that can be exploited but I would make it to closely align the multipart setup. which reject invalid key lengths before allocating. e.g ~~~~~~~ # Capture invalid private-key length if (key_buffer_size != SEED_SIZE && key_buffer_size != SEED_SIZE + MLDSA87_SECRETKEYBYTES) { return PSA_ERROR_INVALID_ARGUMENT; } if (signature_size < actual_signature_length) { return PSA_ERROR_BUFFER_TOO_SMALL; } # Start the PQCP allocator psa_status_t status = tf_psa_crypto_pqcp_alloc_start(); #Then we can then assume the key has a supported length: if (key_buffer_size == SEED_SIZE) { status = sign_from_seed(...); } else { status = pqcp_to_psa_error( sign_from_expanded(key_buffer + SEED_SIZE, ...), PSA_ERROR_HARDWARE_FAILURE); } ~~~~~~~ So ideally we should be doing the following: 1. Validate algorithm, key type and parameter set. 2. Check signature output size and private-key length. 3. Acquire the PQCP allocator. 4. Sign or return PSA_ERROR_INVALID_ARGUMENT.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-07-31 11:52:09 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

I don't understand the concern here. Why does it matter if the code acquires a mutex? Especially one for which we don't expect contention in practice, since the mutex only exists when using a custom allocator, and that only happens in some very constrained environments?

gilles-peskine-arm commented

2026-07-31 11:53:09 -05:00

(Migrated from github.com)

Now that the framework prerequisite is merged, I'm going to rebase this branch. I'll add extra tests in a subsequent commit.

gilles-peskine-arm commented

2026-07-31 11:59:52 -05:00

(Migrated from github.com)

Actually, there's no conflict on the framework, so I'll keep the existing history for now, plus a new framework update commit. It's better to keep the history because there were several changes of the test generator, and if I collapse all the framework updates into one, the generated code and generate_mldsa_tests.py won't be consistent with the submodule state anymore.

Actually, there's no conflict on the framework, so I'll keep the existing history for now, plus a new framework update commit. It's better to keep the history because there were several changes of the test generator, and if I collapse all the framework updates into one, the generated code and `generate_mldsa_tests.py` won't be consistent with the submodule state anymore.

minosgalanakis (Migrated from github.com) reviewed

2026-08-03 09:03:30 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

Mostly I pointed that out as a disparity to the way it is being handled in the other two places in the code. Also this change in order is introduced in this pr, so I am wondering if that was accidental.

My main concern is weather it would be a good idea to call tf_psa_crypto_pqcp_alloc_start() when not validating a valid key_buffer_size and weather it could result in a dos, from a miss-configured client.

I won't block merging this pr for it, but I wanted us to take a closer look to the allocator.

Mostly I pointed that out as a disparity to the way it is being handled in the other two places in the code. Also this change in order is introduced in this pr, so I am wondering if that was accidental. My main concern is weather it would be a good idea to call `tf_psa_crypto_pqcp_alloc_start()` when not validating a valid `key_buffer_size` and weather it could result in a dos, from a miss-configured client. I won't block merging this pr for it, but I wanted us to take a closer look to the allocator.

minosgalanakis (Migrated from github.com) reviewed

2026-08-03 09:03:47 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

					
				@ -186,0 +236,4 @@

				    } else {

				        return export_public_from_expanded(psa_get_key_bits(attributes),

				                                           key_buffer, key_buffer_size,

				                                           data, data_size, data_length);

Thank you for adressing this

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-03 09:20:53 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

Ok, it's good in principle to avoid more complex processing on error paths. But it's not like we can avoid having errors pop up with the allocator active. So it would only be a very minor robustness improvement. I don't think it's worth duplicating the key_buffer_size checks over this.

Ok, it's good in principle to avoid more complex processing on error paths. But it's not like we can avoid having errors pop up with the allocator active. So it would only be a very minor robustness improvement. I don't think it's worth duplicating the `key_buffer_size` checks over this.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-03 10:50:16 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

I'm still fiddling with the follow-up related to expanded keys https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/862 and I might end up refactoring this function (as part of being a bit more systematic about deducing seed/joined/invalid from the private key size and the bits value). Since it's only a minor robustness issue, I'd prefer to do it in the follow-up, if at all.

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-04 04:26:51 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

I've done the refactoring I was thinking of when consuming a private key, and that actually fits into this PR thematically. So I might push it here. But I'm not sure I actually like it.

It pushes the (bits, private_key_size) → (validity, format) decision into a single function, so it's now fully uniform. Here, it does have the effect of moving one validation outside the buffer lock/unlock.

In terms of code size, it's a wash.

I'm not sure I like it because I find the new paths more convoluted. On the one hand, we now have a single point of validation, which is a good thing in principle. On the other hand, when you follow the individual paths, I originally had buffer size checks in the same function that relied on them. Now I find it harder to verify that we're passing valid buffer sizes when we call mldsa-native functions. That argument makes me still prefer the original design.

What do you think?

I've done the refactoring I was thinking of when consuming a private key, and that actually fits into this PR thematically. So I might push it here. But I'm not sure I actually like it. It pushes the (bits, private_key_size) → (validity, format) decision into a single function, so it's now fully uniform. Here, it does have the effect of moving one validation outside the buffer lock/unlock. In terms of code size, it's a wash. I'm not sure I like it because I find the new paths more convoluted. On the one hand, we now have a single point of validation, which is a good thing in principle. On the other hand, when you follow the individual paths, I originally had buffer size checks in the same function that relied on them. Now I find it harder to verify that we're passing valid buffer sizes when we call mldsa-native functions. That argument makes me still prefer the original design. What do you think?

gilles-peskine-arm (Migrated from github.com) reviewed

2026-08-05 05:02:06 -05:00

drivers/pqcp/src/psa_crypto_mldsa.c

Having thought about it overnight, I don't like having the buffer size checks distant from where they matter. Keeping them close means that they will sometimes happen with the buffer mutex held. So I don't plan to make any changes here.

We might want to do some refactoring of buffer size checks when we support multiple parameter sets, but even then, I think the checks may happen with the lock held.

Having thought about it overnight, I don't like having the buffer size checks distant from where they matter. Keeping them close means that they will sometimes happen with the buffer mutex held. So I don't plan to make any changes here. We might want to do some refactoring of buffer size checks when we support multiple parameter sets, but even then, I think the checks may happen with the lock held.