Hi Ilias and Tom,
On Thu, 30 May 2024 at 16:17, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Tom
On Wed, 29 May 2024 at 22:47, Tom Rini trini@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@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@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@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