PK write: avoid large stack buffer #684

2026-02-05T03:52:46-06:00

mpg commented

2026-02-05 03:52:46 -06:00

(Migrated from github.com)

Description

Avoid uselessly allocating a large buffer on the stack when writing out RSA keypairs.

A step towards addressing https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/476

PR checklist

changelog not required because: will be added when we finally fix 476
framework PR not required
mbedtls development PR not required because: crypto only
mbedtls 3.6 PR provided https://github.com/Mbed-TLS/mbedtls/pull/10592
tests provided

## Description Avoid uselessly allocating a large buffer on the stack when writing out RSA keypairs. A step towards addressing https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/476 ## PR checklist - [x] **changelog** not required because: will be added when we finally fix 476 - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: crypto only - [x] **mbedtls 3.6 PR** provided https://github.com/Mbed-TLS/mbedtls/pull/10592 - **tests** provided

valeriosetti (Migrated from github.com) requested changes

2026-02-05 04:43:17 -06:00

valeriosetti (Migrated from github.com) left a comment

All good. I only left a comment for improving test code.

tests/suites/test_suite_pkwrite.function

As I was recently remembered by @gilles-peskine-arm, using macros like TEST_EQUAL() does not cause the test function to terminate immediately. It just marked as failed but then it continues until the next real failure (if any) or to the normal termination.
As a consequence I think that pk_write_check_context should return an error on failure and then consuming functions like pk_write_check_common should just do:

ret = pk_write_check_context(...)
if (ret != 0) {
   goto exit;
}

As I was [recently remembered](https://github.com/Mbed-TLS/mbedtls-framework/pull/257#discussion_r2686772354) by @gilles-peskine-arm, using macros like `TEST_EQUAL()` does not cause the test function to terminate immediately. It just marked as failed but then it continues until the next real failure (if any) or to the normal termination. As a consequence I think that `pk_write_check_context` should return an error on failure and then consuming functions like `pk_write_check_common` should just do: ``` ret = pk_write_check_context(...) if (ret != 0) { goto exit; } ```

mpg (Migrated from github.com) reviewed

2026-02-05 05:23:27 -06:00

tests/suites/test_suite_pkwrite.function

Ah, thanks for the reminder! I don't think really a problem here: if the caller just continues to execute after a failure, it just possibly test again with a wrapping context, which is admittedly a waste of time, but not causing any real issues.

Anyway, I'm changing as you suggested, for the sake of robustness.

Ah, thanks for the reminder! I don't think really a problem here: if the caller just continues to execute after a failure, it just possibly test again with a wrapping context, which is admittedly a waste of time, but not causing any real issues. Anyway, I'm changing as you suggested, for the sake of robustness.

valeriosetti (Migrated from github.com) approved these changes

2026-02-05 05:35:13 -06:00

valeriosetti (Migrated from github.com) reviewed

2026-02-05 05:37:29 -06:00

tests/suites/test_suite_pkwrite.function

Yeah, I agree that in this case this wouldn't have been problematic. My intent is mostly to keep some good practices and common patterns so that if in the future someone else would like to implement something and take a look at what's already there, they will find a good reference

valeriosetti (Migrated from github.com) reviewed

2026-02-05 05:37:40 -06:00

tests/suites/test_suite_pkwrite.function

Thanks for addressing this!

bjwtaylor (Migrated from github.com) reviewed

2026-02-06 09:16:05 -06:00

bjwtaylor (Migrated from github.com) left a comment

Just one small question.

drivers/builtin/src/pkwrite.c

					
				@ -56,0 +50,4 @@

				     * we were supposed to write to its end. */

				    *p -= key_len;

				    memmove(*p, buf, key_len);

				    mbedtls_platform_zeroize(buf, *p - buf);

I know this is pre-existing. However does the psa_export_key guarantee not to leak during a failure? If not is worth zeroising the buffer? Maybe even worth doing it anyway just as good practice?

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

2026-02-06 15:31:07 -06:00

drivers/builtin/src/pkwrite.c

					
				@ -56,0 +50,4 @@

				     * we were supposed to write to its end. */

				    *p -= key_len;

				    memmove(*p, buf, key_len);

				    mbedtls_platform_zeroize(buf, *p - buf);

All PSA API functions are supposed to leave “safe” outputs on failure. That in particular means no confidential data if the function fails.

mpg (Migrated from github.com) reviewed

2026-02-09 02:37:06 -06:00

drivers/builtin/src/pkwrite.c

					
				@ -56,0 +50,4 @@

				     * we were supposed to write to its end. */

				    *p -= key_len;

				    memmove(*p, buf, key_len);

				    mbedtls_platform_zeroize(buf, *p - buf);

Aha, I asked myself the same question while editing the code, then got focused on something else and forgot about this, so thank you Ben for raising it :)

All PSA API functions are supposed to leave “safe” outputs on failure. That in particular means no confidential data if the function fails.

Looking at the API reference, 5.3.2 says: "the content of output parameters is not defined", but a bit below: "When a function returns an error status, it is recommended that implementations set output parameters to safe defaults to avoid leaking confidential data and limit risk, in case an application does not properly handle all errors."

So I guess users should not rely on the content being safe, but as implementers we are imposing this on ourselves as a matter of implementation quality.

But here in PK we're users of the PSA Crypto API, so perhaps we should not make any assumption beyond what the API reference guarantees? Also, if drivers are in use, I don't think we can assume they're going to follow the recommendation as we do.

Aha, I asked myself the same question while editing the code, then got focused on something else and forgot about this, so thank you Ben for raising it :) > All PSA API functions are supposed to leave “safe” outputs on failure. That in particular means no confidential data if the function fails. Looking at the API reference, [5.3.2](https://arm-software.github.io/psa-api/crypto/1.4/overview/conventions.html#behavior-on-error) says: "the content of output parameters is not defined", but a bit below: "When a function returns an error status, it is recommended that implementations set output parameters to safe defaults to avoid leaking confidential data and limit risk, in case an application does not properly handle all errors." So I guess users should not rely on the content being safe, but as implementers we are imposing this on ourselves as a matter of implementation quality. But here in PK we're users of the PSA Crypto API, so perhaps we should not make any assumption beyond what the API reference guarantees? Also, if drivers are in use, I don't think we can assume they're going to follow the recommendation as we do.

mpg (Migrated from github.com) reviewed

2026-02-10 02:39:10 -06:00

drivers/builtin/src/pkwrite.c

					
				@ -56,0 +50,4 @@

				     * we were supposed to write to its end. */

				    *p -= key_len;

				    memmove(*p, buf, key_len);

				    mbedtls_platform_zeroize(buf, *p - buf);

I think the conclusion of yesterday's discussion is that no change is needed here. It is still under discussion, as part of the effort on standardising the driver API, whether the responsibility of ensuring the psa_export_key() doesn't leave confidential data around on failure is on the driver or the core, but in any case it is not PK's responsibility.

As a result, I think no change is needed here. @bjwtaylor if that's also your understanding of the discussion, I think this discussion can be marked as resolved.

I think the conclusion of yesterday's discussion is that no change is needed here. It is still under discussion, as part of the effort on standardising the driver API, whether the responsibility of ensuring the `psa_export_key()` doesn't leave confidential data around on failure is on the driver or the core, but in any case it is not PK's responsibility. As a result, I think no change is needed here. @bjwtaylor if that's also your understanding of the discussion, I think this discussion can be marked as resolved.

bjwtaylor (Migrated from github.com) reviewed

2026-02-10 02:42:12 -06:00

drivers/builtin/src/pkwrite.c

					
				@ -56,0 +50,4 @@

				     * we were supposed to write to its end. */

				    *p -= key_len;

				    memmove(*p, buf, key_len);

				    mbedtls_platform_zeroize(buf, *p - buf);

Agreed, I think we can resolve this as per the discussion yesterday.

bjwtaylor (Migrated from github.com) approved these changes

2026-02-10 02:42:46 -06:00

Sign in to join this conversation.

No reviewers

valeriosetti

bjwtaylor

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!684

Rows
Columns