STM32: add otp_crypto with mbedTLS#2299
Conversation
|
LGTM! Amp found the following, pick and choose as always: PR Review —
|
| ID | Concern | Verdict |
|---|---|---|
| A | AVM_ABORT() on RNG init failure too harsh |
Valid — see fix 5 |
| B | HAL_RNG_DeInit unconditional in free |
Valid — see fix 4 |
| C | mbedtls_hardware_poll returns -1 |
Valid — see fix 3 |
| D | Seed string duplicated from generic_unix | Not borne out — STM32 uses "AtomVM STM32 Mbed-TLS initial seed.", generic_unix uses "AtomVM Mbed-TLS initial seed.". The string is a CTR_DRBG personalization, not a secret. Renaming the local from seed to personalization would be a minor clarity win. |
| E | F4/G0 RNG regex correctness | F410/F412 correctly enabled; F446 incorrectly enabled — see fix 2 |
| F | STM32_HAS_RNG propagates through add_subdirectory |
Yes, standard CMake directory-scope inheritance applies |
| G | MBEDTLS_USER_CONFIG_FILE defined on both library and consumer |
Both are needed: target defs control how mbedTLS is built, library def controls how AtomVM sources see feature macros (e.g. MBEDTLS_PKCS5_C, MBEDTLS_VERSION_C). Keep both. |
| H | platform_nifs.c precedence change |
Valid — see fix 1 |
Threading note
No current threading bug — STM32 disables SMP at the top level, so the lack of mutexes around mbedtls_entropy_context / mbedtls_ctr_drbg_context / RNG handle is acceptable today. If STM32 ever enables SMP, copy the locking pattern from generic_unix/rp2/esp32. Not a blocker.
Test coverage gaps
The current Renode stm32_crypto_test is a useful smoke test, but it only proves mbedTLS is linked and random bytes can be produced. It does not catch:
- wrong no-RNG device classification (would have caught the F446 issue with a build-matrix check)
- NIF precedence regressions
- deterministic crypto functionality
Suggested additions
One deterministic crypto assertion in test_crypto.erl:
%% deterministic crypto path
<<16#BA,16#78,16#16,16#BF,16#8F,16#01,16#CF,16#EA,
16#41,16#41,16#40,16#DE,16#5D,16#AE,16#22,16#23,
16#B0,16#03,16#61,16#A3,16#96,16#17,16#7A,16#9C,
16#B4,16#10,16#FF,16#61,16#F2,16#00,16#15,16#AD>> =
crypto:hash(sha256, <<"abc">>),Build-matrix check for representative parts (configure-time):
- Should have RNG:
stm32f410…,stm32f412…,stm32g041…,stm32g061…,stm32g081…,stm32g0c1… - Should not have RNG:
stm32f401…,stm32f411…,stm32f446…,stm32g071…,stm32g0b1…
A CI job that asserts the printed Has RNG : TRUE/FALSE line per device would have caught the F446 omission.
Recommended patch set summary
- Preserve NIF lookup semantics — register
otp_cryptoas a normal NIF collection (preferred), or at minimum keepnif_collection_resolve_niffirst inplatform_nifs.c. - Fix F4 regex — add
46to the no-RNG alternation; cite AN4230 in the comment. - Harden
sys.c—callocfor platform, addrng_is_initialized, conditionalHAL_RNG_DeInit, freeplatformandlocked_pins, returnMBEDTLS_ERR_ENTROPY_SOURCE_FAILED, prefer graceful degradation on RNG init failure. - Expand tests — one deterministic
crypto:hash/2assertion + a build-matrix RNG-detection check.
Effort: roughly 1–3 hours.
a1f0c1a to
b86212c
Compare
Signed-off-by: Paul Guyot <pguyot@kallisys.net>
petermm
left a comment
There was a problem hiding this comment.
The contributor addressed nearly all of our findings. New commit is 6e69109 on branch w19/stm32-mbedtls.
Addressed:
- ✅ NIF precedence (fix #1) — New otp_crypto_platform.c uses REGISTER_NIF_COLLECTION; platform_nifs.c reverted to original shape.
- ✅ F4 regex (fix #2) — Better than suggested: switched from blacklist to allowlist per AN4230. Universal-RNG families enumerated (f2/f7/g4/h5/h7/l4/u3/u5/wb), F4 RNG parts listed (05/15/07/17/10/12/13/23/27/37/29/39/69/79), G0 security line (41/61/81/c1). F446 now correctly excluded.
- ✅ MBEDTLS_ERR_ENTROPY_SOURCE_FAILED (fix #3) — applied in both error paths.
- ✅ Free platform_data + locked_pins (fix #4 partial) — added.
- ✅ Test coverage — added deterministic crypto:hash tests for md5/sha/sha224/sha256/sha384/sha512 across binary/string/iolist inputs plus badarg checks.
Not addressed (intentional):
- ❌ Graceful RNG degradation (fix #5) — kept fail-fast AVM_ABORT(). Comment now states "HAL_RNG_Init succeeded in stm32_rng_hw_init or we aborted", so HAL_RNG_DeInit remains unconditional and rng_is_initialized flag was not added. This is a deliberate policy choice — defensible given init runs once at boot.
- ❌ CI build-matrix check for RNG detection per device — not added (low priority since the regex was rewritten more rigorously).
Overall: the PR is in much better shape and the remaining items are policy choices, not bugs.
Merge fixes, features, and improvements from release-0.7, including: - Add UART support to RP2 platform (#2125) - Fix uart:open/1,2 and gpio:open/0, gpio:start/0 API (#2258) - Fix localtime/1 memory leak, use-after-free, and TZ restore bugs (#2291) - Fix failure of lldb test on macOS 26 runner (#2296) - Fix two close bugs in atomvm:subprocess/4 (#2297) - CI: fix esp32c61 flappiness (#2298) - STM32: add otp_crypto with mbedTLS (#2299) uart:open/1,2 and gpio:open/0, gpio:start/0 now raise on failure instead of returning {error, Reason}. Update any code that pattern-matched the error tuple.
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later