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

[Ilias Apalodimas]

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

Thanks
/Ilias

>
> Moreover, this means allowing the user to select algorithms mix up with MbedTLS
> and legacy ones, for example, user can select both CONFIG_LEGACY_SHA1
> and CONFIG_MBEDTLS_LIB_CRYPTO_SHA256 at the same time.
> It means to build both legacy lib and MbedTLS which does not make sense.
>
> Why we don't have the CONFIG_MBEDTLS_LIB_CRYPTO as the higher level
> switch and keep CONFIG_<alg> as it was to select the algorithms?
> We still have the same granularity with this I think - and yes I will put CONFIG_<alg>
> in the MbedTLS config file so that it can control only the selected algorithms to be
> built.
> The only downside I can see here is adding a few `ifneq(ONFIG_MBEDTLS_LIB_*)`
> in lib/Makefile and lib/crypto/Makefile, but all the rest of things are straightforward and
> clean.
>
> Thanks.
> Regards,
> Raymond