[PATCH v3 03/25] mbedtls: add mbedtls into the build system

[Raymond Mao]

Hi Ilias,

On Thu, 30 May 2024 at 11:48, Ilias Apalodimas <ilias.apalodimas at linaro.org>
wrote:

> Hi both,
>
> [...]
>
> >> > > > > > >
> >> > > > > > > We need much more granularity here, and to re-think some
> existing
> >> > > > > > > symbols too perhaps. What we should be able to do is pick
> mbedTLS
> >> > > or
> >> > > > > > > "legacy SW implementation" or "HW implementation" for the
> various
> >> > > > > > > algorithms, and that in turn can have some higher level
> grouping
> >> > > to it.
> >> > > > > > > This should then negate a bunch of the Makefile work you're
> doing
> >> > > as we
> >> > > > > > > won't have CONFIG_SHA256 enabled as we'll have
> >> > > > > CONFIG_MBEDTLS_LIB_SHA256
> >> > > > > > > or whatever enabled.
> >> > > > > > >
> >> > > > > >
> >> > > > > > I think we should use CONFIG_MBEDTLS_LIB_[CRYPTO,X509,TLS] for
> >> > > high-level
> >> > > > > > grouping.
> >> > > > > > Underneath, the CONFIG_SHA[1,256,512] switches (and other
> crypto
> >> > > options)
> >> > > > > > can be
> >> > > > > > used as sub build options in both MbedTLS and "legacy libs".
> >> > > > > >
> >> > > > > > Take hash as an example, if the users prefer to use MbedTLS
> other
> >> > > than
> >> > > > > > "legacy libs" for
> >> > > > > > hash operation, CONFIG_MBEDTLS_LIB_CRYPTO should be defined
> as the
> >> > > main
> >> > > > > > switch
> >> > > > > > (the users can still prefer to use "legacy libs" for X509 by
> >> > > > > > keeping  CONFIG_MBEDTLS_LIB_X509
> >> > > > > > disabled).
> >> > > > > > Then enable the algorithms they need (e.g. CONFIG_SHA256) -
> the
> >> > > algorithm
> >> > > > > > options works
> >> > > > > > for both MbedTLS and "legacy libs".
> >> > > > > >
> >> > > > > > HW implementations with MbedTLS (aka, Alternative algorithms
> in
> >> > > MbedTLS)
> >> > > > > is
> >> > > > > > another
> >> > > > > > topic which is not covered in this patch set (It needs to
> migrate
> >> > > each
> >> > > > > > vendor's solution under
> >> > > > > > MbedTLS alternative algorithm).
> >> > > > > > Current patch set is focused on SW implementation with
> MbedTLS.
> >> > > > >
> >> > > > > The "easy" problem with what's in v3 is that X509 and CRYPTO are
> >> > > > > select'd under the main heading.
> >> > > >
> >> > > > Not sure if I get what you mentioned, currently all MbedTLS
> options are
> >> > > > under
> >> > > > Library routines > Security support
> >> > > > Do you think we should keep them in other places?
> >> > > >
> >> > > >
> >> > > > > The harder problem is that we
> >> > > > > intentionally have granularity for SHA256, SHA512, etc, etc and
> all of
> >> > > > > that goes away with the current Kconfig option if you select
> mbedTLS.
> >> > > We
> >> > > > > need to bring that back. And we shouldn't need to have all of
> the ifneq
> >> > > > > statements in Makefiles because both CONFIG_SHA256 and
> >> > > > > CONFIG_MBEDLTS_LIB_CRYPTO_SHA256 will not be true (Or possibly,
> >> > > > > CONFIG_SHA256 gates things U-Boot's internal API for sha256'ing
> >> > > > > something and CONFIG_LEGACY_SHA256 controls building
> lib/sha256.c).
> >> > > > >
> >> > > > > I think we should not introduce new ones like CONFIG_LEGACY_,
> >> > > > CONFIG_SHA[1,256,512] should be used no matter whether MbedTLS is
> >> > > > enabled or not.
> >> > > > I understand your concern, I will bring CONFIG_SHA[1,256,512]
> back when
> >> > > > MbedTLS is enabled. Those options should control the options in
> the
> >> > > MbedTLS
> >> > > > default config file.
> >> > >
> >> > > My concern is that we do not have the correct level of granularity,
> and
> >> > > that can partly be seen by the number of ifneq(...) statements being
> >> > > added around already conditional logic. We should have almost none
> of
> >> > > those, in the end, is what I'm saying. We have a mechanism for
> >> > > configuring the build, Kconfig, and that should drive the decisions
> as
> >> > > much as possible.
> >> >
> >> > The `ifneq(ONFIG_MBEDTLS_LIB_*)` statements are due to the fact that
> we
> >> > still
> >> > need lib/Makefile and lib/crypto/Makefile when building hash and x509
> >> > stuffs with
> >> > MbedTLS enabled.
> >> > To address this, I guess we have to first refactor all "legacy libs"
> that
> >> > will be replaced
> >> > by MbedTLS:
> >> > Move md5, sha* from lib to to a new dir lib/hash and move public_key,
> >> > rsapubkey*,
> >> > rsa_helper, x509*, pkcs7*,mscode* from lib/crypto to a new dir
> lib/x509.
> >> > When they are all independent modules with separated Makefile, we can
> remove
> >> > `ifneq(ONFIG_MBEDTLS_LIB_*)` and all can be driven in lib/Makefile.
> >> >
> >> > Is that something you expect?
> >> > If yes I can do this for v4, or put it into another
> prerequisite/refactor
> >> > series.
> >>
> >> We should not need to do that because we should not have CONFIG_SHA256
> >> set if we are not building lib/sha256.c at that stage, is what I'm
> >> saying. CONFIG_LEGACY_SHA256 should control it and CONFIG_SHA256 should
> >> control the API and CONFIG_MBEDTLS_LIB_CRYPTO_SHA256 should control the
> >> mbedTLS version, and this should either expand on, or if needed
> >> update/rework, the mechanism that lets us also have say
> >> CONFIG_ARMV8_CE_SHA256 for using that HW based version of the support.
> >>
> >
> > But in this case we have to introduce two new Kconfigs for each
> algorithm -
> > CONFIG_LEGACY_<alg> and CONFIG_MBEDTLS_LIB_CRYPTO_<alg>.
> > CONFIG_<alg> is still there, so we have totally 3 Kconfigs for one hash
> alg which
> > seems adding too much complexity.
>
> I haven't looked at your v4 yet, but I think Tom is on the right path here.
> Yes, it might be a bit more work, but in the long run, it's going to
> be easier to maintain both in Makefiles and code.
>
> I think Tom is saying
> The API should be gated by CONFIG_SHA1, CONFIG_SHA256 etc
> the individual implementation musty carry their own symbols e.g
> CONFIG_MBEDTLS_SHA256
> CONFIG_LEGACY_SHA256 etc.
>
> If any of those are selected you turn on CONFIG_SHA256
>
> But for example, in that case, users can pick sha1 from legacy lib but
sha256 from
MbedTLS by selecting both:
CONFIG_LEGACY_SHA1
CONFIG_MBEDTLS_LIB_CRYPTO_SHA256
This is not something we want.

The user should only allow completely switching to MbedTLS or stay with
legacy lib.
That is why I believe we need a high level switch CONFIG_MBEDTLS_LIB_CRYPTO.

Regards,
Raymond