Re: [PATCH v4 04/29] lib: Adapt digest header files to MbedTLS

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6623 bytes --]

On Tue, Jul 02, 2024 at 08:02:37PM -0400, Raymond Mao wrote:
> Hi Tom,
> 
> On Tue, 2 Jul 2024 at 18:48, Tom Rini <trini@konsulko.com> wrote:
> 
> > On Tue, Jul 02, 2024 at 11:22:40AM -0700, Raymond Mao wrote:
> >
> > > Adapt digest header files to support both original libs and MbedTLS
> > > by switching on/off MBEDTLS_LIB_CRYPTO.
> > > Introduce <alg>_LEGACY kconfig for legacy hash implementations.
> > [snip]
> > > diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig
> > > index 3e9057f1acf..6662a9d20f1 100644
> > > --- a/lib/mbedtls/Kconfig
> > > +++ b/lib/mbedtls/Kconfig
> > > @@ -21,9 +21,105 @@ if LEGACY_CRYPTO
> > >
> > >  config LEGACY_CRYPTO_BASIC
> > >       bool "legacy basic crypto libraries"
> > > +     select MD5_LEGACY if MD5
> > > +     select SHA1_LEGACY if SHA1
> > > +     select SHA256_LEGACY if SHA256
> > > +     select SHA512_LEGACY if SHA512
> > > +     select SHA384_LEGACY if SHA384
> > > +     select SPL_MD5_LEGACY if MD5 && SPL
> > > +     select SPL_SHA1_LEGACY if SHA1 && SPL
> > > +     select SPL_SHA256_LEGACY if SHA256 && SPL
> > > +     select SPL_SHA512_LEGACY if SHA512 && SPL
> > > +     select SPL_SHA384_LEGACY if SHA384 && SPL
> > >       help
> > >         Enable legacy basic crypto libraries.
> > >
> > > +if LEGACY_CRYPTO_BASIC
> > > +
> > > +config SHA1_LEGACY
> > > +     bool "Enable SHA1 support with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SHA1
> > > +     help
> > > +       This option enables support of hashing using SHA1 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SHA256_LEGACY
> > > +     bool "Enable SHA256 support with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SHA256
> > > +     help
> > > +       This option enables support of hashing using SHA256 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SHA512_LEGACY
> > > +     bool "Enable SHA512 support with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SHA512
> > > +     default y if TI_SECURE_DEVICE && FIT_SIGNATURE
> > > +     help
> > > +       This option enables support of hashing using SHA512 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SHA384_LEGACY
> > > +     bool "Enable SHA384 support with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SHA384
> > > +     select SHA512_LEGACY
> > > +     help
> > > +       This option enables support of hashing using SHA384 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config MD5_LEGACY
> > > +     bool "Enable MD5 support with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && MD5
> > > +     help
> > > +       This option enables support of hashing using MD5 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +if SPL
> > > +
> > > +config SPL_SHA1_LEGACY
> > > +     bool "Enable SHA1 support in SPL with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SPL_SHA1
> > > +     default y if SHA1 && LEGACY_CRYPTO_BASIC
> > > +     help
> > > +       This option enables support of hashing using SHA1 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SPL_SHA256_LEGACY
> > > +     bool "Enable SHA256 support in SPL with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SPL_SHA256
> > > +     default y if SHA256 && LEGACY_CRYPTO_BASIC
> > > +     help
> > > +       This option enables support of hashing using SHA256 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SPL_SHA512_LEGACY
> > > +     bool "Enable SHA512 support in SPL with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SPL_SHA512
> > > +     default y if SHA512 && LEGACY_CRYPTO_BASIC
> > > +     help
> > > +       This option enables support of hashing using SHA512 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SPL_SHA384_LEGACY
> > > +     bool "Enable SHA384 support in SPL with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SPL_SHA384
> > > +     default y if SHA384 && LEGACY_CRYPTO_BASIC
> > > +     select SPL_SHA512
> > > +     help
> > > +       This option enables support of hashing using SHA384 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +config SPL_MD5_LEGACY
> > > +     bool "Enable MD5 support in SPL with legacy crypto library"
> > > +     depends on LEGACY_CRYPTO_BASIC && SPL_MD5
> > > +     default y if MD5 && LEGACY_CRYPTO_BASIC
> > > +     help
> > > +       This option enables support of hashing using MD5 algorithm
> > > +       with legacy crypto library.
> > > +
> > > +endif # SPL
> > > +
> > > +endif # LEGACY_CRYPTO_BASIC
> > > +
> > >  config LEGACY_CRYPTO_CERT
> > >       bool "legacy certificate libraries"
> > >       help
> >
> > This is all certainly moving in the right direction, but there's
> > dependency issues:
> >    aarch64:  w+   xilinx_zynqmp_kria
> > +(xilinx_zynqmp_kria)
> > +(xilinx_zynqmp_kria) WARNING: unmet direct dependencies detected for
> > SPL_MD5_LEGACY
> > +(xilinx_zynqmp_kria)   Depends on [n]: LEGACY_CRYPTO [=y] && SPL [=y] &&
> > LEGACY_CRYPTO_BASIC [=y] && SPL_MD5 [=n]
> > +(xilinx_zynqmp_kria)   Selected by [y]:
> > +(xilinx_zynqmp_kria)   - LEGACY_CRYPTO_BASIC [=y] && LEGACY_CRYPTO [=y]
> > && MD5 [=y] && SPL [=y]
> >
> 
> I am a bit confused by SPL_MD5, why it is [n] when both MD5 and SPL are [y].
> Of course we can replace 'SPL_MD5' with 'MD5 && SPL' to solve the warning,
> but I guess the wrong dependence is on SPL_MD5 but not the new added ones.
> ```
> config SPL_MD5
> bool "Support MD5 algorithm in SPL"
> depends on SPL
> ```
> I think it should be 'default y if SPL && MD5' instead of 'depends on SPL'.
> 
> Similarly SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is not selected when both
> ASYMMETRIC_PUBLIC_KEY_SUBTYPE
> and SPL are selected;
> SPL_ASN1_DECODER is not selected when both ASN1_DECODER and SPL are
> selected.
> 
> I think generally for each algorithm, 'SPL_<ALG>' config should be selected
> when both 'SPL' and '<ALG>' are selected.
> 
> If you agree I can fix this in the next patch set.

Kconfig error messages are a bit difficult to understand at times, yes.
Please figure out what exactly the depends on / default y combinations
should be such that (a) there's no functional changes, and buildman size
comparison builds are good for that and (b) no warnings like this are
found.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]