[PATCH v6 00/28] Integrate MbedTLS v3.6 LTS with U-Boot

[Simon Glass]

Hi Raymond,

On Fri, 6 Sept 2024 at 08:50, Raymond Mao <raymond.mao at linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 5 Sept 2024 at 20:43, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Raymond,
>>
>> On Tue, 3 Sept 2024 at 08:59, Raymond Mao <raymond.mao at linaro.org> wrote:
>> >
>> > Hi Simon,
>> >
>> > On Sat, 17 Aug 2024 at 11:58, Simon Glass <sjg at chromium.org> wrote:
>> >>
>> >> Hi Raymond,
>> >>
>> >> On Fri, 16 Aug 2024 at 15:44, Raymond Mao <raymond.mao at 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