Hi Philippe, On 2026-06-01T13:42:58, Philippe Reynes <[email protected]> wrote: > mbedtls: enable support of ecc > > Prepares the support of ecdsa signature check > using MbedTLS. It introduces the option ECDSA_MBEDTLS > to enable (or not) the support of ecdsa in MbedTLS. > > So now, ecdsa is no longer supported by default in the lib TLS. > To get it, the option ECDSA_MBEDTLS must be enabled. > > The option ECDSA_MBEDTLS only enables the ecdsa support > in MbedTLS, it doesn't provide any API to use it for > signature check. It will be done in another commit > by adding the option ECDSA_VERIFY_MBEDTLS. > > Two internal options are also added BIGNUM_MBEDTLS > and ECC_MBEDTLS. Those options are used to share code > between different option and avoid duplication. > > Reviewed-by: Raymond Mao <[email protected]> > Signed-off-by: Philippe Reynes <[email protected]> > [...] > > configs/sandbox_defconfig | 1 + > lib/ecdsa/Kconfig | 1 + > lib/mbedtls/Kconfig | 17 +++++++++++++++++ > lib/mbedtls/Makefile | 17 ++++++++++------- > lib/mbedtls/mbedtls_def_config.h | 41 > ++++++++++++++++++++++++++-------------- > 5 files changed, 56 insertions(+), 21 deletions(-)
I have some Kconfig nits mostly around SPL and this is v8 so I'm happy for them to go in as a follow-up if that is easier. Reviewed-by: Simon Glass <[email protected]> > diff --git a/lib/ecdsa/Kconfig b/lib/ecdsa/Kconfig > @@ -1,6 +1,7 @@ > config ECDSA > bool "Enable ECDSA support" > depends on DM > + select ASN1_DECODER This makes every ECDSA user carry the ASN1 decoder, including boards using a hardware implementation (e.g. the stm32mp ROM-API driver) or the libcrypto path, neither of which needs it. The requirement comes from the MbedTLS implementation only, so please can you put it on the MbedTLS side instead, e.g. have ECDSA_MBEDTLS select ASN1_DECODER (alongside its existing dependency on ASN1_DECODER_MBEDTLS)? Also this change is not mentioned in the commit message. > diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig > @@ -301,6 +317,7 @@ config MBEDTLS_LIB_TLS > depends on MBEDTLS_LIB > + select ECC_MBEDTLS Just to check, what happens to wget https when MBEDTLS_LIB_TLS is enabled but ECDSA_MBEDTLS is not? With this patch the ECDSA cipher suites and ECDSA certificate verification disappear from the TLS build, and a large fraction of public servers (e.g. Let's Encrypt) serve ECDSA certificates these days, so existing users may find https silently stops working for many sites. I understand the goal of making ECDSA optional for size reasons, but perhaps ECDSA_MBEDTLS should have 'default y if MBEDTLS_LIB_TLS', or the help text for MBEDTLS_LIB_TLS should mention ECDSA_MBEDTLS is needed for ECDSA certificates. What do you think? > diff --git a/lib/mbedtls/Makefile b/lib/mbedtls/Makefile > @@ -39,13 +39,21 @@ mbedtls_lib_crypto-$(CONFIG_$(PHASE_)HKDF_MBEDTLS) += \ > +mbedtls_lib_x509-$(CONFIG_$(PHASE_)BIGNUM_MBEDTLS) += \ > + $(MBEDTLS_LIB_DIR)/bignum.o \ > + $(MBEDTLS_LIB_DIR)/bignum_core.o > mbedtls_lib_x509-$(CONFIG_$(PHASE_)ASN1_DECODER_MBEDTLS) += \ > $(MBEDTLS_LIB_DIR)/asn1parse.o \ > $(MBEDTLS_LIB_DIR)/asn1write.o \ > $(MBEDTLS_LIB_DIR)/oid.o > mbedtls_lib_x509-$(CONFIG_$(PHASE_)RSA_PUBLIC_KEY_PARSER_MBEDTLS) += \ > - $(MBEDTLS_LIB_DIR)/bignum.o \ > - $(MBEDTLS_LIB_DIR)/bignum_core.o \ These rules use CONFIG_$(PHASE_) but BIGNUM_MBEDTLS and ECC_MBEDTLS have no SPL_ variants in Kconfig. In an SPL build CONFIG_$(PHASE_)BIGNUM_MBEDTLS expands to CONFIG_SPL_BIGNUM_MBEDTLS, which does not exist, so bignum.o is no longer built for SPL even when SPL_RSA_PUBLIC_KEY_PARSER_MBEDTLS is enabled - previously it came in via the RSA-parser rule, which does have an SPL symbol. The same applies to ecp.o and friends with SPL_MBEDTLS_LIB_TLS, which used to get them unconditionally from mbedtls_lib_tls-y. No in-tree defconfig enables these SPL options today, so it is latent, but please can you either add hidden SPL_ variants selected from the SPL options, or use CONFIG_$(XPL_) consistently with the proper-only symbols? > diff --git a/lib/mbedtls/mbedtls_def_config.h > b/lib/mbedtls/mbedtls_def_config.h > @@ -122,7 +144,9 @@ > /* ECDSA */ > #if CONFIG_IS_ENABLED(ASN1_DECODER) > +#if CONFIG_IS_ENABLED(ECDSA_MBEDTLS) > #define MBEDTLS_ECDSA_C > +#endif /* #if CONFIG_IS_ENABLED(ECDSA_MBEDTLS) */ > #define MBEDTLS_ECP_C > #define MBEDTLS_ECDH_C > #endif This define is redundant: the X509 section above already defines MBEDTLS_ECDSA_C whenever ECDSA_MBEDTLS is enabled, and MBEDTLS_LIB_TLS requires the X509 options, so this block can never add anything. Please can you drop it? While here, maybe move MBEDTLS_ECDSA_DETERMINISTIC inside the new ECDSA_MBEDTLS guard? Regards, Simon
