[PATCH 00/13] am625: support secure loading of full barebox

[PATCH 00/13] am625: support secure loading of full barebox

[Sascha Hauer]

On K3 SoCs only a small barebox is loaded by the ROM into SRAM. This
barebox then loads the full barebox from SD/eMMC or USB DFU. In a secure
boot environment the full barebox must be authenticated. This series
implements two ways for accomplishing this.

First way is to utilize the ROM API to authenticate images. The other
way is to compile a secure hash into the first stage binary and check
if the full barebox image matches the hash. Using the ROM API means
different first stage and second stage images can be combined whereas
hashing binds specific builds together avoiding mix and match attacks.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (13):
      firmware: always generate sha256sum
      firmware: add function to verify next image
      ARM: k3: r5: drop loading of separate binaries
      ARM: k3: r5: add proper error handling
      fip: rework fip_image_open()
      fip: fix wrong function call
      fip: add function to calculate a sha256 over FIP image
      ARM: am625: support hash verification of full barebox
      ARM: k3: add support for authenticating images against the ROM API
      ARM: k3: r5: delete fip image when it can't be opened
      ARM: k3: r5: Allow to authenticate next image by ROM API
      scripts/k3img: remove temporary files
      scripts: add k3sign

 arch/arm/mach-k3/Kconfig  |  15 ++++
 arch/arm/mach-k3/common.c |  99 ++++++++++++++++++++++
 arch/arm/mach-k3/r5.c     | 206 +++++++++++++++++++++++++---------------------
 firmware/Kconfig          |  23 ++++++
 firmware/Makefile         |   8 +-
 include/fiptool.h         |   3 +
 include/firmware.h        |  28 +++++++
 include/mach/k3/common.h  |   1 +
 lib/fip.c                 | 101 ++++++++++++++---------
 scripts/k3img             |   9 +-
 scripts/k3sign            | 126 ++++++++++++++++++++++++++++
 11 files changed, 478 insertions(+), 141 deletions(-)
---
base-commit: 748ba0627681797b01a94be1b3f879ed2e52a361
change-id: 20250228-am625-secure-49301f641738

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH 01/13] firmware: always generate sha256sum

[Sascha Hauer]

Right now we only generate the sha256 for firmware files that are
loaded from an external binary. Instead, always generate the sha256
which helps with upcoming support for checking the next image hash.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 firmware/Makefile | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/firmware/Makefile b/firmware/Makefile
index 4cf61587d6..095d6f0e31 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -76,9 +76,6 @@ filechk_fwbin = { \
 	echo "_fwname_$(FWSTR):"				;\
 	printf '.ascii "%s"\n' 'firmware/$(FWNAME)\n'		;\
 	echo "\#endif" 						;\
-}
-
-__fwbin_sha = { \
 	echo "    .section .rodata.$(FWSTR).sha"		;\
 	echo "    .p2align ASM_LGPTR"				;\
 	echo ".global _fw_$(FWSTR)_sha_start"			;\
@@ -90,10 +87,9 @@ __fwbin_sha = { \
 
 filechk_fwbin_ext = { \
 	$(filechk_fwbin)					;\
-	$(__fwbin_sha)						;\
 }
 
-$(obj)/%.gen.S: FORCE
+$(obj)/%.gen.S: $(obj)/%.sha.bin FORCE
 	$(call filechk,fwbin,.rodata.$(FWSTR),)
 
 $(obj)/%.extgen.S: $(obj)/%.sha.bin FORCE

-- 
2.39.5

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

[Sascha Hauer]

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
+
 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 (verbose) {
+		if (ret) {
+			pr_err("next image hash mismatch!\n");
+			pr_err("expected: sha256=%*phN\n", hash_size, _fw_next_image_bin_sha_start);
+			pr_err("found:    sha256=%*phN\n", hash_size, hash_start);
+		} else {
+			pr_info("hash sha256=%*phN OK\n", hash_size, _fw_next_image_bin_sha_start);
+		}
+	}
+
+	return ret;
+}
+
 #endif /* FIRMWARE_H */

-- 
2.39.5

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

[Marco Felsch]

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?

> +
>  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?

Regards,
  Marco

> +
> +	if (verbose) {
> +		if (ret) {
> +			pr_err("next image hash mismatch!\n");
> +			pr_err("expected: sha256=%*phN\n", hash_size, _fw_next_image_bin_sha_start);
> +			pr_err("found:    sha256=%*phN\n", hash_size, hash_start);
> +		} else {
> +			pr_info("hash sha256=%*phN OK\n", hash_size, _fw_next_image_bin_sha_start);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  #endif /* FIRMWARE_H */
> 
> -- 
> 2.39.5
> 
> 
> 

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

[Sascha Hauer]

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 |

[PATCH 03/13] ARM: k3: r5: drop loading of separate binaries

[Sascha Hauer]

For starting the A52 cores we need TFA, OP-TEE, barebox and ti-dm
binaries. These can be loaded as separate files from SD/eMMC/DFU or
alternatively combined into a single FIP image.
FIP images are convenient to handle, they can easily be generated on the
command line and fiptool also has support for replacing blobs in images.
This makes handling of separate binaries rather unnecessary and support
for it only makes the loading code more complex. Drop it and make FIP
images the only option which also faciliates integrating of secure boot.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/r5.c | 70 +--------------------------------------------------
 1 file changed, 1 insertion(+), 69 deletions(-)

diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
index ced1eb2856..2418e9ae73 100644
--- a/arch/arm/mach-k3/r5.c
+++ b/arch/arm/mach-k3/r5.c
@@ -288,50 +288,15 @@ static void do_dfu(void)
 	struct usbgadget_funcs funcs = {};
 	int ret;
 	struct stat s;
-	ssize_t size;
 
 	funcs.flags |= USBGADGET_DFU;
-	funcs.dfu_opts = "/optee.bin(optee)c,"
-			 "/bl31.bin(tfa)c,"
-			 "/ti-dm.bin(ti-dm)c,"
-			 "/barebox.bin(barebox)cs,"
-			 "/fip.img(fip)cs";
+	funcs.dfu_opts = "/fip.img(fip)cs";
 
 	ret = usbgadget_prepare_register(&funcs);
 	if (ret)
 		goto err;
 
 	while (1) {
-		if (!have_bl32) {
-			size = read_file_into_buf("/optee.bin", BL32_ADDRESS, SZ_32M);
-			if (size > 0) {
-				printf("Downloaded OP-TEE\n");
-				have_bl32 = true;
-			}
-		}
-
-		if (!have_bl31) {
-			size = read_file_into_buf("/bl31.bin", BL31_ADDRESS, SZ_32M);
-			if (size > 0) {
-				printf("Downloaded TF-A\n");
-				have_bl31 = true;
-			}
-		}
-
-		if (!k3_ti_dm) {
-			ret = read_file_2("/ti-dm.bin", &size, &k3_ti_dm, FILESIZE_MAX);
-			if (!ret) {
-				printf("Downloaded TI-DM\n");
-			}
-		}
-
-		size = read_file_into_buf("/barebox.bin", BAREBOX_ADDRESS, SZ_32M);
-		if (size > 0) {
-			have_bl33 = true;
-			printf("Downloaded barebox image, DFU done\n");
-			break;
-		}
-
 		ret = stat("/fip.img", &s);
 		if (!ret) {
 			printf("Downloaded FIP image, DFU done\n");
@@ -352,45 +317,12 @@ static void do_dfu(void)
 
 static int load_images(void)
 {
-	ssize_t size;
 	int err;
 
 	err = load_fip("/boot/k3.fip", 0);
 	if (!err)
 		return 0;
 
-	size = read_file_into_buf("/boot/optee.bin", BL32_ADDRESS, SZ_32M);
-	if (size < 0) {
-		if (size != -ENOENT) {
-			pr_err("Cannot load optee.bin: %pe\n", ERR_PTR(size));
-			return size;
-		}
-		pr_info("optee.bin not found, continue without\n");
-	} else {
-		pr_debug("Loaded optee.bin (size %u) to 0x%p\n", size, BL32_ADDRESS);
-	}
-
-	size = read_file_into_buf("/boot/barebox.bin", BAREBOX_ADDRESS, SZ_32M);
-	if (size < 0) {
-		pr_err("Cannot load barebox.bin: %pe\n", ERR_PTR(size));
-		return size;
-	}
-	pr_debug("Loaded barebox.bin (size %u) to 0x%p\n", size, BAREBOX_ADDRESS);
-
-	size = read_file_into_buf("/boot/bl31.bin", BL31_ADDRESS, SZ_32M);
-	if (size < 0) {
-		pr_err("Cannot load bl31.bin: %pe\n", ERR_PTR(size));
-		return size;
-	}
-	pr_debug("Loaded bl31.bin (size %u) to 0x%p\n", size, BL31_ADDRESS);
-
-	err = read_file_2("/boot/ti-dm.bin", &size, &k3_ti_dm, FILESIZE_MAX);
-	if (err) {
-		pr_err("Cannot load ti-dm.bin: %pe\n", ERR_PTR(err));
-		return err;
-	}
-	pr_debug("Loaded ti-dm.bin (size %u)\n", size);
-
 	return 0;
 }
 

-- 
2.39.5

Re: [PATCH 03/13] ARM: k3: r5: drop loading of separate binaries

[Marco Felsch]

On 25-02-28, Sascha Hauer wrote:
> For starting the A52 cores we need TFA, OP-TEE, barebox and ti-dm
	             ^
		     3
> binaries. These can be loaded as separate files from SD/eMMC/DFU or
> alternatively combined into a single FIP image.
> FIP images are convenient to handle, they can easily be generated on the
> command line and fiptool also has support for replacing blobs in images.
> This makes handling of separate binaries rather unnecessary and support
> for it only makes the loading code more complex. Drop it and make FIP
> images the only option which also faciliates integrating of secure boot.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-k3/r5.c | 70 +--------------------------------------------------
>  1 file changed, 1 insertion(+), 69 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> index ced1eb2856..2418e9ae73 100644
> --- a/arch/arm/mach-k3/r5.c
> +++ b/arch/arm/mach-k3/r5.c
> @@ -288,50 +288,15 @@ static void do_dfu(void)
>  	struct usbgadget_funcs funcs = {};
>  	int ret;
>  	struct stat s;
> -	ssize_t size;
>  
>  	funcs.flags |= USBGADGET_DFU;
> -	funcs.dfu_opts = "/optee.bin(optee)c,"
> -			 "/bl31.bin(tfa)c,"
> -			 "/ti-dm.bin(ti-dm)c,"
> -			 "/barebox.bin(barebox)cs,"
> -			 "/fip.img(fip)cs";
> +	funcs.dfu_opts = "/fip.img(fip)cs";
>  
>  	ret = usbgadget_prepare_register(&funcs);
>  	if (ret)
>  		goto err;
>  
>  	while (1) {
> -		if (!have_bl32) {
> -			size = read_file_into_buf("/optee.bin", BL32_ADDRESS, SZ_32M);
> -			if (size > 0) {
> -				printf("Downloaded OP-TEE\n");
> -				have_bl32 = true;
> -			}
> -		}
> -
> -		if (!have_bl31) {
> -			size = read_file_into_buf("/bl31.bin", BL31_ADDRESS, SZ_32M);
> -			if (size > 0) {
> -				printf("Downloaded TF-A\n");
> -				have_bl31 = true;
> -			}
> -		}
> -
> -		if (!k3_ti_dm) {
> -			ret = read_file_2("/ti-dm.bin", &size, &k3_ti_dm, FILESIZE_MAX);
> -			if (!ret) {
> -				printf("Downloaded TI-DM\n");
> -			}
> -		}
> -
> -		size = read_file_into_buf("/barebox.bin", BAREBOX_ADDRESS, SZ_32M);
> -		if (size > 0) {
> -			have_bl33 = true;
> -			printf("Downloaded barebox image, DFU done\n");
> -			break;
> -		}
> -
>  		ret = stat("/fip.img", &s);
>  		if (!ret) {
>  			printf("Downloaded FIP image, DFU done\n");
> @@ -352,45 +317,12 @@ static void do_dfu(void)
>  
>  static int load_images(void)
>  {
> -	ssize_t size;
>  	int err;
>  
>  	err = load_fip("/boot/k3.fip", 0);

Since load_fip() is the only call we could remove the entire function
and call load_fip() directly from k3_r5_start_image().

Regards,
  Marco

>  	if (!err)
>  		return 0;
>  
> -	size = read_file_into_buf("/boot/optee.bin", BL32_ADDRESS, SZ_32M);
> -	if (size < 0) {
> -		if (size != -ENOENT) {
> -			pr_err("Cannot load optee.bin: %pe\n", ERR_PTR(size));
> -			return size;
> -		}
> -		pr_info("optee.bin not found, continue without\n");
> -	} else {
> -		pr_debug("Loaded optee.bin (size %u) to 0x%p\n", size, BL32_ADDRESS);
> -	}
> -
> -	size = read_file_into_buf("/boot/barebox.bin", BAREBOX_ADDRESS, SZ_32M);
> -	if (size < 0) {
> -		pr_err("Cannot load barebox.bin: %pe\n", ERR_PTR(size));
> -		return size;
> -	}
> -	pr_debug("Loaded barebox.bin (size %u) to 0x%p\n", size, BAREBOX_ADDRESS);
> -
> -	size = read_file_into_buf("/boot/bl31.bin", BL31_ADDRESS, SZ_32M);
> -	if (size < 0) {
> -		pr_err("Cannot load bl31.bin: %pe\n", ERR_PTR(size));
> -		return size;
> -	}
> -	pr_debug("Loaded bl31.bin (size %u) to 0x%p\n", size, BL31_ADDRESS);
> -
> -	err = read_file_2("/boot/ti-dm.bin", &size, &k3_ti_dm, FILESIZE_MAX);
> -	if (err) {
> -		pr_err("Cannot load ti-dm.bin: %pe\n", ERR_PTR(err));
> -		return err;
> -	}
> -	pr_debug("Loaded ti-dm.bin (size %u)\n", size);
> -
>  	return 0;
>  }
>  
> 
> -- 
> 2.39.5
> 
> 
> 

[PATCH 04/13] ARM: k3: r5: add proper error handling

[Sascha Hauer]

Add missing error handling in the image loading code. Properly check for
errors and panic when mandatory binaries are missing instead of blindly
continuing.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/r5.c | 57 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
index 2418e9ae73..e12c888afa 100644
--- a/arch/arm/mach-k3/r5.c
+++ b/arch/arm/mach-k3/r5.c
@@ -283,7 +283,7 @@ static int load_fip(const char *filename, off_t offset)
 	return 0;
 }
 
-static void do_dfu(void)
+static int do_dfu(void)
 {
 	struct usbgadget_funcs funcs = {};
 	int ret;
@@ -293,37 +293,24 @@ static void do_dfu(void)
 	funcs.dfu_opts = "/fip.img(fip)cs";
 
 	ret = usbgadget_prepare_register(&funcs);
-	if (ret)
-		goto err;
+	if (ret) {
+		pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
 
 	while (1) {
 		ret = stat("/fip.img", &s);
 		if (!ret) {
 			printf("Downloaded FIP image, DFU done\n");
-			load_fip("/fip.img", 0);
-			break;
+			ret = load_fip("/fip.img", 0);
+			if (!ret)
+				return 0;
 		}
 
 		command_slice_release();
 		mdelay(50);
 		command_slice_acquire();
 	};
-
-	return;
-
-err:
-	pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
-}
-
-static int load_images(void)
-{
-	int err;
-
-	err = load_fip("/boot/k3.fip", 0);
-	if (!err)
-		return 0;
-
-	return 0;
 }
 
 static int load_fip_emmc(void)
@@ -332,6 +319,7 @@ static int load_fip_emmc(void)
 	struct mci *mci;
 	char *fname;
 	const char *mmcdev = "mmc0";
+	int ret;
 
 	device_detect_by_name(mmcdev);
 
@@ -348,11 +336,11 @@ static int load_fip_emmc(void)
 
 	fname = xasprintf("/dev/%s.boot%d", mmcdev, bootpart - 1);
 
-	load_fip(fname, K3_EMMC_BOOTPART_TIBOOT3_BIN_SIZE);
+	ret = load_fip(fname, K3_EMMC_BOOTPART_TIBOOT3_BIN_SIZE);
 
 	free(fname);
 
-	return 0;
+	return ret;
 }
 
 static int k3_r5_start_image(void)
@@ -365,11 +353,28 @@ static int k3_r5_start_image(void)
 	struct rproc *arm64_rproc;
 
 	if (IS_ENABLED(CONFIG_USB_GADGET_DFU) && bootsource_get() == BOOTSOURCE_SERIAL)
-		do_dfu();
+		err = do_dfu();
 	else if (k3_boot_is_emmc())
-		load_fip_emmc();
+		err = load_fip_emmc();
 	else
-		load_images();
+		err = load_fip("/boot/k3.fip", 0);
+
+	if (err) {
+		pr_crit("Unable to load FIP image\n");
+		panic("Stop booting\n");
+	}
+
+	if (!have_bl31)
+		panic("No TFA found in FIP image\n");
+
+	if (!have_bl32)
+		pr_info("No OP-TEE found. Continuing without\n");
+
+	if (!have_bl33)
+		panic("No bl33 found in FIP image\n");
+
+	if (!k3_ti_dm)
+		panic("No ti-dm binary found\n");
 
 	ti_sci = ti_sci_get_handle(NULL);
 	if (IS_ERR(ti_sci))

-- 
2.39.5

Re: [PATCH 04/13] ARM: k3: r5: add proper error handling

[Marco Felsch]

On 25-02-28, Sascha Hauer wrote:
> Add missing error handling in the image loading code. Properly check for
> errors and panic when mandatory binaries are missing instead of blindly
> continuing.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-k3/r5.c | 57 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> index 2418e9ae73..e12c888afa 100644
> --- a/arch/arm/mach-k3/r5.c
> +++ b/arch/arm/mach-k3/r5.c
> @@ -283,7 +283,7 @@ static int load_fip(const char *filename, off_t offset)
>  	return 0;
>  }
>  
> -static void do_dfu(void)
> +static int do_dfu(void)
>  {
>  	struct usbgadget_funcs funcs = {};
>  	int ret;
> @@ -293,37 +293,24 @@ static void do_dfu(void)
>  	funcs.dfu_opts = "/fip.img(fip)cs";
>  
>  	ret = usbgadget_prepare_register(&funcs);
> -	if (ret)
> -		goto err;
> +	if (ret) {
> +		pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
>  
>  	while (1) {
>  		ret = stat("/fip.img", &s);
>  		if (!ret) {
>  			printf("Downloaded FIP image, DFU done\n");
> -			load_fip("/fip.img", 0);
> -			break;
> +			ret = load_fip("/fip.img", 0);
> +			if (!ret)
> +				return 0;
>  		}
>  
>  		command_slice_release();
>  		mdelay(50);
>  		command_slice_acquire();
>  	};
> -
> -	return;
> -
> -err:
> -	pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> -}
> -
> -static int load_images(void)
> -{
> -	int err;
> -
> -	err = load_fip("/boot/k3.fip", 0);
> -	if (!err)
> -		return 0;
> -
> -	return 0;
>  }

Removing load_images should go into patch-3.

>  
>  static int load_fip_emmc(void)
> @@ -332,6 +319,7 @@ static int load_fip_emmc(void)
>  	struct mci *mci;
>  	char *fname;
>  	const char *mmcdev = "mmc0";
> +	int ret;

Nit: Sometimes you do use ret, sometimes err. I would like to align it
if possible.

Rest LGTM.

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>

Regards,
  Marco

>  
>  	device_detect_by_name(mmcdev);
>  
> @@ -348,11 +336,11 @@ static int load_fip_emmc(void)
>  
>  	fname = xasprintf("/dev/%s.boot%d", mmcdev, bootpart - 1);
>  
> -	load_fip(fname, K3_EMMC_BOOTPART_TIBOOT3_BIN_SIZE);
> +	ret = load_fip(fname, K3_EMMC_BOOTPART_TIBOOT3_BIN_SIZE);
>  
>  	free(fname);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int k3_r5_start_image(void)
> @@ -365,11 +353,28 @@ static int k3_r5_start_image(void)
>  	struct rproc *arm64_rproc;
>  
>  	if (IS_ENABLED(CONFIG_USB_GADGET_DFU) && bootsource_get() == BOOTSOURCE_SERIAL)
> -		do_dfu();
> +		err = do_dfu();
>  	else if (k3_boot_is_emmc())
> -		load_fip_emmc();
> +		err = load_fip_emmc();
>  	else
> -		load_images();
> +		err = load_fip("/boot/k3.fip", 0);
> +
> +	if (err) {
> +		pr_crit("Unable to load FIP image\n");
> +		panic("Stop booting\n");
> +	}
> +
> +	if (!have_bl31)
> +		panic("No TFA found in FIP image\n");
> +
> +	if (!have_bl32)
> +		pr_info("No OP-TEE found. Continuing without\n");
> +
> +	if (!have_bl33)
> +		panic("No bl33 found in FIP image\n");
> +
> +	if (!k3_ti_dm)
> +		panic("No ti-dm binary found\n");
>  
>  	ti_sci = ti_sci_get_handle(NULL);
>  	if (IS_ERR(ti_sci))
> 
> -- 
> 2.39.5
> 
> 
> 

Re: [PATCH 04/13] ARM: k3: r5: add proper error handling

[Sascha Hauer]

On Mon, Mar 10, 2025 at 07:52:17PM +0100, Marco Felsch wrote:
> On 25-02-28, Sascha Hauer wrote:
> > Add missing error handling in the image loading code. Properly check for
> > errors and panic when mandatory binaries are missing instead of blindly
> > continuing.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/mach-k3/r5.c | 57 ++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 31 insertions(+), 26 deletions(-)
> > 
> > diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> > index 2418e9ae73..e12c888afa 100644
> > --- a/arch/arm/mach-k3/r5.c
> > +++ b/arch/arm/mach-k3/r5.c
> > @@ -283,7 +283,7 @@ static int load_fip(const char *filename, off_t offset)
> >  	return 0;
> >  }
> >  
> > -static void do_dfu(void)
> > +static int do_dfu(void)
> >  {
> >  	struct usbgadget_funcs funcs = {};
> >  	int ret;
> > @@ -293,37 +293,24 @@ static void do_dfu(void)
> >  	funcs.dfu_opts = "/fip.img(fip)cs";
> >  
> >  	ret = usbgadget_prepare_register(&funcs);
> > -	if (ret)
> > -		goto err;
> > +	if (ret) {
> > +		pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> > +		return ret;
> > +	}
> >  
> >  	while (1) {
> >  		ret = stat("/fip.img", &s);
> >  		if (!ret) {
> >  			printf("Downloaded FIP image, DFU done\n");
> > -			load_fip("/fip.img", 0);
> > -			break;
> > +			ret = load_fip("/fip.img", 0);
> > +			if (!ret)
> > +				return 0;
> >  		}
> >  
> >  		command_slice_release();
> >  		mdelay(50);
> >  		command_slice_acquire();
> >  	};
> > -
> > -	return;
> > -
> > -err:
> > -	pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> > -}
> > -
> > -static int load_images(void)
> > -{
> > -	int err;
> > -
> > -	err = load_fip("/boot/k3.fip", 0);
> > -	if (!err)
> > -		return 0;
> > -
> > -	return 0;
> >  }
> 
> Removing load_images should go into patch-3.

I deliberately did it in this patch because load_images already does
error checking, but up to now the return value of load_images() wasn't
checked. By doing it like you suggest I would either end up calling
load_fip() without checking the error, or I would introduce an
incomplete error checking in k3_r5_start_image() where the return value
of load_fip() is checked, but not the return values of do_dfu and
load_fip_emmc().

> >  static int load_fip_emmc(void)
> > @@ -332,6 +319,7 @@ static int load_fip_emmc(void)
> >  	struct mci *mci;
> >  	char *fname;
> >  	const char *mmcdev = "mmc0";
> > +	int ret;
> 
> Nit: Sometimes you do use ret, sometimes err. I would like to align it
> if possible.

Changed to 'ret' consistently.

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 |

Re: [PATCH 04/13] ARM: k3: r5: add proper error handling

[Marco Felsch]

On 25-03-11, Sascha Hauer wrote:
> On Mon, Mar 10, 2025 at 07:52:17PM +0100, Marco Felsch wrote:
> > On 25-02-28, Sascha Hauer wrote:
> > > Add missing error handling in the image loading code. Properly check for
> > > errors and panic when mandatory binaries are missing instead of blindly
> > > continuing.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  arch/arm/mach-k3/r5.c | 57 ++++++++++++++++++++++++++++-----------------------
> > >  1 file changed, 31 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> > > index 2418e9ae73..e12c888afa 100644
> > > --- a/arch/arm/mach-k3/r5.c
> > > +++ b/arch/arm/mach-k3/r5.c
> > > @@ -283,7 +283,7 @@ static int load_fip(const char *filename, off_t offset)
> > >  	return 0;
> > >  }
> > >  
> > > -static void do_dfu(void)
> > > +static int do_dfu(void)
> > >  {
> > >  	struct usbgadget_funcs funcs = {};
> > >  	int ret;
> > > @@ -293,37 +293,24 @@ static void do_dfu(void)
> > >  	funcs.dfu_opts = "/fip.img(fip)cs";
> > >  
> > >  	ret = usbgadget_prepare_register(&funcs);
> > > -	if (ret)
> > > -		goto err;
> > > +	if (ret) {
> > > +		pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> > > +		return ret;
> > > +	}
> > >  
> > >  	while (1) {
> > >  		ret = stat("/fip.img", &s);
> > >  		if (!ret) {
> > >  			printf("Downloaded FIP image, DFU done\n");
> > > -			load_fip("/fip.img", 0);
> > > -			break;
> > > +			ret = load_fip("/fip.img", 0);
> > > +			if (!ret)
> > > +				return 0;
> > >  		}
> > >  
> > >  		command_slice_release();
> > >  		mdelay(50);
> > >  		command_slice_acquire();
> > >  	};
> > > -
> > > -	return;
> > > -
> > > -err:
> > > -	pr_err("DFU failed with: %pe\n", ERR_PTR(ret));
> > > -}
> > > -
> > > -static int load_images(void)
> > > -{
> > > -	int err;
> > > -
> > > -	err = load_fip("/boot/k3.fip", 0);
> > > -	if (!err)
> > > -		return 0;
> > > -
> > > -	return 0;
> > >  }
> > 
> > Removing load_images should go into patch-3.
> 
> I deliberately did it in this patch because load_images already does
> error checking, but up to now the return value of load_images() wasn't
> checked. By doing it like you suggest I would either end up calling
> load_fip() without checking the error, or I would introduce an
> incomplete error checking in k3_r5_start_image() where the return value
> of load_fip() is checked, but not the return values of do_dfu and
> load_fip_emmc().

Ah okay, good point. In that case I'm fine with it.

Regards,
  Marco


> 
> > >  static int load_fip_emmc(void)
> > > @@ -332,6 +319,7 @@ static int load_fip_emmc(void)
> > >  	struct mci *mci;
> > >  	char *fname;
> > >  	const char *mmcdev = "mmc0";
> > > +	int ret;
> > 
> > Nit: Sometimes you do use ret, sometimes err. I would like to align it
> > if possible.
> 
> Changed to 'ret' consistently.
> 
> 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 |
> 

[PATCH 05/13] fip: rework fip_image_open()

[Sascha Hauer]

fip_image_open() used to do all the parsing into a struct fip_state
itself. Instead, only load the FIP image into a buffer and call
fip_do_parse_buf() with this buffer. This has the advantage that we
have all parsing of the FIP image in a single place. Also this helps
with a followup patch which calculates a sha256 over a FIP image
which can easily done when we have the whole FIP image in a contiguous
buffer.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 lib/fip.c | 78 +++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/lib/fip.c b/lib/fip.c
index 23e82098da..aab2795476 100644
--- a/lib/fip.c
+++ b/lib/fip.c
@@ -23,6 +23,7 @@
 #include <string.h>
 #include <libfile.h>
 #include <fs.h>
+#include <linux/kernel.h>
 
 #include <fip.h>
 #include <fiptool.h>
@@ -447,22 +448,17 @@ int fip_update(struct fip_state *fip)
 }
 
 /*
- * fip_image_open - open a FIP image for readonly access
+ * fip_image_open - open a FIP image
  * @filename: The filename of the FIP image
  * @offset: The offset of the FIP image in the file
  *
- * This opens a FIP image for readonly access. This is an alternative
- * implementation for fip_parse() with these differences:
+ * This opens a FIP image. This is an alternative implementation for
+ * fip_parse() with these differences:
  * - suitable for reading FIP images from raw partitions. This function
  *   only reads the FIP image, even when the partition is bigger than the
  *   image
  * - Allows to specify an offset within the partition where the FIP image
  *   starts
- * - Do not memdup the images from the full FIP image
- *
- * This function is for easy readonly access to the images within the FIP
- * image. Do not call any of the above FIP manipulation functions other than
- * fip_free() on an image opened with this function.
  */
 struct fip_state *fip_image_open(const char *filename, size_t offset)
 {
@@ -470,11 +466,13 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
 	int ret;
 	int fd;
 	struct fip_state *fip_state;
-	LIST_HEAD(entries);
 	size_t fip_headers_size, total = 0;
-	struct fip_image_desc *desc;
 	off_t pos;
 	int n_entries = 0;
+	struct fip_toc_entry toc_entries[16];
+	void *buf, *ptr;
+	int i;
+	struct fip_toc_entry *toc_entry;
 
 	fd = open(filename, O_RDONLY);
 	if (fd < 0)
@@ -506,11 +504,9 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
 
 	/* read all toc entries */
 	while (1) {
-		struct fip_image_desc *desc = xzalloc(sizeof(*desc));
-		struct fip_image *image = xzalloc(sizeof(*image));
-		struct fip_toc_entry *toc_entry = &image->toc_e;
+		uint64_t this_end;
 
-		desc->image = image;
+		toc_entry = &toc_entries[n_entries];
 
 		ret = read_full(fd, toc_entry, sizeof(*toc_entry));
 		if (ret < 0)
@@ -520,53 +516,61 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
 			goto err;
 		}
 
-		list_add_tail(&desc->list, &fip_state->descs);
-
 		pr_debug("Read TOC entry %pU %llu %llu\n", &toc_entry->uuid,
 			 toc_entry->offset_address, toc_entry->size);
 
-		/* Found the ToC terminator, we are done. */
-		if (uuid_is_null(&toc_entry->uuid))
-			break;
-	}
-
-	/* determine buffer size */
-	fip_for_each_desc(fip_state, desc) {
-		uint64_t this_end = desc->image->toc_e.offset_address + desc->image->toc_e.size;
+		this_end = toc_entry->offset_address + toc_entry->size;
 
 		if (this_end > total)
 			total = this_end;
+
 		n_entries++;
-	}
 
-	fip_headers_size = n_entries * sizeof(struct fip_toc_entry) + sizeof(fip_toc_header_t);
+		if (n_entries >= ARRAY_SIZE(toc_entries)) {
+			ret = -ENOSPC;
+			goto err;
+		}
 
-	total -= fip_headers_size;
+		/* Found the ToC terminator, we are done. */
+		if (uuid_is_null(&toc_entry->uuid))
+			break;
+	}
 
-	fip_state->buffer = malloc(total);
-	if (!fip_state->buffer) {
+	buf = malloc(total);
+	if (!buf) {
 		ret = -ENOMEM;
 		goto err;
 	}
 
-	ret = read_full(fd, fip_state->buffer, total);
+	ptr = buf;
+
+	memcpy(ptr, &toc_header, sizeof(toc_header));
+	ptr += sizeof(toc_header);
+
+	for (i = 0; i < n_entries; i++) {
+		memcpy(ptr, &toc_entries[i], sizeof(*toc_entry));
+		ptr += sizeof(*toc_entry);
+	}
+
+	fip_headers_size = n_entries * sizeof(struct fip_toc_entry) + sizeof(fip_toc_header_t);
+
+	ret = read_full(fd, ptr, total - fip_headers_size);
 	if (ret < 0)
 		goto err;
 
-	if (ret < total) {
+	if (ret < total - fip_headers_size) {
 		ret = -ENODATA;
 		goto err;
 	}
 
-	close(fd);
+	ret = fip_do_parse_buf(fip_state, buf, total, NULL);
+	if (ret)
+		goto err;
 
-	fip_for_each_desc(fip_state, desc) {
-		desc->image->buffer = fip_state->buffer +
-			desc->image->toc_e.offset_address - fip_headers_size;
-		desc->image->buf_no_free = true;
-	}
+	close(fd);
 
 	return fip_state;
+
 err:
 	close(fd);
 	fip_free(fip_state);

-- 
2.39.5

[PATCH 06/13] fip: fix wrong function call

[Sascha Hauer]

fip_parse_buf() is to be called when a buffer shall be parsed that is
allocated externally and thus shall not be freed in fip_free(). This
is not what we want in fip_parse(), we want exactly the buffer to be
freed, so call fip_do_parse_buf() instead.

Fixes: 748ba06276 ("fip: add function to parse FIP from a buffer")
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 lib/fip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fip.c b/lib/fip.c
index aab2795476..8086b43412 100644
--- a/lib/fip.c
+++ b/lib/fip.c
@@ -274,7 +274,7 @@ int fip_parse(struct fip_state *fip,
 		return ret;
 	}
 
-	ret = fip_parse_buf(fip, buf, size, toc_header_out);
+	ret = fip_do_parse_buf(fip, buf, size, toc_header_out);
 
 	if (ret)
 		free(buf);

-- 
2.39.5

[PATCH 07/13] fip: add function to calculate a sha256 over FIP image

[Sascha Hauer]

This adds fip_sha256() to calculate a sha256 over a FIP image.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/fiptool.h |  3 +++
 lib/fip.c         | 23 ++++++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/fiptool.h b/include/fiptool.h
index bb63a79c16..73b0fbe398 100644
--- a/include/fiptool.h
+++ b/include/fiptool.h
@@ -38,6 +38,7 @@ struct fip_state {
 	size_t nr_image_descs;
 	int verbose;
 	void *buffer;
+	size_t bufsize;
 	bool buf_no_free;
 };
 
@@ -100,4 +101,6 @@ extern toc_entry_t plat_def_toc_entries[];
 
 struct fip_state *fip_image_open(const char *filename, size_t offset);
 
+int fip_sha256(struct fip_state *fip, char *hash);
+
 #endif /* FIPTOOL_H */
diff --git a/lib/fip.c b/lib/fip.c
index 8086b43412..7a5e3dc844 100644
--- a/lib/fip.c
+++ b/lib/fip.c
@@ -24,6 +24,7 @@
 #include <libfile.h>
 #include <fs.h>
 #include <linux/kernel.h>
+#include <digest.h>
 
 #include <fip.h>
 #include <fiptool.h>
@@ -168,6 +169,7 @@ static int fip_do_parse_buf(struct fip_state *fip, void *buf, size_t size,
 	int terminated = 0;
 
 	fip->buffer = buf;
+	fip->bufsize = size;
 
 	bufend = fip->buffer + size;
 
@@ -570,10 +572,29 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
 	close(fd);
 
 	return fip_state;
-
 err:
 	close(fd);
 	fip_free(fip_state);
 
 	return ERR_PTR(ret);
 }
+
+int fip_sha256(struct fip_state *fip, char *hash)
+{
+	struct digest *d;
+	int ret;
+
+	d = digest_alloc_by_algo(HASH_ALGO_SHA256);
+	if (!d)
+		return -ENOSYS;
+
+	digest_init(d);
+
+	digest_update(d, fip->buffer, fip->bufsize);
+
+	ret = digest_final(d, hash);
+
+	digest_free(d);
+
+	return ret;
+}

-- 
2.39.5

[PATCH 08/13] ARM: am625: support hash verification of full barebox

[Sascha Hauer]

This implements the necessary SoC code to check the full barebox against
a sha256 compiled into the first stage barebox.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/Kconfig |  1 +
 arch/arm/mach-k3/r5.c    | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
index 50919dc7e3..561ad1dac4 100644
--- a/arch/arm/mach-k3/Kconfig
+++ b/arch/arm/mach-k3/Kconfig
@@ -16,6 +16,7 @@ config MACH_K3_CORTEX_R5
 	select ELF
 	select K3_DDRSS
 	select FIP
+	select HAVE_FIRMWARE_VERIFY_NEXT_IMAGE
 	depends on 32BIT
 	select ARM_USE_COMPRESSED_DTB
 	default y
diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
index e12c888afa..cb52ff364d 100644
--- a/arch/arm/mach-k3/r5.c
+++ b/arch/arm/mach-k3/r5.c
@@ -248,6 +248,8 @@ static int load_fip(const char *filename, off_t offset)
 {
 	struct fip_state *fip;
 	struct fip_image_desc *desc;
+	unsigned char shasum[SHA256_DIGEST_SIZE];
+	int ret;
 
 	fip = fip_image_open(filename, offset);
 	if (IS_ERR(fip)) {
@@ -255,6 +257,18 @@ static int load_fip(const char *filename, off_t offset)
 		return PTR_ERR(fip);
 	}
 
+	if (IS_ENABLED(CONFIG_FIRMWARE_VERIFY_NEXT_IMAGE)) {
+		ret = fip_sha256(fip, shasum);
+		if (ret) {
+			pr_err("Cannot calc fip sha256: %pe\n", ERR_PTR(ret));
+			return ret;
+		}
+
+		ret = firmware_next_image_verify(shasum, SHA256_DIGEST_SIZE, true);
+		if (ret)
+			return ret;
+	}
+
 	fip_for_each_desc(fip, desc) {
 		struct fip_toc_entry *toc_entry = &desc->image->toc_e;
 

-- 
2.39.5

Re: [PATCH 08/13] ARM: am625: support hash verification of full barebox

[Marco Felsch]

On 25-02-28, Sascha Hauer wrote:
> This implements the necessary SoC code to check the full barebox against
> a sha256 compiled into the first stage barebox.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-k3/Kconfig |  1 +
>  arch/arm/mach-k3/r5.c    | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> index 50919dc7e3..561ad1dac4 100644
> --- a/arch/arm/mach-k3/Kconfig
> +++ b/arch/arm/mach-k3/Kconfig
> @@ -16,6 +16,7 @@ config MACH_K3_CORTEX_R5
>  	select ELF
>  	select K3_DDRSS
>  	select FIP
> +	select HAVE_FIRMWARE_VERIFY_NEXT_IMAGE
>  	depends on 32BIT
>  	select ARM_USE_COMPRESSED_DTB
>  	default y
> diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> index e12c888afa..cb52ff364d 100644
> --- a/arch/arm/mach-k3/r5.c
> +++ b/arch/arm/mach-k3/r5.c
> @@ -248,6 +248,8 @@ static int load_fip(const char *filename, off_t offset)
>  {
>  	struct fip_state *fip;
>  	struct fip_image_desc *desc;
> +	unsigned char shasum[SHA256_DIGEST_SIZE];
> +	int ret;
>  
>  	fip = fip_image_open(filename, offset);
>  	if (IS_ERR(fip)) {
> @@ -255,6 +257,18 @@ static int load_fip(const char *filename, off_t offset)
>  		return PTR_ERR(fip);
>  	}
>  
> +	if (IS_ENABLED(CONFIG_FIRMWARE_VERIFY_NEXT_IMAGE)) {
> +		ret = fip_sha256(fip, shasum);
> +		if (ret) {
> +			pr_err("Cannot calc fip sha256: %pe\n", ERR_PTR(ret));
> +			return ret;
> +		}
> +
> +		ret = firmware_next_image_verify(shasum, SHA256_DIGEST_SIZE, true);
> +		if (ret)
> +			return ret;

Albeit it would involve way more effort, I would like to see that the
FIP image format does have support for signatures within their "struct
image_desc" for each image.
This way it would be far easier for us to verify each image separately
and in a common way. Also it wouldn't require to rebuild the "r5"
tiboot3.bin to include the the updated sha256sum each time.

I do see why we can't use the externa-firmware mechanism now but I
wouldn't call it firmware_next_image_verify(). Maybe limit the scope to
fip and extent it later or remove/deprecate it once we managed to add
signatures to the FIP format.
Also the shasum size seems like the user would have a choice to choose
other sha-sums which he hasn't, therefore I would drop it.

Regards,
  Marco

> +	}
> +
>  	fip_for_each_desc(fip, desc) {
>  		struct fip_toc_entry *toc_entry = &desc->image->toc_e;
>  
> 
> -- 
> 2.39.5
> 
> 
> 

Re: [PATCH 08/13] ARM: am625: support hash verification of full barebox

[Sascha Hauer]

On Mon, Mar 10, 2025 at 08:22:26PM +0100, Marco Felsch wrote:
> On 25-02-28, Sascha Hauer wrote:
> > This implements the necessary SoC code to check the full barebox against
> > a sha256 compiled into the first stage barebox.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/mach-k3/Kconfig |  1 +
> >  arch/arm/mach-k3/r5.c    | 14 ++++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> > index 50919dc7e3..561ad1dac4 100644
> > --- a/arch/arm/mach-k3/Kconfig
> > +++ b/arch/arm/mach-k3/Kconfig
> > @@ -16,6 +16,7 @@ config MACH_K3_CORTEX_R5
> >  	select ELF
> >  	select K3_DDRSS
> >  	select FIP
> > +	select HAVE_FIRMWARE_VERIFY_NEXT_IMAGE
> >  	depends on 32BIT
> >  	select ARM_USE_COMPRESSED_DTB
> >  	default y
> > diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> > index e12c888afa..cb52ff364d 100644
> > --- a/arch/arm/mach-k3/r5.c
> > +++ b/arch/arm/mach-k3/r5.c
> > @@ -248,6 +248,8 @@ static int load_fip(const char *filename, off_t offset)
> >  {
> >  	struct fip_state *fip;
> >  	struct fip_image_desc *desc;
> > +	unsigned char shasum[SHA256_DIGEST_SIZE];
> > +	int ret;
> >  
> >  	fip = fip_image_open(filename, offset);
> >  	if (IS_ERR(fip)) {
> > @@ -255,6 +257,18 @@ static int load_fip(const char *filename, off_t offset)
> >  		return PTR_ERR(fip);
> >  	}
> >  
> > +	if (IS_ENABLED(CONFIG_FIRMWARE_VERIFY_NEXT_IMAGE)) {
> > +		ret = fip_sha256(fip, shasum);
> > +		if (ret) {
> > +			pr_err("Cannot calc fip sha256: %pe\n", ERR_PTR(ret));
> > +			return ret;
> > +		}
> > +
> > +		ret = firmware_next_image_verify(shasum, SHA256_DIGEST_SIZE, true);
> > +		if (ret)
> > +			return ret;
> 
> Albeit it would involve way more effort, I would like to see that the
> FIP image format does have support for signatures within their "struct
> image_desc" for each image.
> This way it would be far easier for us to verify each image separately
> and in a common way. Also it wouldn't require to rebuild the "r5"
> tiboot3.bin to include the the updated sha256sum each time.

Having to rebuild the tiboot3.bin for the updated sha256sum is not a
downside in this case, but actually the feature I wanted to implement.

Using a hash avoids mix-and-match attacks between different 1st stage
images combined with different 2nd stage images.

So yes, using the FIP image signing mechanisms would be nice to have,
but doesn't meet my goal.

> Also the shasum size seems like the user would have a choice to choose
> other sha-sums which he hasn't, therefore I would drop it.

ok.

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 |

[PATCH 09/13] ARM: k3: add support for authenticating images against the ROM API

[Sascha Hauer]

This adds support for authenticating an image against the K3 ROM API.
Also included in this patch is a new command for testing the
authentication.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/Kconfig  |  7 ++++
 arch/arm/mach-k3/common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++
 include/mach/k3/common.h  |  1 +
 3 files changed, 107 insertions(+)

diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
index 561ad1dac4..37d5155577 100644
--- a/arch/arm/mach-k3/Kconfig
+++ b/arch/arm/mach-k3/Kconfig
@@ -37,4 +37,11 @@ config MACH_BEAGLEPLAY
 	help
 	  Say Y here if you are using a TI AM62x based BeaglePlay board
 
+config ARCH_K3_COMMAND_AUTHENTICATE
+	bool "k3_authenticate_image command"
+	depends on COMMAND_SUPPORT
+	help
+	  Add k3_authenticate_image command to test authenticating images
+	  against the K3 ROM API.
+
 endmenu
diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index 8dc9d3bc9e..a324e2d5f6 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 #include <of.h>
 #include <io.h>
+#include <dma.h>
 #include <deep-probe.h>
 #include <init.h>
 #include <envfs.h>
@@ -8,11 +9,15 @@
 #include <stdio.h>
 #include <xfuncs.h>
 #include <malloc.h>
+#include <command.h>
+#include <getopt.h>
+#include <libfile.h>
 #include <pm_domain.h>
 #include <bootsource.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <mach/k3/common.h>
+#include <soc/ti/ti_sci_protocol.h>
 
 static const struct of_device_id k3_of_match[] = {
 	{
@@ -334,3 +339,97 @@ static int omap_env_init(void)
 	return 0;
 }
 late_initcall(omap_env_init);
+
+int k3_authenticate_image(void **buf, size_t *size)
+{
+	const struct ti_sci_handle *ti_sci;
+	u64 image_addr;
+	int ret;
+	unsigned int s = *size;
+
+	ti_sci = ti_sci_get_handle(NULL);
+	if (IS_ERR(ti_sci))
+		return -EINVAL;
+
+	image_addr = dma_map_single(NULL, *buf, *size, DMA_BIDIRECTIONAL);
+
+	ret = ti_sci->ops.proc_ops.proc_auth_boot_image(ti_sci, &image_addr, &s);
+
+	dma_unmap_single(NULL, image_addr, *size, DMA_BIDIRECTIONAL);
+
+	if (ret)
+		*size = 0;
+	else
+		*size = s;
+
+	return ret;
+}
+
+#ifdef CONFIG_ARCH_K3_COMMAND_AUTHENTICATE
+static int do_k3_authenticate_image(int argc, char *argv[])
+{
+	void *buf;
+	size_t size;
+	int ret;
+	int opt;
+	const char *outfile = NULL;
+	const char *filename;
+
+	while ((opt = getopt(argc, argv, "o:")) > 0) {
+		switch (opt) {
+		case 'o':
+			outfile = optarg;
+			break;
+		}
+	}
+
+	argc -= optind;
+
+	if (argc == 0)
+		return COMMAND_ERROR_USAGE;
+
+	filename = argv[0];
+
+	ret = read_file_2(filename, &size, &buf, FILESIZE_MAX);
+	if (ret)
+		return ret;
+
+	ret = k3_authenticate_image(&buf, &size);
+	if (ret) {
+		printf("authenticating %s failed: %pe\n", filename, ERR_PTR(ret));
+		ret = COMMAND_ERROR;
+		goto err;
+	}
+
+	printf("%s successfully authenticated\n", filename);
+
+	if (outfile) {
+		ret = write_file(outfile, buf, size);
+		if (ret) {
+			printf("Failed to write output file: %pe\n", ERR_PTR(ret));
+			goto err;
+		}
+	}
+
+	ret = 0;
+err:
+	free(buf);
+
+	return ret;
+}
+
+BAREBOX_CMD_HELP_START(k3_authenticate_image)
+BAREBOX_CMD_HELP_TEXT("Authenticate image with K3 ROM API")
+BAREBOX_CMD_HELP_TEXT("")
+BAREBOX_CMD_HELP_TEXT("Options:")
+BAREBOX_CMD_HELP_OPT ("-o <out>",  "store unpacked authenticated image in <out>")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(k3_authenticate_image)
+	.cmd = do_k3_authenticate_image,
+	BAREBOX_CMD_DESC("authenticate file with K3 ROM API")
+	BAREBOX_CMD_OPTS("[-o <out>] file")
+	BAREBOX_CMD_GROUP(CMD_GRP_MISC)
+	BAREBOX_CMD_HELP(k3_authenticate_image)
+BAREBOX_CMD_END
+#endif
diff --git a/include/mach/k3/common.h b/include/mach/k3/common.h
index 871e9f39e3..94c5fba19d 100644
--- a/include/mach/k3/common.h
+++ b/include/mach/k3/common.h
@@ -13,6 +13,7 @@ bool k3_boot_is_emmc(void);
 u64 am625_sdram_size(void);
 void am625_register_dram(void);
 void am625_enable_32k_crystal(void);
+int k3_authenticate_image(void **buf, size_t *size);
 
 #define K3_EMMC_BOOTPART_TIBOOT3_BIN_SIZE	SZ_1M
 

-- 
2.39.5

[PATCH 10/13] ARM: k3: r5: delete fip image when it can't be opened

[Sascha Hauer]

In case the uploaded FIP image can't be opened due to corruption of
the image we end up in an endless loop trying to open the same image
again and again. Delete the uploaded image in this case to give the
user a chance to upload another image.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/r5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
index cb52ff364d..c8a91e2597 100644
--- a/arch/arm/mach-k3/r5.c
+++ b/arch/arm/mach-k3/r5.c
@@ -319,6 +319,7 @@ static int do_dfu(void)
 			ret = load_fip("/fip.img", 0);
 			if (!ret)
 				return 0;
+			unlink("/fip.img");
 		}
 
 		command_slice_release();

-- 
2.39.5

[PATCH 11/13] ARM: k3: r5: Allow to authenticate next image by ROM API

[Sascha Hauer]

This adds the Kconfig option CONFIG_ARCH_K3_AUTHENTICATE_IMAGE. When
enabled, the full barebox image will only be started when it can be
authenticated using the ROM API.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-k3/Kconfig |  7 ++++++
 arch/arm/mach-k3/r5.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
index 37d5155577..e93e3154c8 100644
--- a/arch/arm/mach-k3/Kconfig
+++ b/arch/arm/mach-k3/Kconfig
@@ -37,6 +37,13 @@ config MACH_BEAGLEPLAY
 	help
 	  Say Y here if you are using a TI AM62x based BeaglePlay board
 
+config ARCH_K3_AUTHENTICATE_IMAGE
+	bool "Force authentication of FIP image against ROM API"
+	help
+	  By enabling this option the FIP image loaded by the first stage
+	  will be authenticated against the K3 ROM API. Images which fail
+	  to authenticate will not be started.
+
 config ARCH_K3_COMMAND_AUTHENTICATE
 	bool "k3_authenticate_image command"
 	depends on COMMAND_SUPPORT
diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
index c8a91e2597..d0093a3be7 100644
--- a/arch/arm/mach-k3/r5.c
+++ b/arch/arm/mach-k3/r5.c
@@ -244,6 +244,64 @@ static uuid_t uuid_ti_dm_fw = UUID_TI_DM_FW;
 static uuid_t uuid_bl33 = UUID_NON_TRUSTED_FIRMWARE_BL33;
 static uuid_t uuid_bl32 = UUID_SECURE_PAYLOAD_BL32;
 
+static struct fip_state *fip_image_load_auth(const char *filename, size_t offset)
+{
+	struct fip_state *fip = NULL;
+	int fd;
+	unsigned int maxsize = SZ_4M;
+	size_t size;
+	void *buf = NULL;
+	int ret;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0)
+		return ERR_PTR(-errno);
+
+	if (offset) {
+		loff_t pos;
+		pos = lseek(fd, offset, SEEK_SET);
+		if (pos < 0) {
+			ret = -errno;
+			goto err;
+		}
+	}
+
+	buf = xzalloc(maxsize);
+
+	/*
+	 * There is no easy way to determine the size of the certificates the ROM
+	 * takes as images, so the best we can do here is to assume a maximum size
+	 * and load this.
+	 */
+	ret = read_full(fd, buf, maxsize);
+	if (ret < 0)
+		goto err;
+
+	size = maxsize;
+
+	ret = k3_authenticate_image(&buf, &size);
+	if (ret) {
+		pr_err("Failed to authenticate %s\n", filename);
+		goto err;
+	}
+
+	fip = fip_new();
+	ret = fip_parse_buf(fip, buf, size, NULL);
+	if (ret)
+		goto err;
+
+	close(fd);
+
+	return fip;
+err:
+	if (fip)
+		fip_free(fip);
+	close(fd);
+	free(buf);
+
+	return ERR_PTR(ret);
+}
+
 static int load_fip(const char *filename, off_t offset)
 {
 	struct fip_state *fip;
@@ -251,7 +309,11 @@ static int load_fip(const char *filename, off_t offset)
 	unsigned char shasum[SHA256_DIGEST_SIZE];
 	int ret;
 
-	fip = fip_image_open(filename, offset);
+	if (IS_ENABLED(CONFIG_ARCH_K3_AUTHENTICATE_IMAGE))
+		fip = fip_image_load_auth(filename, offset);
+	else
+		fip = fip_image_open(filename, offset);
+
 	if (IS_ERR(fip)) {
 		pr_err("Cannot open FIP image: %pe\n", fip);
 		return PTR_ERR(fip);

-- 
2.39.5

Re: [PATCH 11/13] ARM: k3: r5: Allow to authenticate next image by ROM API

[Marco Felsch]

On 25-02-28, Sascha Hauer wrote:
> This adds the Kconfig option CONFIG_ARCH_K3_AUTHENTICATE_IMAGE. When
> enabled, the full barebox image will only be started when it can be
> authenticated using the ROM API.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/mach-k3/Kconfig |  7 ++++++
>  arch/arm/mach-k3/r5.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> index 37d5155577..e93e3154c8 100644
> --- a/arch/arm/mach-k3/Kconfig
> +++ b/arch/arm/mach-k3/Kconfig
> @@ -37,6 +37,13 @@ config MACH_BEAGLEPLAY
>  	help
>  	  Say Y here if you are using a TI AM62x based BeaglePlay board
>  
> +config ARCH_K3_AUTHENTICATE_IMAGE
> +	bool "Force authentication of FIP image against ROM API"
> +	help
> +	  By enabling this option the FIP image loaded by the first stage
> +	  will be authenticated against the K3 ROM API. Images which fail
> +	  to authenticate will not be started.

The ROM is checking only the sha sum or does the ROM also have some kind
of signature-checking (pub/priv keys requried)?

Regards,
  Marco


> +
>  config ARCH_K3_COMMAND_AUTHENTICATE
>  	bool "k3_authenticate_image command"
>  	depends on COMMAND_SUPPORT
> diff --git a/arch/arm/mach-k3/r5.c b/arch/arm/mach-k3/r5.c
> index c8a91e2597..d0093a3be7 100644
> --- a/arch/arm/mach-k3/r5.c
> +++ b/arch/arm/mach-k3/r5.c
> @@ -244,6 +244,64 @@ static uuid_t uuid_ti_dm_fw = UUID_TI_DM_FW;
>  static uuid_t uuid_bl33 = UUID_NON_TRUSTED_FIRMWARE_BL33;
>  static uuid_t uuid_bl32 = UUID_SECURE_PAYLOAD_BL32;
>  
> +static struct fip_state *fip_image_load_auth(const char *filename, size_t offset)
> +{
> +	struct fip_state *fip = NULL;
> +	int fd;
> +	unsigned int maxsize = SZ_4M;
> +	size_t size;
> +	void *buf = NULL;
> +	int ret;
> +
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0)
> +		return ERR_PTR(-errno);
> +
> +	if (offset) {
> +		loff_t pos;
> +		pos = lseek(fd, offset, SEEK_SET);
> +		if (pos < 0) {
> +			ret = -errno;
> +			goto err;
> +		}
> +	}
> +
> +	buf = xzalloc(maxsize);
> +
> +	/*
> +	 * There is no easy way to determine the size of the certificates the ROM
> +	 * takes as images, so the best we can do here is to assume a maximum size
> +	 * and load this.
> +	 */
> +	ret = read_full(fd, buf, maxsize);
> +	if (ret < 0)
> +		goto err;
> +
> +	size = maxsize;
> +
> +	ret = k3_authenticate_image(&buf, &size);
> +	if (ret) {
> +		pr_err("Failed to authenticate %s\n", filename);
> +		goto err;
> +	}
> +
> +	fip = fip_new();
> +	ret = fip_parse_buf(fip, buf, size, NULL);
> +	if (ret)
> +		goto err;
> +
> +	close(fd);
> +
> +	return fip;
> +err:
> +	if (fip)
> +		fip_free(fip);
> +	close(fd);
> +	free(buf);
> +
> +	return ERR_PTR(ret);
> +}
> +
>  static int load_fip(const char *filename, off_t offset)
>  {
>  	struct fip_state *fip;
> @@ -251,7 +309,11 @@ static int load_fip(const char *filename, off_t offset)
>  	unsigned char shasum[SHA256_DIGEST_SIZE];
>  	int ret;
>  
> -	fip = fip_image_open(filename, offset);
> +	if (IS_ENABLED(CONFIG_ARCH_K3_AUTHENTICATE_IMAGE))
> +		fip = fip_image_load_auth(filename, offset);
> +	else
> +		fip = fip_image_open(filename, offset);
> +
>  	if (IS_ERR(fip)) {
>  		pr_err("Cannot open FIP image: %pe\n", fip);
>  		return PTR_ERR(fip);
> 
> -- 
> 2.39.5
> 
> 
> 

Re: [PATCH 11/13] ARM: k3: r5: Allow to authenticate next image by ROM API

[Sascha Hauer]

On Mon, Mar 10, 2025 at 08:26:01PM +0100, Marco Felsch wrote:
> On 25-02-28, Sascha Hauer wrote:
> > This adds the Kconfig option CONFIG_ARCH_K3_AUTHENTICATE_IMAGE. When
> > enabled, the full barebox image will only be started when it can be
> > authenticated using the ROM API.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  arch/arm/mach-k3/Kconfig |  7 ++++++
> >  arch/arm/mach-k3/r5.c    | 64 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 70 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
> > index 37d5155577..e93e3154c8 100644
> > --- a/arch/arm/mach-k3/Kconfig
> > +++ b/arch/arm/mach-k3/Kconfig
> > @@ -37,6 +37,13 @@ config MACH_BEAGLEPLAY
> >  	help
> >  	  Say Y here if you are using a TI AM62x based BeaglePlay board
> >  
> > +config ARCH_K3_AUTHENTICATE_IMAGE
> > +	bool "Force authentication of FIP image against ROM API"
> > +	help
> > +	  By enabling this option the FIP image loaded by the first stage
> > +	  will be authenticated against the K3 ROM API. Images which fail
> > +	  to authenticate will not be started.
> 
> The ROM is checking only the sha sum or does the ROM also have some kind
> of signature-checking (pub/priv keys requried)?

The ROM cheks against signatures

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 |

[PATCH 12/13] scripts/k3img: remove temporary files

[Sascha Hauer]

Remove temporary files when exiting the script. While at it add a "set
-e" to exit the script when a command fails.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 scripts/k3img | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/k3img b/scripts/k3img
index 048da82b92..a514852fcd 100755
--- a/scripts/k3img
+++ b/scripts/k3img
@@ -1,5 +1,7 @@
 #!/bin/bash
 
+set -e
+
 TEMP=$(getopt -o '' --long 'sysfw:,sysfwdata:,dmdata:,out:,sbl:,key:,innerdata:' -n 'k3img' -- "$@")
 
 if [ $? -ne 0 ]; then
@@ -76,8 +78,11 @@ dmdatasize=$(stat -c%s $dmdata)
 
 total=$(($sblsize + $sysfwsize + $sysfwdatasize + $dmdatasize))
 
-certcfg=$(mktemp k3img.XXXXXXX)
-cert=$(mktemp k3img.XXXXXXX)
+TMPDIR="$(mktemp -d)"
+trap 'rm -rf -- "$TMPDIR"' EXIT
+
+certcfg=${TMPDIR}/certcfg
+cert=${TMPDIR}/cert
 
 num_comp=4
 

-- 
2.39.5

[PATCH 13/13] scripts: add k3sign

[Sascha Hauer]

This adds k3sign which is a small example script to generate a
certificate from an input file suitable for verification against the K3
ROM API.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 scripts/k3sign | 126 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/scripts/k3sign b/scripts/k3sign
new file mode 100755
index 0000000000..df66501eee
--- /dev/null
+++ b/scripts/k3sign
@@ -0,0 +1,126 @@
+#!/bin/bash
+
+set -e
+
+myname=${0##*/}
+
+usage() {
+	cat >&2 << EOL
+
+Sign an image suitable for authenticating with the K3 ROM API
+Usage:
+$myname options <INFILE>
+options:
+	--key <KEYFILE>	The key to sign the image with
+	--out <OUTFILE>	Write output to OUTFILE
+	--help		This help
+EOL
+	exit 1
+}
+
+TEMP=$(getopt -o '' --long 'out:,key:,help' -n 'k3img' -- "$@")
+
+if [ $? -ne 0 ]; then
+	echo 'Terminating...' >&2
+	exit 1
+fi
+
+# Note the quotes around "$TEMP": they are essential!
+eval set -- "$TEMP"
+unset TEMP
+
+while true; do
+        case "$1" in
+	'--out')
+		out="$2"
+		shift 2
+		continue
+	;;
+	'--key')
+		key="$2"
+		shift 2
+		continue
+	;;
+	'--help')
+		usage
+		continue
+	;;
+	'--')
+		shift
+		break
+	;;
+	*)
+		echo 'Internal error!' >&2
+		exit 1
+	;;
+	esac
+done
+
+if [ $# = 0 ]; then
+	echo "No input file given"
+	usage
+fi
+
+in=$1
+
+if [ -z "$out" ]; then
+	out=$in.cert
+fi
+
+if [ -z "$key" ]; then
+	echo "No key given (--key)"
+	exit 1
+fi
+
+filesha=$(sha512sum $in | sed 's/ .*//')
+filesize=$(stat -c%s $in)
+
+TMPDIR="$(mktemp -d)"
+trap 'rm -rf -- "$TMPDIR"' EXIT
+
+certcfg=${TMPDIR}/certcfg
+cert=${TMPDIR}/cert
+
+cat > $certcfg <<EndOfHereDocument
+[ req ]
+distinguished_name     = req_distinguished_name
+x509_extensions        = v3_ca
+prompt                 = no
+dirstring_type         = nobmp
+
+[ req_distinguished_name ]
+C                      = US
+ST                     = TX
+L                      = Dallas
+O                      = Texas Instruments Incorporated
+OU                     = Processors
+CN                     = TI Support
+emailAddress           = support@ti.com
+
+[ v3_ca ]
+basicConstraints       = CA:true
+1.3.6.1.4.1.294.1.3    = ASN1:SEQUENCE:swrv
+1.3.6.1.4.1.294.1.34   = ASN1:SEQUENCE:sysfw_image_integrity
+1.3.6.1.4.1.294.1.35   = ASN1:SEQUENCE:sysfw_image_load
+1.3.6.1.4.1.294.1.37   = ASN1:SEQUENCE:firewall
+
+[ swrv ]
+swrv = INTEGER:1
+
+[ sysfw_image_integrity ]
+shaType                = OID:2.16.840.1.101.3.4.2.3
+shaValue               = FORMAT:HEX,OCT:$filesha
+imageSize              = INTEGER:$filesize
+
+[ sysfw_image_load ]
+destAddr = FORMAT:HEX,OCT:00000000
+authInPlace = INTEGER:0x2
+
+[ firewall ]
+numFirewallRegions = INTEGER:0
+
+EndOfHereDocument
+
+openssl req -new -x509 -key $key -nodes -outform DER -out $cert -config $certcfg -sha512
+
+cat $cert $in > $out

-- 
2.39.5

Re: [PATCH 00/13] am625: support secure loading of full barebox

[Marco Felsch]

Hi Sascha,

On 25-02-28, Sascha Hauer wrote:
> On K3 SoCs only a small barebox is loaded by the ROM into SRAM. This
> barebox then loads the full barebox from SD/eMMC or USB DFU. In a secure
> boot environment the full barebox must be authenticated. This series
> implements two ways for accomplishing this.
> 
> First way is to utilize the ROM API to authenticate images. The other
> way is to compile a secure hash into the first stage binary and check
> if the full barebox image matches the hash. Using the ROM API means
> different first stage and second stage images can be combined whereas
> hashing binds specific builds together avoiding mix and match attacks.

before having a closer look on your patchset, do we really want to have
the 2nd case to be available? If we really want the 2nd case to be
available we should bound it to CONFIG_INSECURE (if not already done).

Regards,
  Marco

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> Sascha Hauer (13):
>       firmware: always generate sha256sum
>       firmware: add function to verify next image
>       ARM: k3: r5: drop loading of separate binaries
>       ARM: k3: r5: add proper error handling
>       fip: rework fip_image_open()
>       fip: fix wrong function call
>       fip: add function to calculate a sha256 over FIP image
>       ARM: am625: support hash verification of full barebox
>       ARM: k3: add support for authenticating images against the ROM API
>       ARM: k3: r5: delete fip image when it can't be opened
>       ARM: k3: r5: Allow to authenticate next image by ROM API
>       scripts/k3img: remove temporary files
>       scripts: add k3sign
> 
>  arch/arm/mach-k3/Kconfig  |  15 ++++
>  arch/arm/mach-k3/common.c |  99 ++++++++++++++++++++++
>  arch/arm/mach-k3/r5.c     | 206 +++++++++++++++++++++++++---------------------
>  firmware/Kconfig          |  23 ++++++
>  firmware/Makefile         |   8 +-
>  include/fiptool.h         |   3 +
>  include/firmware.h        |  28 +++++++
>  include/mach/k3/common.h  |   1 +
>  lib/fip.c                 | 101 ++++++++++++++---------
>  scripts/k3img             |   9 +-
>  scripts/k3sign            | 126 ++++++++++++++++++++++++++++
>  11 files changed, 478 insertions(+), 141 deletions(-)
> ---
> base-commit: 748ba0627681797b01a94be1b3f879ed2e52a361
> change-id: 20250228-am625-secure-49301f641738
> 
> Best regards,
> -- 
> Sascha Hauer <s.hauer@pengutronix.de>
> 
> 
> 

Re: [PATCH 00/13] am625: support secure loading of full barebox

[Sascha Hauer]

On Mon, Mar 10, 2025 at 06:40:58PM +0100, Marco Felsch wrote:
> Hi Sascha,
> 
> On 25-02-28, Sascha Hauer wrote:
> > On K3 SoCs only a small barebox is loaded by the ROM into SRAM. This
> > barebox then loads the full barebox from SD/eMMC or USB DFU. In a secure
> > boot environment the full barebox must be authenticated. This series
> > implements two ways for accomplishing this.
> > 
> > First way is to utilize the ROM API to authenticate images. The other
> > way is to compile a secure hash into the first stage binary and check
> > if the full barebox image matches the hash. Using the ROM API means
> > different first stage and second stage images can be combined whereas
> > hashing binds specific builds together avoiding mix and match attacks.
> 
> before having a closer look on your patchset, do we really want to have
> the 2nd case to be available?

Yes, as explained to avoid mix-and-match attacks.

> If we really want the 2nd case to be
> available we should bound it to CONFIG_INSECURE (if not already done).

Ok, will do.

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 |

Re: [PATCH 00/13] am625: support secure loading of full barebox

[Marco Felsch]

On 25-03-11, Sascha Hauer wrote:
> On Mon, Mar 10, 2025 at 06:40:58PM +0100, Marco Felsch wrote:
> > Hi Sascha,
> > 
> > On 25-02-28, Sascha Hauer wrote:
> > > On K3 SoCs only a small barebox is loaded by the ROM into SRAM. This
> > > barebox then loads the full barebox from SD/eMMC or USB DFU. In a secure
> > > boot environment the full barebox must be authenticated. This series
> > > implements two ways for accomplishing this.
> > > 
> > > First way is to utilize the ROM API to authenticate images. The other
> > > way is to compile a secure hash into the first stage binary and check
> > > if the full barebox image matches the hash. Using the ROM API means
> > > different first stage and second stage images can be combined whereas
> > > hashing binds specific builds together avoiding mix and match attacks.
> > 
> > before having a closer look on your patchset, do we really want to have
> > the 2nd case to be available?
> 
> Yes, as explained to avoid mix-and-match attacks.

Argh.. sorry, I meant the first case, the ROM API one. If the ROM API
allows mix-and-match attacks, we need to mark it as INSECURE. Sorry for
the confusion.

Regards,
  Marco

> > If we really want the 2nd case to be
> > available we should bound it to CONFIG_INSECURE (if not already done).
> 
> Ok, will do.
> 
> 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 |
> 

Re: [PATCH 00/13] am625: support secure loading of full barebox

[Sascha Hauer]

On Tue, Mar 11, 2025 at 09:48:33AM +0100, Marco Felsch wrote:
> On 25-03-11, Sascha Hauer wrote:
> > On Mon, Mar 10, 2025 at 06:40:58PM +0100, Marco Felsch wrote:
> > > Hi Sascha,
> > > 
> > > On 25-02-28, Sascha Hauer wrote:
> > > > On K3 SoCs only a small barebox is loaded by the ROM into SRAM. This
> > > > barebox then loads the full barebox from SD/eMMC or USB DFU. In a secure
> > > > boot environment the full barebox must be authenticated. This series
> > > > implements two ways for accomplishing this.
> > > > 
> > > > First way is to utilize the ROM API to authenticate images. The other
> > > > way is to compile a secure hash into the first stage binary and check
> > > > if the full barebox image matches the hash. Using the ROM API means
> > > > different first stage and second stage images can be combined whereas
> > > > hashing binds specific builds together avoiding mix and match attacks.
> > > 
> > > before having a closer look on your patchset, do we really want to have
> > > the 2nd case to be available?
> > 
> > Yes, as explained to avoid mix-and-match attacks.
> 
> Argh.. sorry, I meant the first case, the ROM API one. If the ROM API
> allows mix-and-match attacks, we need to mark it as INSECURE. Sorry for
> the confusion.

I wouldn't call using the ROM API insecure. In the end with the ROM API
the images are still signed and different people likely come to
different conclusions whether they want to sign or rather hash the
images.

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 |