Hi Raymond, On Fri, 6 Sept 2024 at 08:50, Raymond Mao <raymond....@linaro.org> wrote: > > Hi Simon, > > On Thu, 5 Sept 2024 at 20:43, Simon Glass <s...@chromium.org> wrote: >> >> Hi Raymond, >> >> On Tue, 3 Sept 2024 at 08:59, Raymond Mao <raymond....@linaro.org> wrote: >> > >> > Hi Simon, >> > >> > On Sat, 17 Aug 2024 at 11:58, Simon Glass <s...@chromium.org> wrote: >> >> >> >> Hi Raymond, >> >> >> >> On Fri, 16 Aug 2024 at 15:44, Raymond Mao <raymond....@linaro.org> wrote: >> >> > >> >> > Integrate MbedTLS v3.6 LTS (currently v3.6.0) with U-Boot. >> >> > >> >> > Motivations: >> >> > ------------ >> >> > >> >> > 1. MbedTLS is well maintained with LTS versions. >> >> > 2. LWIP is integrated with MbedTLS and easily to enable HTTPS. >> >> > 3. MbedTLS recently switched license back to GPLv2. >> >> > >> >> > Prerequisite: >> >> > ------------- >> >> > >> >> > This patch series requires mbedtls git repo to be added as a >> >> > subtree to the main U-Boot repo via: >> >> > $ git subtree add --prefix lib/mbedtls/external/mbedtls \ >> >> > https://github.com/Mbed-TLS/mbedtls.git \ >> >> > v3.6.0 --squash >> >> > Moreover, due to the Windows-style files from mbedtls git repo, >> >> > we need to convert the CRLF endings to LF and do a commit manually: >> >> > $ git add --renormalize . >> >> > $ git commit >> >> > >> >> > New Kconfig options: >> >> > -------------------- >> >> > >> >> > `MBEDTLS_LIB` is for MbedTLS general switch. >> >> > `MBEDTLS_LIB_CRYPTO` is for replacing original digest and crypto libs >> >> > with >> >> > MbedTLS. >> >> > `MBEDTLS_LIB_X509` is for replacing original X509, PKCS7, MSCode, ASN1, >> >> > and Pubkey parser with MbedTLS. >> >> > `LEGACY_CRYPTO` is introduced as a main switch for legacy crypto >> >> > library. >> >> > `LEGACY_CRYPTO_BASIC` is for the basic crypto functionalities and >> >> > `LEGACY_CRYPTO_CERT` is for the certificate related functionalities. >> >> > For each of the algorithm, a pair of `<alg>_LEGACY` and `<alg>_MBEDTLS` >> >> > Kconfig options are introduced. Meanwhile, `SPL_` Kconfig options are >> >> > introduced. >> >> > >> >> > In this patch set, MBEDTLS_LIB, MBEDTLS_LIB_CRYPTO and MBEDTLS_LIB_X509 >> >> > are by default enabled in qemu_arm64_defconfig and sandbox_defconfig >> >> > for testing purpose. >> >> > >> >> > Patches for external MbedTLS project: >> >> > ------------------------------------- >> >> > >> >> > Since U-Boot uses Microsoft Authentication Code to verify PE/COFFs >> >> > executables which is not supported by MbedTLS at the moment, >> >> > addtional patches for MbedTLS are created to adapt with the EFI loader: >> >> > 1. Decoding of Microsoft Authentication Code. >> >> > 2. Decoding of PKCS#9 Authenticate Attributes. >> >> > 3. Extending MbedTLS PKCS#7 lib to support multiple signer's >> >> > certificates. >> >> > 4. MbedTLS native test suites for PKCS#7 signer's info. >> >> > >> >> > All above 4 patches (tagged with `mbedtls/external`) are submitted to >> >> > MbedTLS project and being reviewed, eventually they should be part of >> >> > MbedTLS LTS release. >> >> > But before that, please merge them into U-Boot, otherwise the building >> >> > will be broken when MBEDTLS_LIB_X509 is enabled. >> >> > >> >> > See below PR link for the reference: >> >> > https://github.com/Mbed-TLS/mbedtls/pull/9001 >> >> > >> >> > Miscellaneous: >> >> > -------------- >> >> > >> >> > Optimized MbedTLS library size by tailoring the config file >> >> > and disabling all unnecessary features for EFI loader. >> >> > From v2, original libs (rsa, asn1_decoder, rsa_helper, md5, sha1, >> >> > sha256, >> >> > sha512) are completely replaced when MbedTLS is enabled. >> >> > From v3, the size-growth is slightly reduced by refactoring Hash >> >> > functions. >> >> > From v6, smaller implementations for SHA256 and SHA512 are enabled and >> >> > target size reduce significantly. >> >> > Target(QEMU arm64) size-growth when enabling MbedTLS: >> >> > v1: 6.03% >> >> > v2: 4.66% >> >> > v3 - v5: 4.55% >> >> > v6: 2.90% >> >> > >> >> > Please see the latest output from buildman for size-growth on QEMU >> >> > arm64, >> >> > Sandbox and Nanopi A64. [1] >> >> > >> >> > Tests done: >> >> > ----------- >> >> > >> >> > EFI Secure Boot test (EFI variables loading and verifying, EFI signed >> >> > image >> >> > verifying and booting) via U-Boot console. >> >> > EFI Secure Boot and Capsule sandbox test passed. >> >> > >> >> > Known issues: >> >> > ------------- >> >> > >> >> > None. >> >> >> >> I wonder if we could leave out the SHA stuff? The algorithms are >> >> stable and this would seem to avoid much of the size growth, and all >> >> the pain of trying to integrate another yet another hashing layer (we >> >> already have normal, progressive and h/w acceleration, plus >> >> UCLASS_HASH which h/w acceleration should use but that migration never >> >> happened). I struggle to see any benefit in replacing U-Boot's very >> >> solid hashing infra with something else, particularly as this series >> >> adds yet another. Better to invest the time to refactor it. I asked >> >> about this before and was told that it would happen 'later'. Let's >> >> just not change it at all, then it is more likely someone will sort it >> >> out. >> >> >> > Unfortunately, MbedTLS depends on its own digest layer. Unless we patch >> > MbedTLS >> > to allow an external digest library from U-Boot ... >> >> Yes that sounds best. It looks like only a few call sites, so it >> should be a matter of leaving out the MbedTLS code and adding some >> static inlines. >> > Inspired by Ilias's reply to patch #7, though we can use the MbedTLS hash > alternative options, > we still need to convert all U-Boot hash APIs to adapt to the MbedTLS style. > This will impact all callers in U-Boot and I don't think it worth to do, at > least now.
Agreed. > As the first patch set to introduce MbedTLS to U-Boot with turning on all > necessary features, > I think this patch set is in the best way with an overall consideration. I am not convinced, sorry. Can you update MbedTLS so that its hash algo can be changed to call the U-Boot one? Then we can deal with hardware acceleration, driver model and avoid yet another layer of cruft in common/hash.c I see only a few calls...and the hash algos are so simple and stable that there really is no value to U-Boot of all of this pain. Alternatively, if for some reason you really want this series in as is, if you have a plan and scheduling to tidy this up immediately after this series, I could be convinced to look the other way. Regards, Simon
