Hi Ilias, On Thu, 30 May 2024 at 11:48, Ilias Apalodimas <ilias.apalodi...@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
