Re: [PATCH 02/13] firmware: add function to verify next image

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Mar 10, 2025 at 07:37:33PM +0100, Marco Felsch wrote:
> On 25-02-28, Sascha Hauer wrote:
> > Some SoCs use a startup sequence that includes multiple stages where a
> > full barebox is loaded by an early small barebox that fits into the
> > SoC's SRAM. This is commonly referred to as xload. In a secure boot
> > environment it's necessary to load only trusted barebox images. One
> > way to accomplish this is to compile a sha256 into the first stage
> > barebox and to verify the full barebox against this hash.
> > 
> > This patch adds the generic parts for this. The full barebox binary
> > can be put into the first stage build as a firmware file. The firmware
> > itself won't be used, only the hash is compiled into the image. SoC
> > code can then check the full barebox image against the hash. As this
> > requires SoC code to check the hash, the option is hidden behind
> > CONFIG_HAVE_FIRMWARE_VERIFY_NEXT_IMAGE. SoC code can select this option
> > when it implements the required hash checking.
> > 
> > It's worth noting that using a hash for verification has one advantage
> > over cryptographicaly signing followup images: It ties first stage
> > and full barebox stages together effectively avoiding mix-and-match
> > attacks.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  firmware/Kconfig   | 23 +++++++++++++++++++++++
> >  firmware/Makefile  |  2 ++
> >  include/firmware.h | 28 ++++++++++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> > 
> > diff --git a/firmware/Kconfig b/firmware/Kconfig
> > index ba005976c5..bdb71321bc 100644
> > --- a/firmware/Kconfig
> > +++ b/firmware/Kconfig
> > @@ -108,4 +108,27 @@ config FIRMWARE_LS1028A_ATF
> >  config FIRMWARE_LS1046A_ATF
> >  	bool
> >  
> > +config HAVE_FIRMWARE_VERIFY_NEXT_IMAGE
> > +	bool
> > +
> > +config FIRMWARE_VERIFY_NEXT_IMAGE
> > +	depends on HAVE_FIRMWARE_VERIFY_NEXT_IMAGE
> > +	bool "verify next image to load"
> > +	help
> > +	  The boot process of some SoCs uses multiple stages where the first stage is
> > +	  a stripped down barebox loaded by the SoC's ROM and the next state is a full
> > +	  barebox loaded by the first stage. In a trusted boot scenario the next stage
> > +	  has to be verified by the first stage,
> > +
> > +	  This option allows to specify the next image to be loaded. Put the next stage
> > +	  image to firmware/next-image.bin. The image itself is not used, but a sha256
> > +	  hash of the image will be generated and compiled into the first stage which
> > +	  can be used to verify the next stage.
> > +
> > +	  Note that this option only enabled generation of the sha256 hash. Loading and
> > +	  starting the next stage is highly SoC dependent and it's the SoC code's
> > +	  responsibility to actually verify the hash and to only start successfully
> > +	  verified images. The function to check the next stage image hash is
> > +	  firmware_next_image_verify(), make sure your SoC code uses it.
> > +
> >  endmenu
> > diff --git a/firmware/Makefile b/firmware/Makefile
> > index 095d6f0e31..67fd898890 100644
> > --- a/firmware/Makefile
> > +++ b/firmware/Makefile
> > @@ -34,6 +34,8 @@ pbl-firmware-$(CONFIG_ARCH_RK3588) += rk3588-bl32.bin
> >  pbl-firmware-$(CONFIG_ARCH_RK3399) += rk3399-bl32.bin
> >  endif
> >  
> > +firmware-$(CONFIG_FIRMWARE_NEXT_IMAGE) += next-image.bin
> 
> Why can't we use the fw-external here?

With fw-external the hash will end up in the PBL. I need it in barebox
proper.

> 
> > +
> >  firmware-$(CONFIG_DRIVER_NET_FSL_FMAN) += fsl_fman_ucode_ls1046_r1.0_106_4_18.bin
> >  
> >  fw-external-$(CONFIG_FIRMWARE_LS1028A_ATF) += ls1028a-bl31.bin
> > diff --git a/include/firmware.h b/include/firmware.h
> > index d7feae1371..7225b55e4f 100644
> > --- a/include/firmware.h
> > +++ b/include/firmware.h
> > @@ -13,6 +13,8 @@
> >  #include <debug_ll.h>
> >  #include <linux/kernel.h>
> >  #include <asm/sections.h>
> > +#include <crypto/sha.h>
> > +#include <crypto.h>
> >  
> >  struct firmware {
> >  	size_t size;
> > @@ -113,4 +115,30 @@ static inline void firmware_ext_verify(const void *data_start, size_t data_size,
> >  #define get_builtin_firmware_ext(name, base, start, size)		\
> >  	__get_builtin_firmware(name, (long)base - (long)_text, start, size)
> >  
> > +static inline int firmware_next_image_verify(const void *hash_start, size_t hash_size, bool verbose)
> > +{
> > +	extern char _fw_next_image_bin_sha_start[];
> > +	int ret;
> > +
> > +	if (!IS_ENABLED(CONFIG_FIRMWARE_NEXT_IMAGE))
> > +		return -EINVAL;
> > +
> > +	if (hash_size != SHA256_DIGEST_SIZE)
> > +		return -EINVAL;
> > +
> > +	ret = crypto_memneq(hash_start, _fw_next_image_bin_sha_start, hash_size);
> 
> If we don't check the runtime sha256 of next_image an attacker could
> replace next_image and keep the builtin sha256sum and we wouldn't
> recognize it, or do I miss something?

Not sure I can follow you. The sha256 of the next image is passed to
this function as argument and compared against the compiled in hash.

If you miss the call to actually calculate the hash: The caller of this
function calls it as I consider this being board dependent. Background
it that at least in a previous version I calculated the hash while
reading in the image, I never had a contiguous buffer where I could
calculate the hash over.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |