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

[Raymond Mao]

Hi Ilias and Tom,

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

> Hi Tom
>
> On Wed, 29 May 2024 at 22:47, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, May 29, 2024 at 03:42:04PM -0400, Raymond Mao wrote:
> > > Hi Tom,
> > >
> > > On Wed, 29 May 2024 at 14:43, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > > On Wed, May 29, 2024 at 02:38:10PM -0400, Raymond Mao wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Wed, 29 May 2024 at 14:01, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > > On Wed, May 29, 2024 at 01:42:16PM -0400, Raymond Mao wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Wed, 29 May 2024 at 12:58, Tom Rini <trini at konsulko.com>
> wrote:
> > > > > > >
> > > > > > > > On Tue, May 28, 2024 at 07:09:14AM -0700, Raymond Mao wrote:
> > > > > > > >
> > > > > > > > > Port mbedtls with dummy libc header files.
> > > > > > > > > Add mbedtls default config header file.
> > > > > > > > > Optimize mbedtls default config by disabling unused
> features to
> > > > > > > > > reduce the target size.
> > > > > > > > > Add mbedtls kbuild makefile.
> > > > > > > > > Add Kconfig and mbedtls config submenu.
> > > > > > > > [snip]
> > > > > > > > > diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig
> > > > > > > > > new file mode 100644
> > > > > > > > > index 00000000000..d6e77d56871
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/lib/mbedtls/Kconfig
> > > > > > > > > @@ -0,0 +1,25 @@
> > > > > > > > > +menuconfig MBEDTLS_LIB
> > > > > > > > > +     bool "Use mbedtls libraries"
> > > > > > > > > +     select MBEDTLS_LIB_CRYPTO
> > > > > > > > > +     select MBEDTLS_LIB_X509
> > > > > > > > > +     help
> > > > > > > > > +       Enable mbedtls libraries
> > > > > > > > > +
> > > > > > > > > +if MBEDTLS_LIB
> > > > > > > > > +
> > > > > > > > > +config MBEDTLS_LIB_CRYPTO
> > > > > > > > > +     bool "Crypto library"
> > > > > > > > > +     help
> > > > > > > > > +       Enable mbedtls crypto library
> > > > > > > > > +
> > > > > > > > > +config MBEDTLS_LIB_X509
> > > > > > > > > +     bool "X509 library"
> > > > > > > > > +     help
> > > > > > > > > +       Enable mbedtls X509 library
> > > > > > > > > +
> > > > > > > > > +config MBEDTLS_LIB_TLS
> > > > > > > > > +     bool "TLS library"
> > > > > > > > > +     help
> > > > > > > > > +       Enable mbedtls TLS library
> > > > > > > > > +
> > > > > > > > > +endif # MBEDTLS_LIB
> > > > > > > >
> > > > > > > > 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.
> >
>
> Just trying to make sure we are all on the same page here.
> Your main concerns are that the configuration options are not on par,
> we lack granularity, and it's currently confusing to turn the feature
> on since we have to enable both CONFIG_SHA256 &&
> CONFIG_MBEDTLS_LIB_CRYPTO. This also leads to makefiles being a bit
> weird-looking as well. Correct?
>
> The suggestion is that we add 3 symbols overall (again only using
> sha256 as an example, this applies to all)
> CONFIG_LEGACY_SHA256 -> This is the renamed CONFIG_SHA256
> CONFIG_MBEDTLS_SHA256 -> new symbol and has to map the existing options
> CONFIG_SHA256 -> This gets selected if either of the above is on and
> we use it to control the API.
>
> > 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.
>
> I am fine with all the above. This is the part that confused me. If we
> end up with the symbols above, we will need,
>
> obj-CONFIG_LEGACY$(SPL_)SHA256 -> compiles the existing code
> obj-CONFIG_MBEDTLS$(SPL_)SHA256 -> compiles the new mbedTLS variant,
> which now goes into a new .c file instead of an ifdef in the existing
> lib/sha256.c
>
> Did I miss anything?
>
>
Moreover, to prevent user selecting algorithms mixed up from MbedTLS and
the legacy
ones (e.g. sha1 from legacy lib but sha256 from MbedTLS):

When any of 'CONFIG_LEGACY_<alg>' is ON, CONFIG_LEGACY_CRYPTO is
selected automatically.
When each of 'CONFIG_ MBEDTLS  _<alg>' is ON, CONFIG_MBEDTLS_LIB_CRYPTO
is selected automatically.
Mark CONFIG_LEGACY_CRYPTO and CONFIG_MBEDTLS_LIB_CRYPTO as
mutually exclusive.

Is this correct?

Regards,
Raymond