Re: [Buildroot] [PATCH 1/1] boot/arm-trusted-firmware: add trusted boot option

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Javad,

Do you have any feedback on the below questions?

Thanks!

Thomas

On Wed, 10 Apr 2024 21:28:34 +0200
Thomas Petazzoni via buildroot <buildroot@buildroot.org> wrote:

> Hello Javad,
> 
> On Thu, 28 Mar 2024 19:12:47 +0100
> Javad Rahimipetroudi via buildroot <buildroot@buildroot.org> wrote:
> 
> > This patch adds the required fields to enable Trusted Board Boot in
> > TF-A. The users should provide ROT_KEY private key to build the TF-A in
> > this mode. The ROT_KEY is used to sign the FIP image during the TF-A
> > build. Furthermore, the source code of the mbedTLS is also used during
> > the build process.
> > 
> > Signed-off-by: Javad Rahimipetroudi <javad.rahimipetroudi@mind.be>  
> 
> Thanks for this contribution! It looks good, I only have one
> doubt/issue with it.
> 
> > +ifeq ($(BR2_TARGET_ARM_TRUSTED_FIRMWARE_TRUSTED_BOOT),y)
> > +ARM_TRUSTED_FIRMWARE_TRUSTED_BOOT_ROT_KEY = $(call qstrip,$(BR2_TARGET_ARM_TRUSTED_FIRMWARE_ROT_KEY))
> > +ARM_TRUSTED_FIRMWARE_MAKE_OPTS += \
> > +	TRUSTED_BOARD_BOOT=1 \
> > +	MBEDTLS_DIR=$(MBEDTLS_SRCDIR) \  
> 
> This re-use of the mbedtls source code, outside of the mbedtls package
> build itself sounded a bit suspicious to me. Indeed, mbedtls being a
> dependency of arm-trusted-firmware, it means that 
> $(MBEDTLS_SRCDIR) will contain an already built mbedtls. Would this be
> a problem?
> 
> Looking at the arm-trusted-firmware build logic, it looks like it
> isn't: the TF-A build system will rebuild in its own folder the mbedtls
> library. However, when I see:
> 
> LIBMBEDTLS_SRCS         += $(addprefix ${MBEDTLS_DIR}/library/,         \
>                                         aes.c                           \
>                                         asn1parse.c                     \
>                                         asn1write.c                     \
>                                         cipher.c                        \
>                                         cipher_wrap.c                   \
>                                         constant_time.c                 \
>                                         hash_info.c                     \
>                                         memory_buffer_alloc.c           \
>                                         oid.c                           \
>                                         platform.c                      \
>                                         platform_util.c                 \
>                                         bignum.c                        \
>                                         bignum_core.c                   \
>                                         gcm.c                           \
>                                         md.c                            \
>                                         pk.c                            \
>                                         pk_wrap.c                       \
>                                         pkparse.c                       \
>                                         pkwrite.c                       \
>                                         sha256.c                        \
>                                         sha512.c                        \
>                                         ecdsa.c                         \
>                                         ecp_curves.c                    \
>                                         ecp.c                           \
>                                         rsa.c                           \
>                                         rsa_alt_helpers.c               \
>                                         x509.c                          \
>                                         x509_crt.c                      \
>                                         )
> 
> in the TF-A build system, I'm a bit scared, because it means that there
> is a pretty tight coupling between the version of TF-A and the version
> of mbedtls. If we update mbedtls to a newer version which has an
> additional source file... TF-A would have to be updated accordingly.
> This looks a bit "meh" to me.
> 
> However, I don't really have a super great alternative to offer. The
> only alternative that I can think of is to have
> boot/arm-trusted-firmware/ download/extract its own copy of mbedtls, so
> that (1) we control its version independently of the mbedtls package
> and (2) we don't poke into the mbedtls source directory.
> 
> Let's see what the other maintainers think of this somewhat special
> situation.
> 
> Thomas



-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot