[PATCH v8 03/27] lib: Adapt digest header files to MbedTLS

Raymond Mao raymond.mao at linaro.org
Fri Oct 11 20:25:20 CEST 2024


Hi Tom,

On Wed, 9 Oct 2024 at 13:52, Tom Rini <trini at konsulko.com> wrote:

> On Wed, Oct 09, 2024 at 08:32:29PM +0300, Ilias Apalodimas wrote:
> > On Wed, 9 Oct 2024 at 18:32, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >
> > > Hi Tom,
> > >
> > > On Wed, 9 Oct 2024 at 10:38, Tom Rini <trini at konsulko.com> wrote:
> > >>
> > >> On Thu, Oct 03, 2024 at 02:50:16PM -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.
> > >> > sha256.o should depend on SHA256 kconfig only but not
> SUPPORT_EMMC_RPMB,
> > >> > SHA256 should be selected when SUPPORT_EMMC_RPMB is enabled instead.
> > >> >
> > >> > `IS_ENABLED` or `CONFIG_IS_ENABLED` is not applicable here, since
> > >> > including <linux/kconfig.h> causes undefined reference on schedule()
> > >> > with sandbox build, as <linux/kconfig.h> includes
> <generated/autoconf.h>
> > >> > which enables `CONFIG_HW_WATCHDOG` and `CONFIG_WATCHDOG` but no
> schedule()
> > >> > are defined in sandbox build,
> > >> > Thus we use `#if defined(CONFIG_MBEDTLS_LIB_CRYPTO)` instead.
> > >> >
> > >> > Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
> > >> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > >>
> > >> There's three platforms where we see something like:
> > >>        arm: (for 1/1 boards) all +5651.0 data +112.0 rodata +139.0
> text +5400.0
> > >>             o4-imx6ull-nano: all +5651 data +112 rodata +139 text
> +5400
> > >>                u-boot: add: 23/0, grow: 1/0 bytes: 1172/0 (1172)
> > >>                  function                                   old
>  new   delta
> > >>                  hash_command                               108
>  296    +188
> > >>                  sha1_finish                                  -
>  156    +156
> > >>                  static.sha1_update                           -
>  114    +114
> > >>                  hash_algo                                    -
>  112    +112
> > >>                  sha1_padding                                 -
> 64     +64
> > >>                  hash_lookup_algo                             -
> 60     +60
> > >>                  sha1_starts                                  -
> 52     +52
> > >>                  crc16_ccitt_wd_buf                           -
> 36     +36
> > >>                  sha256_csum_wd                               -
> 34     +34
> > >>                  sha1_csum_wd                                 -
> 34     +34
> > >>                  hash_finish_sha256                           -
> 34     +34
> > >>                  hash_finish_sha1                             -
> 34     +34
> > >>                  crc32_wd_buf                                 -
> 34     +34
> > >>                  hash_finish_crc32                            -
> 28     +28
> > >>                  hash_finish_crc16_ccitt                      -
> 28     +28
> > >>                  hash_init_sha256                             -
> 22     +22
> > >>                  hash_init_sha1                               -
> 22     +22
> > >>                  hash_update_crc32                            -
> 20     +20
> > >>                  hash_update_crc16_ccitt                      -
> 20     +20
> > >>                  hash_init_crc32                              -
> 20     +20
> > >>                  hash_init_crc16_ccitt                        -
> 20     +20
> > >>                  hash_update_sha256                           -
> 16     +16
> > >>                  hash_update_sha1                             -
> 16     +16
> > >>                  sha1_update                                  -
>  8      +8
> > >>
> > >> This is because:
> > >> > diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> > >> > index 982e84dc3bc..5d7fd904950 100644
> > >> > --- a/drivers/mmc/Kconfig
> > >> > +++ b/drivers/mmc/Kconfig
> > >> > @@ -119,6 +119,7 @@ config MMC_HW_PARTITIONING
> > >> >  config SUPPORT_EMMC_RPMB
> > >> >       bool "Support eMMC replay protected memory block (RPMB)"
> > >> >       imply CMD_MMC_RPMB
> > >> > +     select SHA256
> > >> >       help
> > >> >         Enable support for reading, writing and programming the
> > >> >         key for the Replay Protection Memory Block partition in
> eMMC.
> > >>
> > >> Wasn't true / required before now, no hashing algorithms were enabled.
> > >> This was fine because:
> > >> [snip]
> > >> > diff --git a/lib/Makefile b/lib/Makefile
> > >> > index c4950b78a29..33755778283 100644
> > >> > --- a/lib/Makefile
> > >> > +++ b/lib/Makefile
> > >> > @@ -50,7 +50,6 @@ obj-$(CONFIG_XXHASH) += xxhash.o
> > >> >  obj-y += net_utils.o
> > >> >  obj-$(CONFIG_PHYSMEM) += physmem.o
> > >> >  obj-y += rc4.o
> > >> > -obj-$(CONFIG_SUPPORT_EMMC_RPMB) += sha256.o
> > >> >  obj-$(CONFIG_RBTREE) += rbtree.o
> > >> >  obj-$(CONFIG_BITREVERSE) += bitrev.o
> > >> >  obj-y += list_sort.o
> > >>
> > >> Got us the library access without bringing in everything else. And
> since
> > >> two of the platforms that are hitting this now are "nano" this is an
> > >> important thing to figure out how to continue to support. If there's
> > >> just no way around it, we can likely live with the size increase, but
> > >> I'd like to see this looked in to specifically first, thanks!
> > >>
> > > To address this, I think there are two options.
> > > 1. Introduce SUPPORT_EMMC_RPMB into the MbedTLS sub makefile.
> > > 2. Make MBEDTLS_LIB_CRYPTO depends on !SUPPORT_EMMC_RPMB.
> > > 1) looks to be ugly, I prefer 2) if you agree.
> >
> > 2 is not a good idea either. We can't just drop RPMB support when
> > mbedTLS is enabled
>
> To be clear, while I hope we can do something about this growth, I would
> rather live with it (as it's not an unreasonable amount) than do 2, and
> if 1 is too ugly, probably skip that as well. If it's not a matter of
> loosening some select statements, or maybe introducing a library type
> symbol we can see if anyone else more motivated has a better idea as
> it's literally 3 platforms (ev-imx280-nano-x-mb is the other nano, and
> then uniphier_v8 where it's arguably a missing feature anyhow) rather
> than a large number of them. And not even other "mini" or "nano"
> configs.
>
> Yes. I figured out what the problem is.
The "select SHA256" change doesn't matter, but size growth was introduced by
the inline function in sha1_alt.h I added in patch #2 of v8.
I already fixed this in my working branch:

   aarch64: (for 1/1 boards) all +8502.0 data +224.0 rodata +218.0 text
+8060.0
            uniphier_v8    : all +8502 data +224 rodata +218 text +8060
       arm: (for 2/2 boards) all +5649.0 data +112.0 rodata +137.0 text
+5400.0
            o4-imx6ull-nano: all +5651 data +112 rodata +139 text +5400
            ev-imx280-nano-x-mb: all +5647 data +112 rodata +135 text +5400

Now there is no "u-boot: add" any more on these three boards.
I will update v9 with this fix, and we don't need either 1) or 2) options
mentioned
in my previous reply.

Regards,
Raymond


More information about the U-Boot mailing list