[tf-psa-crypto] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 #212

valeriosetti · 2025-03-18T13:42:53Z

Description

This helps resolving Mbed-TLS/mbedtls#9618

CI note

Due to dependencies with the main repo PR please evaluate the CI of 10090 as validation of this PR.

PR checklist

changelog done
framework PR
mbedtls development PR [development] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 mbedtls#10090
mbedtls 3.6 PR not required: new feature
tests TODO

gilles-peskine-arm

I just did a design review. I won't be able to do a full code review.

We just realized this morning that the issue description wasn't very clear, and several things were in Janos's mind and mine but we hadn't written them down. Sorry about this. I added a note to the issue and some comments here, hopefully that should clarify things.

gilles-peskine-arm · 2025-03-19T13:13:51Z

drivers/builtin/include/mbedtls/platform.h

+ *                              in such information, so the function must handle
+ *                              this.
+ */
+int mbedtls_entropy_hardware_poll(unsigned char *output, size_t output_size,


Non-blocker: since the function is now declared in platform.h, should we call it mbedtls_platform_xxx? Same question about the configuration option. Maybe MBEDTLS_PLATFORM_GET_ENTROPY_ALT and mbedtls_platform_get_entropy()?

gilles-peskine-arm · 2025-03-19T13:15:52Z

drivers/builtin/include/mbedtls/platform.h

+ * \param[out] entropy_content  Measure of the entropy content (in bits) of the
+ *                              data written in the \p output buffer. The pointer


Please add the constraint that for the time being, the function must set entropy_content = 8 * *output_len on success. The library will be extended later to support partial entropy.

gilles-peskine-arm · 2025-03-19T13:16:39Z

drivers/builtin/include/mbedtls/platform.h

+ *                              can be \c NULL in case the caller is not interested
+ *                              in such information, so the function must handle
+ *                              this.


We don't need to support callers that don't want the value. entropy_content is guaranteed to be non-null.

gilles-peskine-arm · 2025-03-19T13:23:55Z

drivers/builtin/include/mbedtls/platform.h

+ * \brief           User defined callback function that is used from the entropy
+ *                  module to gather entropy data from some hardware device.
+ *
+ * \warning         This is not provided by Mbed TLS.


I don't think “not provided by Mbed TLS” is a useful warning. Also, it's TF-PSA-Crypto now.

On the other hand, the function's documentation should state that this function is only used when MBEDTLS_ENTROPY_HARDWARE_POLL is defined. This is otherwise not visible when you read the Doxygen-rendered documentation.

gilles-peskine-arm · 2025-03-19T13:26:32Z

ChangeLog.d/9618.txt

+     `MBEDTLS_ENTROPY_HARDWARE_POLL_MIN` can optionally optinally define the
+     minimum number of bytes that should be polled using that function before
+     declaring entropy gathering completed. If not defined, then a default
+     value of 32 bytes is used.


MBEDTLS_ENTROPY_HARDWARE_POLL_MIN should not be a configuration parameter, at least not in this form. The RNG needs a certain amount of entropy. This minimum is up to the system configuration (and in particular the required cryptographic strength for the system), not up to the hardware integration. The person writing the TRNG device driver doesn't get to choose this value.

I think we should just use the existing MBEDTLS_ENTROPY_MIN_PLATFORM here. The distinction between “platform source” and “hardware source” doesn't really make sense as it's currently described in entropy_poll.h. Actually the “platform source” is the operating system integrations that the library supports out of the box, and the “hardware source” is the one for other systems where the integrator needs to provide a driver.

gilles-peskine-arm · 2025-03-19T13:28:16Z

ChangeLog.d/9618.txt

+     removed.
+
+Features
+   * The new symbol `MBEDTLS_ENTROPY_HARDWARE_POLL` is added to allow the


The changelog entries should probably mention the function names as well as the option names, for easier searching.

gilles-peskine-arm · 2025-03-19T13:35:52Z

drivers/builtin/src/entropy_poll.h

+        void *data, unsigned char *output, size_t len, size_t *olen)
+{
+    (void) data;
+    return mbedtls_entropy_hardware_poll(output, len, olen, NULL);


The wrapper should check the entropy content and fail if it's less than full entropy, i.e. if entropy_output < 8 * *olen. Please add a unit test for that in test_suite_entropy.

Also the entropy collection should fail if *olen is too small and as a result there isn't enough entropy, collected from all sources. I believe this is covered by the existing tests in test_suite_entropy, but please double-check.

The wrapper should check the entropy content and fail if it's less than full entropy, i.e. if entropy_output < 8 * *olen. Please add a unit test for that in test_suite_entropy.

While adding the check on the wrapper and testing it with the new component tf_psa_crypto_platform_get_entropy_alt I found that tests are already failing if the returned entropy content is not correct, so there seems to be no need for a new test step. As reference implementation I'm using the one proposed in the companion PR Mbed-TLS/mbedtls-framework#151.

valeriosetti · 2025-04-01T20:06:36Z

I think that CI failures in test_psasim and test_suite_with_psasim can be ignored in this PR as the same tests in the main PR are OK

valeriosetti · 2025-04-01T20:07:56Z

I just forced push the last commit following the merge of Mbed-TLS/mbedtls-framework#151. No other change was made

valeriosetti · 2025-04-04T12:35:58Z

I think that CI failures in test_psasim and test_suite_with_psasim can be ignored in this PR as the same tests in the main PR are OK

Thinking on this a bit more I think I can explain why psasim tests fail in this PR. The PR on the main repo modifies config.py (only in the main repo) so that the full config doesn't set MBEDTLS_PLATFORM_GET_ENTROPY_ALT.
The problem is that when Mbed-TLS/mbedtls#10090 is tested then it pick this PR as the tf-psa-crypto version. The opposite is not true: when this PR is tested on the CI, the main repo is checked out at development and there the full config enables MBEDTLS_PLATFORM_GET_ENTROPY_ALT.

gilles-peskine-arm · 2025-04-30T08:33:11Z

It's problematic to merge something in the crypto repository that breaks the CI, since it means crypto will be broken on all branches that don't have this PR merged yet.

Can you please add a transition function mbedtls_platform_entropy_poll that's just good enough for how it's used in selftest.c, with a comment stating that it's going to be removed soon? And might as well make a PR that reverts that commit, to be merged in a few weeks.

valeriosetti · 2025-04-30T09:58:39Z

Can you please add a transition function mbedtls_platform_entropy_poll that's just good enough for how it's used in selftest.c, with a comment stating that it's going to be removed soon? And might as well make a PR that reverts that commit, to be merged in a few weeks.

@gilles-peskine-arm I just created #273 to track this

valeriosetti · 2025-04-30T10:22:41Z

@gilles-peskine-arm although I added the stub function for mbedtls_platform_entropy_poll there are remaining failures on the CI related to the fact that the main branch actually needs some more fixes on its codebase to make this PR in (configuration header files, config.py, etc - see Mbed-TLS/mbedtls#10090 for the full list).
I'm not sure I can make the CI fully happy in this PR.,,

valeriosetti · 2025-05-05T16:03:29Z

Since we agreed to manually merge this PR and then set Mbed-TLS/mbedtls#10090 on the merge queue immediately after that I removed the temporary commit 26fdaa1 which was added as attempt to make the CI fully happy.
We will rely on CI of 10090 to validate this PR.

gilles-peskine-arm

LGTM

component_tf_psa_crypto_test_platform_get_entropy_alt() was missing the _test_ part in order to be aligned with the standard naming form. Correct naming is component_tf_psa_crypto_test_platform_get_entropy_alt(). Signed-off-by: Valerio Setti <[email protected]>

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti · 2025-05-06T04:31:30Z

Sorry for the last minute fix, but since I had to modify the PR on the main repo to make its CI happy, I preferred to fix the name of the test component being added also in this PR (see new commit).

gilles-peskine-arm

LGTM at 1294fd2. The CI is still unhappy but that's expected: we are breaking TF-PSA-Crypto, and Mbed-TLS/mbedtls#10090 will fix it.

gilles-peskine-arm · 2025-05-06T14:40:34Z

configs/crypto-config-ccm-aes-sha256.h

@@ -24,5 +24,6 @@
 #define MBEDTLS_PSA_CRYPTO_C
 #define MBEDTLS_CTR_DRBG_C
 #define MBEDTLS_ENTROPY_C
+#define MBEDTLS_PLATFORM_C


This shouldn't be necessary. MBEDTLS_PLATFORM_C should only be needed to support runtime configuration of platform features. Neither MBEDTLS_PLATFORM_GET_ENTROPY_ALT nor !MBEDTLS_PLATFORM_GET_ENTROPY_ALT should require MBEDTLS_PLATFORM_C.

I think the natural way to fix this is to move the code from platform.c to platform_util.c.

At this point, given that we're planning to do a disruptive combined merge tonight, I propose to merge as is, and do a follow-up that does the move and removes the added enabling of MBEDTLS_PLATFORM_C in both TF-PSA-Crypto and mbedtls.

gilles-peskine-arm · 2025-05-06T16:28:19Z

As previously discussed, I'm going ahead and force-merging this pull request. This will break the CI on TF-PSA-Crypto where it consumes the head of mbedtls, to be fixed by merging Mbed-TLS/mbedtls#10090.

valeriosetti mentioned this pull request Mar 18, 2025

[framework] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 Mbed-TLS/mbedtls-framework#151

Merged

3 tasks

valeriosetti self-assigned this Mar 18, 2025

valeriosetti added size-s Estimated task size: small (~2d) needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon labels Mar 18, 2025

valeriosetti added this to Roadmap pull requests (new board) Mar 18, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Mar 18, 2025

gilles-peskine-arm requested changes Mar 19, 2025

View reviewed changes

valeriosetti force-pushed the issue9618-tfpsacrypto branch from 49842b8 to a5de9ab Compare March 24, 2025 12:46

valeriosetti requested a review from gilles-peskine-arm March 24, 2025 12:50

valeriosetti mentioned this pull request Mar 24, 2025

[development] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 Mbed-TLS/mbedtls#10090

Merged

6 tasks

valeriosetti force-pushed the issue9618-tfpsacrypto branch from a5de9ab to 17e588b Compare March 24, 2025 14:59

valeriosetti changed the title ~~[tf-psa-crypto] MBEDTLS_ENTROPY_HARDWARE_ALT in 4.0~~ [tf-psa-crypto] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 Mar 24, 2025

gabor-mezei-arm self-requested a review March 27, 2025 09:24

valeriosetti removed the needs-ci Needs to pass CI tests label Mar 27, 2025

bjwtaylor previously approved these changes Mar 27, 2025

View reviewed changes

valeriosetti dismissed bjwtaylor’s stale review via dfe34b3 March 31, 2025 10:23

valeriosetti force-pushed the issue9618-tfpsacrypto branch 2 times, most recently from dfe34b3 to 92e6f42 Compare April 1, 2025 07:33

valeriosetti requested a review from bjwtaylor April 1, 2025 07:34

valeriosetti force-pushed the issue9618-tfpsacrypto branch from 92e6f42 to fa5d3d3 Compare April 1, 2025 20:07

gabor-mezei-arm previously approved these changes Apr 2, 2025

View reviewed changes

bjwtaylor previously approved these changes Apr 4, 2025

View reviewed changes

valeriosetti dismissed stale reviews from bjwtaylor and gabor-mezei-arm via 0b4ca69 April 7, 2025 08:18

valeriosetti mentioned this pull request Apr 30, 2025

Entropy: remove empty definition of mbedtls_platform_entropy_poll() #273

Closed

valeriosetti force-pushed the issue9618-tfpsacrypto branch from ced4965 to 88c818c Compare April 30, 2025 09:51

valeriosetti force-pushed the issue9618-tfpsacrypto branch from 88c818c to 996bb07 Compare May 5, 2025 16:00

gilles-peskine-arm previously approved these changes May 5, 2025

View reviewed changes

valeriosetti added 2 commits May 6, 2025 06:26

framework: update reference

1294fd2

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti dismissed gilles-peskine-arm’s stale review via 1294fd2 May 6, 2025 04:26

valeriosetti force-pushed the issue9618-tfpsacrypto branch from 996bb07 to 1294fd2 Compare May 6, 2025 04:26

valeriosetti requested a review from gilles-peskine-arm May 6, 2025 04:31

gilles-peskine-arm approved these changes May 6, 2025

View reviewed changes

gilles-peskine-arm added needs-work needs-review Every commit must be reviewed by at least two team members and removed needs-review Every commit must be reviewed by at least two team members needs-work labels May 6, 2025

felixc-arm mentioned this pull request May 6, 2025

Move public legacy headers to /include/mbedtls #247

Merged

4 tasks

gilles-peskine-arm reviewed May 6, 2025

View reviewed changes

gilles-peskine-arm mentioned this pull request May 6, 2025

Don't require MBEDTLS_PLATFORM_C for entropy #282

Closed

bjwtaylor approved these changes May 6, 2025

View reviewed changes

gilles-peskine-arm merged commit 5ab6c9c into Mbed-TLS:development May 6, 2025
2 of 5 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) May 6, 2025

felixc-arm mentioned this pull request May 12, 2025

Privatize functions and some types/macros in cipher primitive headers #271

Merged

4 tasks

felixc-arm mentioned this pull request Jun 3, 2025

Remove requirment on MBEDTLS_PLATFORM_C for entropy #302

Merged

4 tasks

gilles-peskine-arm mentioned this pull request Jun 5, 2025

Allow builds with no RNG but entropy in an NV seed #307

Open

gilles-peskine-arm mentioned this pull request Jun 23, 2025

Simplify RNG options for TF-PSA-Crypto 1.0 #328

Open

		* \param[out] entropy_content Measure of the entropy content (in bits) of the
		* data written in the \p output buffer. The pointer

[tf-psa-crypto] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 #212

[tf-psa-crypto] MBEDTLS_PLATFORM_GET_ENTROPY_ALT in 4.0 #212

Uh oh!

Conversation

valeriosetti commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

CI note

PR checklist

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriosetti commented Apr 1, 2025

Uh oh!

valeriosetti commented Apr 1, 2025

Uh oh!

valeriosetti commented Apr 4, 2025

Uh oh!

gilles-peskine-arm commented Apr 30, 2025

Uh oh!

valeriosetti commented Apr 30, 2025

Uh oh!

valeriosetti commented Apr 30, 2025

Uh oh!

valeriosetti commented May 5, 2025

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

valeriosetti commented May 6, 2025

Uh oh!

gilles-peskine-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

valeriosetti commented Mar 18, 2025 •

edited

Loading

gilles-peskine-arm left a comment •

edited

Loading