[PATCH v2 0/4] Add support for booting EFI FIT images

[PATCH v2 0/4] Add support for booting EFI FIT images

[Cristian Ciocaltea]

Currently the only way to run an EFI binary like GRUB2 is via the
'bootefi' command, which cannot be used in a verified boot scenario.

The obvious solution to this limitation is to add support for
booting FIT images containing those EFI binaries.

The implementation relies on a new image type - IH_OS_EFI - which
can be created by using 'os = "efi"' inside an ITS file:

/ {
    #address-cells = <1>;

    images {
        efi-grub {
            description = "GRUB EFI";
            data = /incbin/("EFI/BOOT/bootarm.efi");
            type = "kernel_noload";
            arch = "arm";
            os = "efi";
            compression = "none";
            load = <0x0>;
            entry = <0x0>;
            hash-1 {
                algo = "sha256";
            };
        };
    };

    configurations {
        default = "config-grub";
        config-grub {
            kernel = "efi-grub";
            signature-1 {
                algo = "sha256,rsa2048";
                sign-images = "kernel";
            };
        };
    };
};

The bootm command has been extended to handle the IH_OS_EFI images.
To enable this feature, a new configuration option has been added:
BOOTM_EFI

I tested the solution using the 'qemu_arm' board:

=> load scsi 0:1 ${kernel_addr_r} efi-image.fit
=> bootm ${kernel_addr_r}#config-grub

Changes since v1:
 * Rebase patches on Heinrich Schuchardt's patch series:
   efi_loader: prepare for FIT images
   https://lists.denx.de/pipermail/u-boot/2019-December/393192.html
 * Add sample configuration: doc/uImage.FIT/uefi.its
 * Update uefi documentation: doc/uefi/uefi.rst

Cristian Ciocaltea (4):
  image: Add IH_OS_EFI for EFI chain-load boot
  bootm: Add a bootm command for type IH_OS_EFI
  doc: Add sample uefi.its image description file
  doc: uefi.rst: Document launching UEFI binaries from FIT images

 cmd/Kconfig             |  7 +++++
 common/bootm_os.c       | 61 +++++++++++++++++++++++++++++++++++++
 common/image-fit.c      |  3 +-
 common/image.c          |  1 +
 doc/uImage.FIT/uefi.its | 67 +++++++++++++++++++++++++++++++++++++++++
 doc/uefi/uefi.rst       | 34 +++++++++++++++++++++
 include/image.h         |  1 +
 7 files changed, 173 insertions(+), 1 deletion(-)
 create mode 100644 doc/uImage.FIT/uefi.its

-- 
2.17.1

[PATCH v2 1/4] image: Add IH_OS_EFI for EFI chain-load boot

[Cristian Ciocaltea]

Add a new OS type to be used for chain-loading an EFI compatible
firmware or boot loader like GRUB2, possibly in a verified boot
scenario.

Bellow is sample ITS file that generates a FIT image supporting
secure boot. Please note the presence of 'os = "efi";' line, which
identifies the currently introduced OS type:

/ {
    #address-cells = <1>;

    images {
        efi-grub {
            description = "GRUB EFI";
            data = /incbin/("EFI/BOOT/bootarm.efi");
            type = "kernel_noload";
            arch = "arm";
            os = "efi";
            compression = "none";
            load = <0x0>;
            entry = <0x0>;
            hash-1 {
                algo = "sha256";
            };
        };
    };

    configurations {
        default = "config-grub";
        config-grub {
            kernel = "efi-grub";
            signature-1 {
                algo = "sha256,rsa2048";
                sign-images = "kernel";
            };
        };
    };
};

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 common/image-fit.c | 3 ++-
 common/image.c     | 1 +
 include/image.h    | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index 5c63c769de..19e313bf41 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1925,7 +1925,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		image_type == IH_TYPE_FPGA ||
 		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
 		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
-		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
+		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
+		fit_image_check_os(fit, noffset, IH_OS_EFI);
 
 	/*
 	 * If either of the checks fail, we should report an error, but
diff --git a/common/image.c b/common/image.c
index f17fa40c49..2e0e2b0e7f 100644
--- a/common/image.c
+++ b/common/image.c
@@ -134,6 +134,7 @@ static const table_entry_t uimage_os[] = {
 	{	IH_OS_OPENRTOS,	"openrtos",	"OpenRTOS",		},
 #endif
 	{	IH_OS_OPENSBI,	"opensbi",	"RISC-V OpenSBI",	},
+	{	IH_OS_EFI,	"efi",		"EFI Firmware" },
 
 	{	-1,		"",		"",			},
 };
diff --git a/include/image.h b/include/image.h
index f4d2aaf53e..4a280b78e7 100644
--- a/include/image.h
+++ b/include/image.h
@@ -157,6 +157,7 @@ enum {
 	IH_OS_ARM_TRUSTED_FIRMWARE,     /* ARM Trusted Firmware */
 	IH_OS_TEE,			/* Trusted Execution Environment */
 	IH_OS_OPENSBI,			/* RISC-V OpenSBI */
+	IH_OS_EFI,			/* EFI Firmware (e.g. GRUB2) */
 
 	IH_OS_COUNT,
 };
-- 
2.17.1

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Cristian Ciocaltea]

Add support for booting EFI binaries contained in FIT images.
A typical usage scenario is chain-loading GRUB2 in a verified
boot environment.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 cmd/Kconfig       |  7 ++++++
 common/bootm_os.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index cf982ff65e..39fa87082d 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -263,6 +263,13 @@ config CMD_BOOTI
 	help
 	  Boot an AArch64 Linux Kernel image from memory.
 
+config BOOTM_EFI
+	bool "Support booting EFI OS images"
+	depends on CMD_BOOTEFI
+	default y
+	help
+	  Support booting EFI images via the bootm command.
+
 config BOOTM_LINUX
 	bool "Support booting Linux OS images"
 	depends on CMD_BOOTM || CMD_BOOTZ || CMD_BOOTI
diff --git a/common/bootm_os.c b/common/bootm_os.c
index 6fb7d658da..7aa76052b9 100644
--- a/common/bootm_os.c
+++ b/common/bootm_os.c
@@ -6,10 +6,12 @@
 
 #include <common.h>
 #include <bootm.h>
+#include <efi_loader.h>
 #include <env.h>
 #include <fdt_support.h>
 #include <linux/libfdt.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <vxworks.h>
 #include <tee/optee.h>
 
@@ -462,6 +464,62 @@ static int do_bootm_tee(int flag, int argc, char * const argv[],
 }
 #endif
 
+#ifdef CONFIG_BOOTM_EFI
+static int do_bootm_efi(int flag, int argc, char * const argv[],
+			bootm_headers_t *images)
+{
+	int ret;
+	efi_status_t efi_ret;
+	uintptr_t fdt_addr;
+	void *image_buf;
+
+	if (flag != BOOTM_STATE_OS_GO)
+		return 0;
+
+	/* Locate EFI binary and FDT, if provided */
+	ret = bootm_find_images(flag, argc, argv);
+	if (ret)
+		return ret;
+
+	/* Initialize EFI drivers */
+	efi_ret = efi_init_obj_list();
+	if (efi_ret != EFI_SUCCESS) {
+		printf("## Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       efi_ret & ~EFI_ERROR_MASK);
+		return 1;
+	}
+
+	/* Install device tree */
+	if (images->ft_len)
+		fdt_addr = (uintptr_t)images->ft_addr;
+	else
+		fdt_addr = EFI_FDT_USE_INTERNAL;
+
+	efi_ret = efi_install_fdt(fdt_addr);
+	if (efi_ret != EFI_SUCCESS) {
+		printf("## Error: Cannot install device tree, r = %lu\n",
+		       efi_ret & ~EFI_ERROR_MASK);
+		return 1;
+	}
+
+	/* Run EFI image */
+	printf("## Transferring control to EFI (at address %08lx) ...\n",
+	       images->ep);
+	bootstage_mark(BOOTSTAGE_ID_RUN_OS);
+
+	image_buf = map_sysmem(images->ep, images->os.image_len);
+
+	efi_ret = efi_run_image(image_buf, images->os.image_len);
+	if (efi_ret != EFI_SUCCESS) {
+		printf("## Error: Cannot run EFI image, r = %lu\n",
+		       efi_ret & ~EFI_ERROR_MASK);
+		return 1;
+	}
+
+	return 0;
+}
+#endif
+
 static boot_os_fn *boot_os[] = {
 	[IH_OS_U_BOOT] = do_bootm_standalone,
 #ifdef CONFIG_BOOTM_LINUX
@@ -498,6 +556,9 @@ static boot_os_fn *boot_os[] = {
 #ifdef CONFIG_BOOTM_OPTEE
 	[IH_OS_TEE] = do_bootm_tee,
 #endif
+#ifdef CONFIG_BOOTM_EFI
+	[IH_OS_EFI] = do_bootm_efi,
+#endif
 };
 
 /* Allow for arch specific config before we boot */
-- 
2.17.1

[PATCH v2 3/4] doc: Add sample uefi.its image description file

[Cristian Ciocaltea]

This patch adds an example FIT image description file demonstrating
the usage of bootm command to securely launch UEFI binaries.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 doc/uImage.FIT/uefi.its | 67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 doc/uImage.FIT/uefi.its

diff --git a/doc/uImage.FIT/uefi.its b/doc/uImage.FIT/uefi.its
new file mode 100644
index 0000000000..e336ad938d
--- /dev/null
+++ b/doc/uImage.FIT/uefi.its
@@ -0,0 +1,67 @@
+/*
+ * Example FIT image description file demonstrating the usage of
+ * bootm command to launch UEFI binaries.
+ *
+ * Two boot configurations are available to enable booting GRUB2 on QEMU,
+ * the former uses a FDT blob contained in the FIT image, while the later
+ * relies on the FDT provided by the board emulator.
+ */
+
+/dts-v1/;
+
+/ {
+	description = "GRUB2 EFI and QEMU FDT blob";
+	#address-cells = <1>;
+
+	images {
+		efi-grub {
+			description = "GRUB EFI Firmware";
+			data = /incbin/("efi-part/EFI/BOOT/bootarm.efi");
+			type = "kernel_noload";
+			arch = "arm";
+			os = "efi";
+			compression = "none";
+			load = <0x0>;
+			entry = <0x0>;
+			hash-1 {
+				algo = "sha256";
+			};
+		};
+
+		fdt-qemu {
+			description = "QEMU DTB";
+			data = /incbin/("qemu-arm.dtb");
+			type = "flat_dt";
+			arch = "arm";
+			compression = "none";
+			hash-1 {
+				algo = "sha256";
+			};
+		};
+	};
+
+	configurations {
+		default = "config-grub-fdt";
+
+		config-grub-fdt {
+			description = "GRUB EFI Boot with FDT";
+			kernel = "efi-grub";
+			fdt = "fdt-qemu";
+			signature-1 {
+				algo = "sha256,rsa2048";
+				key-name-hint = "dev";
+				sign-images = "kernel", "fdt";
+			};
+		};
+
+		config-grub-nofdt {
+			description = "GRUB EFI Boot w/o FDT";
+			kernel = "efi-grub";
+			signature-1 {
+				algo = "sha256,rsa2048";
+				key-name-hint = "dev";
+				sign-images = "kernel";
+			};
+		};
+	};
+};
-- 
2.17.1

[PATCH v2 4/4] doc: uefi.rst: Document launching UEFI binaries from FIT images

[Cristian Ciocaltea]

This patch adds a new section "Launching a UEFI binary from a FIT image"
documenting the usage of the CONFIG_BOOTM_EFI extension to bootm command
that offers a secure boot alternative for UEFI binaries such as GRUB2.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 doc/uefi/uefi.rst | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
index db942df694..a8fd886d6b 100644
--- a/doc/uefi/uefi.rst
+++ b/doc/uefi/uefi.rst
@@ -63,6 +63,40 @@ The environment variable 'bootargs' is passed as load options in the UEFI system
 table. The Linux kernel EFI stub uses the load options as command line
 arguments.
 
+Launching a UEFI binary from a FIT image
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+A signed FIT image can be used to securely boot a UEFI image via the
+bootm command. This feature is available if U-Boot is configured with::
+
+    CONFIG_BOOTM_EFI=y
+
+A sample configuration is provided as file doc/uImage.FIT/uefi.its.
+
+Below you find the output of an example session starting GRUB::
+
+    => load mmc 0:1 ${kernel_addr_r} image.fit
+    4620426 bytes read in 83 ms (53.1 MiB/s)
+    => bootm ${kernel_addr_r}#config-grub-nofdt
+    ## Loading kernel from FIT Image at 40400000 ...
+       Using 'config-grub-nofdt' configuration
+       Verifying Hash Integrity ... sha256,rsa2048:dev+ OK
+       Trying 'efi-grub' kernel subimage
+         Description:  GRUB EFI Firmware
+         Created:      2019-11-20   8:18:16 UTC
+         Type:         Kernel Image (no loading done)
+         Compression:  uncompressed
+         Data Start:   0x404000d0
+         Data Size:    450560 Bytes = 440 KiB
+         Hash algo:    sha256
+         Hash value:   4dbee00021112df618f58b3f7cf5e1595533d543094064b9ce991e8b054a9eec
+       Verifying Hash Integrity ... sha256+ OK
+       XIP Kernel Image (no loading done)
+    ## Transferring control to EFI (at address 404000d0) ...
+    Welcome to GRUB!
+
+See doc/uImage.FIT/howto.txt for an introduction to FIT images.
+
 Executing the boot manager
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.17.1

[PATCH v2 4/4] doc: uefi.rst: Document launching UEFI binaries from FIT images

[Heinrich Schuchardt]

On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> This patch adds a new section "Launching a UEFI binary from a FIT image"
> documenting the usage of the CONFIG_BOOTM_EFI extension to bootm command
> that offers a secure boot alternative for UEFI binaries such as GRUB2.
>
> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH v2 1/4] image: Add IH_OS_EFI for EFI chain-load boot

[Heinrich Schuchardt]

On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> Add a new OS type to be used for chain-loading an EFI compatible
> firmware or boot loader like GRUB2, possibly in a verified boot
> scenario.
>
> Bellow is sample ITS file that generates a FIT image supporting
> secure boot. Please note the presence of 'os = "efi";' line, which
> identifies the currently introduced OS type:
>
> / {
>      #address-cells = <1>;
>
>      images {
>          efi-grub {
>              description = "GRUB EFI";
>              data = /incbin/("EFI/BOOT/bootarm.efi");

According to UEFI Spec 2.8 the default file name for 32 bit ARM is
BOOTARM.EFI. But GRUB calls the file grubarm.efi.

So shouldn't we use grubarm.efi here as filename?

You use EFI/BOOT as directory name. I think this path does not add
benefit to the example. The other *.its files also come without any
specific path.

Best regards

Heinrich

>              type = "kernel_noload";
>              arch = "arm";
>              os = "efi";
>              compression = "none";
>              load = <0x0>;
>              entry = <0x0>;
>              hash-1 {
>                  algo = "sha256";
>              };
>          };
>      };
>
>      configurations {
>          default = "config-grub";
>          config-grub {
>              kernel = "efi-grub";
>              signature-1 {
>                  algo = "sha256,rsa2048";
>                  sign-images = "kernel";
>              };
>          };
>      };
> };
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>   common/image-fit.c | 3 ++-
>   common/image.c     | 1 +
>   include/image.h    | 1 +
>   3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 5c63c769de..19e313bf41 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1925,7 +1925,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>   		image_type == IH_TYPE_FPGA ||
>   		fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
>   		fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
> -		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
> +		fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
> +		fit_image_check_os(fit, noffset, IH_OS_EFI);
>
>   	/*
>   	 * If either of the checks fail, we should report an error, but
> diff --git a/common/image.c b/common/image.c
> index f17fa40c49..2e0e2b0e7f 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -134,6 +134,7 @@ static const table_entry_t uimage_os[] = {
>   	{	IH_OS_OPENRTOS,	"openrtos",	"OpenRTOS",		},
>   #endif
>   	{	IH_OS_OPENSBI,	"opensbi",	"RISC-V OpenSBI",	},
> +	{	IH_OS_EFI,	"efi",		"EFI Firmware" },
>
>   	{	-1,		"",		"",			},
>   };
> diff --git a/include/image.h b/include/image.h
> index f4d2aaf53e..4a280b78e7 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -157,6 +157,7 @@ enum {
>   	IH_OS_ARM_TRUSTED_FIRMWARE,     /* ARM Trusted Firmware */
>   	IH_OS_TEE,			/* Trusted Execution Environment */
>   	IH_OS_OPENSBI,			/* RISC-V OpenSBI */
> +	IH_OS_EFI,			/* EFI Firmware (e.g. GRUB2) */
>
>   	IH_OS_COUNT,
>   };
>

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt]

On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> Add support for booting EFI binaries contained in FIT images.
> A typical usage scenario is chain-loading GRUB2 in a verified
> boot environment.
>
> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>

Reading through the code it looks good. What I really need to do is
analyze the address usage on the sandbox. To me it is unclear if
images->fdt_addr is a physical address or an address in the address
space of the sandbox.

Did you test this on the sandbox? You can use
lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
command for loading the FIT image.

Shouldn't we add booting a UEFI FIT image to the Python test in
test/py/tests/test_fit.py?

doc/uImage.FIT/signature.txt describes that several properties of the
RSA public key should be stored in the control device tree.
Unfortunately no example is supplied in which format they should be
stored. Could you send me an example, please.

I found the following

https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb

Is this an accurate description? Or how do you get the parameters from
your RSA public key?

Best regards

Heinrich

[PATCH v2 1/4] image: Add IH_OS_EFI for EFI chain-load boot

[Peter Robinson]

On Tue, Dec 10, 2019 at 6:30 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> > Add a new OS type to be used for chain-loading an EFI compatible
> > firmware or boot loader like GRUB2, possibly in a verified boot
> > scenario.
> >
> > Bellow is sample ITS file that generates a FIT image supporting
> > secure boot. Please note the presence of 'os = "efi";' line, which
> > identifies the currently introduced OS type:
> >
> > / {
> >      #address-cells = <1>;
> >
> >      images {
> >          efi-grub {
> >              description = "GRUB EFI";
> >              data = /incbin/("EFI/BOOT/bootarm.efi");
>
> According to UEFI Spec 2.8 the default file name for 32 bit ARM is
> BOOTARM.EFI. But GRUB calls the file grubarm.efi.

In Linux the boot<arch>.efi file is provided by shim [1] and used for
secure boot etc, I believe the default is also the fall back boot
method. I'm unaware of shim currently being built for armv7.

[1] https://github.com/rhboot/shim/

> So shouldn't we use grubarm.efi here as filename?
>
> You use EFI/BOOT as directory name. I think this path does not add
> benefit to the example. The other *.its files also come without any
> specific path.
>
> Best regards
>
> Heinrich
>
> >              type = "kernel_noload";
> >              arch = "arm";
> >              os = "efi";
> >              compression = "none";
> >              load = <0x0>;
> >              entry = <0x0>;
> >              hash-1 {
> >                  algo = "sha256";
> >              };
> >          };
> >      };
> >
> >      configurations {
> >          default = "config-grub";
> >          config-grub {
> >              kernel = "efi-grub";
> >              signature-1 {
> >                  algo = "sha256,rsa2048";
> >                  sign-images = "kernel";
> >              };
> >          };
> >      };
> > };
> >
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >   common/image-fit.c | 3 ++-
> >   common/image.c     | 1 +
> >   include/image.h    | 1 +
> >   3 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/image-fit.c b/common/image-fit.c
> > index 5c63c769de..19e313bf41 100644
> > --- a/common/image-fit.c
> > +++ b/common/image-fit.c
> > @@ -1925,7 +1925,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> >               image_type == IH_TYPE_FPGA ||
> >               fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
> >               fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
> > -             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
> > +             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
> > +             fit_image_check_os(fit, noffset, IH_OS_EFI);
> >
> >       /*
> >        * If either of the checks fail, we should report an error, but
> > diff --git a/common/image.c b/common/image.c
> > index f17fa40c49..2e0e2b0e7f 100644
> > --- a/common/image.c
> > +++ b/common/image.c
> > @@ -134,6 +134,7 @@ static const table_entry_t uimage_os[] = {
> >       {       IH_OS_OPENRTOS, "openrtos",     "OpenRTOS",             },
> >   #endif
> >       {       IH_OS_OPENSBI,  "opensbi",      "RISC-V OpenSBI",       },
> > +     {       IH_OS_EFI,      "efi",          "EFI Firmware" },
> >
> >       {       -1,             "",             "",                     },
> >   };
> > diff --git a/include/image.h b/include/image.h
> > index f4d2aaf53e..4a280b78e7 100644
> > --- a/include/image.h
> > +++ b/include/image.h
> > @@ -157,6 +157,7 @@ enum {
> >       IH_OS_ARM_TRUSTED_FIRMWARE,     /* ARM Trusted Firmware */
> >       IH_OS_TEE,                      /* Trusted Execution Environment */
> >       IH_OS_OPENSBI,                  /* RISC-V OpenSBI */
> > +     IH_OS_EFI,                      /* EFI Firmware (e.g. GRUB2) */
> >
> >       IH_OS_COUNT,
> >   };
> >
>

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Cristian Ciocaltea]

On Tue, Dec 10, 2019 at 08:32:17PM +0100, Heinrich Schuchardt wrote:
> On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> > Add support for booting EFI binaries contained in FIT images.
> > A typical usage scenario is chain-loading GRUB2 in a verified
> > boot environment.
> > 
> > Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>
> 
> Reading through the code it looks good. What I really need to do is
> analyze the address usage on the sandbox. To me it is unclear if
> images->fdt_addr is a physical address or an address in the address
> space of the sandbox.
> 
> Did you test this on the sandbox? You can use
> lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
> command for loading the FIT image.

I only tested on qemu, I've never used the sandbox, so it's a good
opportunity to give it a try.

> Shouldn't we add booting a UEFI FIT image to the Python test in
> test/py/tests/test_fit.py?

Unfortunately I'm not familiar with the testing framework (including
Python scripting), but I'll do my best to add such a test.

> doc/uImage.FIT/signature.txt describes that several properties of the
> RSA public key should be stored in the control device tree.
> Unfortunately no example is supplied in which format they should be
> stored. Could you send me an example, please.
> 
> I found the following
> 
> https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb
> 
> Is this an accurate description? Or how do you get the parameters from
> your RSA public key?

My test scenario involves the following steps:

1. Create a public/private key pair
$ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
        -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537

2. Create a certificate containing the public key
$ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}

3. Dump QEMU virt board DTB
$ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
        -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]

4. Create (unsigned) FIT image and put the public key into DTB, with
   the 'required' property set, telling U-Boot that this key MUST be
   verified for the image to be valid
$ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}

5. Sign the FIT image
$ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}

6. Run QEMU supplying the DTB containing the public key and the
   u-boot binary built with CONFIG_OF_BOARD
$ qemu-system-arm -nographic \
    -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
    -dtb ${BOARD_DTB} [...]

This is what I get after booting QEMU with the command above:

=> fdt addr $fdtcontroladdr
=> fdt print
/ {
    [...]
	signature {
		key-dev {
			required = "conf";
			algo = "sha256,rsa2048";
			rsa,r-squared = * 0x5ef05188 [0x00000100];
			rsa,modulus = * 0x5ef05294 [0x00000100];
			rsa,exponent = <0x00000000 0x00010001>;
			rsa,n0-inverse = <0x649cd557>;
			rsa,num-bits = <0x00000800>;
			key-name-hint = "dev";
		};
	};
    [...]

> Best regards
> 
> Heinrich

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt]

On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
> On Tue, Dec 10, 2019 at 08:32:17PM +0100, Heinrich Schuchardt wrote:
>> On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
>>> Add support for booting EFI binaries contained in FIT images.
>>> A typical usage scenario is chain-loading GRUB2 in a verified
>>> boot environment.
>>>
>>> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>
>>
>> Reading through the code it looks good. What I really need to do is
>> analyze the address usage on the sandbox. To me it is unclear if
>> images->fdt_addr is a physical address or an address in the address
>> space of the sandbox.
>>
>> Did you test this on the sandbox? You can use
>> lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
>> command for loading the FIT image.
>
> I only tested on qemu, I've never used the sandbox, so it's a good
> opportunity to give it a try.
>
>> Shouldn't we add booting a UEFI FIT image to the Python test in
>> test/py/tests/test_fit.py?
>
> Unfortunately I'm not familiar with the testing framework (including
> Python scripting), but I'll do my best to add such a test.
>
>> doc/uImage.FIT/signature.txt describes that several properties of the
>> RSA public key should be stored in the control device tree.
>> Unfortunately no example is supplied in which format they should be
>> stored. Could you send me an example, please.
>>
>> I found the following
>>
>> https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb
>>
>> Is this an accurate description? Or how do you get the parameters from
>> your RSA public key?
>
> My test scenario involves the following steps:
>
> 1. Create a public/private key pair
> $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
>          -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
>
> 2. Create a certificate containing the public key
> $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
>
> 3. Dump QEMU virt board DTB
> $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
>          -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
>
> 4. Create (unsigned) FIT image and put the public key into DTB, with
>     the 'required' property set, telling U-Boot that this key MUST be
>     verified for the image to be valid
> $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
>
> 5. Sign the FIT image
> $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
>
> 6. Run QEMU supplying the DTB containing the public key and the
>     u-boot binary built with CONFIG_OF_BOARD
> $ qemu-system-arm -nographic \
>      -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
>      -dtb ${BOARD_DTB} [...]
>
> This is what I get after booting QEMU with the command above:
>
> => fdt addr $fdtcontroladdr
> => fdt print
> / {
>      [...]
> 	signature {
> 		key-dev {
> 			required = "conf";
> 			algo = "sha256,rsa2048";
> 			rsa,r-squared = * 0x5ef05188 [0x00000100];
> 			rsa,modulus = * 0x5ef05294 [0x00000100];
> 			rsa,exponent = <0x00000000 0x00010001>;
> 			rsa,n0-inverse = <0x649cd557>;
> 			rsa,num-bits = <0x00000800>;
> 			key-name-hint = "dev";
> 		};
> 	};
>      [...]

See my patch

doc: fitImage: example of a signature node
https://lists.denx.de/pipermail/u-boot/2019-December/393613.html

---

Booting of the sandbox fails due to an incorrect address passed for the
device tree:

=> host load hostfs - $kernel_addr_r image.fit
26558 bytes read in 0 ms
=> bootm ${kernel_addr_r}#config-grub-fdt
## Loading kernel from FIT Image at 01000000 ...
    Using 'config-grub-fdt' configuration
    Verifying Hash Integrity ... OK
    Trying 'helloworld' kernel subimage
      Description:  Hello World
      Created:      2019-12-11   9:19:22 UTC
      Type:         Kernel Image (no loading done)
      Compression:  uncompressed
      Data Start:   0x010000cc
      Data Size:    2733 Bytes = 2.7 KiB
      Hash algo:    sha256
      Hash value:
5c3ba35a1cb4abfe8a867ea6ac709574535794a7d7d03ba1ec2273b956d13983
    Verifying Hash Integrity ... sha256+ OK
    XIP Kernel Image (no loading done)
## Loading fdt from FIT Image at 01000000 ...
    Using 'config-grub-fdt' configuration
    Verifying Hash Integrity ... OK
    Trying 'fdt-test' fdt subimage
      Description:  QEMU DTB
      Created:      2019-12-11   9:19:22 UTC
      Type:         Flat Device Tree
      Compression:  uncompressed
      Data Start:   0x01000c74
      Data Size:    19713 Bytes = 19.3 KiB
      Architecture: ARM
      Hash algo:    sha256
      Hash value:
3e4f4e2b512f7a03a7f9ccecfb2b6bf7ceea75882370639460fd61502d903cd1
    Verifying Hash Integrity ... sha256+ OK
    Booting using the fdt blob at 0x1000c74
Found 0 disks
phys_to_virt: Cannot map sandbox address 11001c74 (SDRAM from 0 to 8000000)
Aborted

Please, check both the FDT and the non-FDT case on the sandbox and
resubmit the patch.

Best regards

Heinrich

[PATCH v2 1/4] image: Add IH_OS_EFI for EFI chain-load boot

[Cristian Ciocaltea]

On Tue, Dec 10, 2019 at 10:49:09PM +0000, Peter Robinson wrote:
> On Tue, Dec 10, 2019 at 6:30 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> > > Add a new OS type to be used for chain-loading an EFI compatible
> > > firmware or boot loader like GRUB2, possibly in a verified boot
> > > scenario.
> > >
> > > Bellow is sample ITS file that generates a FIT image supporting
> > > secure boot. Please note the presence of 'os = "efi";' line, which
> > > identifies the currently introduced OS type:
> > >
> > > / {
> > >      #address-cells = <1>;
> > >
> > >      images {
> > >          efi-grub {
> > >              description = "GRUB EFI";
> > >              data = /incbin/("EFI/BOOT/bootarm.efi");
> >
> > According to UEFI Spec 2.8 the default file name for 32 bit ARM is
> > BOOTARM.EFI. But GRUB calls the file grubarm.efi.
> 
> In Linux the boot<arch>.efi file is provided by shim [1] and used for
> secure boot etc, I believe the default is also the fall back boot
> method. I'm unaware of shim currently being built for armv7.
> 
> [1] https://github.com/rhboot/shim/
> 
> > So shouldn't we use grubarm.efi here as filename?

My build platform relies on buildroot and that is the default path
where the GRUB EFI binary is deployed. I don't know the reasons behind,
but most probably they are related to portability/compatibility,
as Peter already pointed out.

> > You use EFI/BOOT as directory name. I think this path does not add
> > benefit to the example. The other *.its files also come without any
> > specific path.

Totally agree, I will remove the directory path.

> > Best regards
> >
> > Heinrich
> >
> > >              type = "kernel_noload";
> > >              arch = "arm";
> > >              os = "efi";
> > >              compression = "none";
> > >              load = <0x0>;
> > >              entry = <0x0>;
> > >              hash-1 {
> > >                  algo = "sha256";
> > >              };
> > >          };
> > >      };
> > >
> > >      configurations {
> > >          default = "config-grub";
> > >          config-grub {
> > >              kernel = "efi-grub";
> > >              signature-1 {
> > >                  algo = "sha256,rsa2048";
> > >                  sign-images = "kernel";
> > >              };
> > >          };
> > >      };
> > > };
> > >
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > > ---
> > >   common/image-fit.c | 3 ++-
> > >   common/image.c     | 1 +
> > >   include/image.h    | 1 +
> > >   3 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/common/image-fit.c b/common/image-fit.c
> > > index 5c63c769de..19e313bf41 100644
> > > --- a/common/image-fit.c
> > > +++ b/common/image-fit.c
> > > @@ -1925,7 +1925,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
> > >               image_type == IH_TYPE_FPGA ||
> > >               fit_image_check_os(fit, noffset, IH_OS_LINUX) ||
> > >               fit_image_check_os(fit, noffset, IH_OS_U_BOOT) ||
> > > -             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
> > > +             fit_image_check_os(fit, noffset, IH_OS_OPENRTOS) ||
> > > +             fit_image_check_os(fit, noffset, IH_OS_EFI);
> > >
> > >       /*
> > >        * If either of the checks fail, we should report an error, but
> > > diff --git a/common/image.c b/common/image.c
> > > index f17fa40c49..2e0e2b0e7f 100644
> > > --- a/common/image.c
> > > +++ b/common/image.c
> > > @@ -134,6 +134,7 @@ static const table_entry_t uimage_os[] = {
> > >       {       IH_OS_OPENRTOS, "openrtos",     "OpenRTOS",             },
> > >   #endif
> > >       {       IH_OS_OPENSBI,  "opensbi",      "RISC-V OpenSBI",       },
> > > +     {       IH_OS_EFI,      "efi",          "EFI Firmware" },
> > >
> > >       {       -1,             "",             "",                     },
> > >   };
> > > diff --git a/include/image.h b/include/image.h
> > > index f4d2aaf53e..4a280b78e7 100644
> > > --- a/include/image.h
> > > +++ b/include/image.h
> > > @@ -157,6 +157,7 @@ enum {
> > >       IH_OS_ARM_TRUSTED_FIRMWARE,     /* ARM Trusted Firmware */
> > >       IH_OS_TEE,                      /* Trusted Execution Environment */
> > >       IH_OS_OPENSBI,                  /* RISC-V OpenSBI */
> > > +     IH_OS_EFI,                      /* EFI Firmware (e.g. GRUB2) */
> > >
> > >       IH_OS_COUNT,
> > >   };
> > >
> >

[PATCH v2 3/4] doc: Add sample uefi.its image description file

[Heinrich Schuchardt]

On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> This patch adds an example FIT image description file demonstrating
> the usage of bootm command to securely launch UEFI binaries.
>
> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt]

On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
> 1. Create a public/private key pair
> $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
>          -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
>
> 2. Create a certificate containing the public key
> $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
>
> 3. Dump QEMU virt board DTB
> $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
>          -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
>
> 4. Create (unsigned) FIT image and put the public key into DTB, with
>     the 'required' property set, telling U-Boot that this key MUST be
>     verified for the image to be valid
> $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
>
> 5. Sign the FIT image
> $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}

Thanks for the description

tools/fit_check_sign does not change any file. The signature is added in
step 4.

What seems to be missing in the U-Boot build system is the capability to
specify a public key in the configuation file to automatically include
the public key in the generated dtbs similar to Linux's
CONFIG_SYSTEM_TRUSTED_KEYS.

Best regards

Heinrich

>
> 6. Run QEMU supplying the DTB containing the public key and the
>     u-boot binary built with CONFIG_OF_BOARD
> $ qemu-system-arm -nographic \
>      -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
>      -dtb ${BOARD_DTB} [...]

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Cristian Ciocaltea]

On Wed, Dec 11, 2019 at 11:13:28AM +0100, Heinrich Schuchardt wrote:
> On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
> > 1. Create a public/private key pair
> > $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
> >          -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
> > 
> > 2. Create a certificate containing the public key
> > $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
> > 
> > 3. Dump QEMU virt board DTB
> > $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
> >          -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
> > 
> > 4. Create (unsigned) FIT image and put the public key into DTB, with
> >     the 'required' property set, telling U-Boot that this key MUST be
> >     verified for the image to be valid
> > $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
> > 
> > 5. Sign the FIT image
> > $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
> 
> Thanks for the description
> 
> tools/fit_check_sign does not change any file. The signature is added in
> step 4.

You are right, I've taken the commands from a script I use to automate
the whole procedure and I've just missed the verification step.

> What seems to be missing in the U-Boot build system is the capability to
> specify a public key in the configuation file to automatically include
> the public key in the generated dtbs similar to Linux's
> CONFIG_SYSTEM_TRUSTED_KEYS.

That would be a nice addition. Currently it is only possible to pass
the 'EXT_DTB' parameter to 'make' in order to provide the path to an
external DTB file to be put in the U-Boot image.

> Best regards
> 
> Heinrich
> 
> > 
> > 6. Run QEMU supplying the DTB containing the public key and the
> >     u-boot binary built with CONFIG_OF_BOARD
> > $ qemu-system-arm -nographic \
> >      -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
> >      -dtb ${BOARD_DTB} [...]
> 

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt]

On 12/11/19 12:36 PM, Cristian Ciocaltea wrote:
> On Wed, Dec 11, 2019 at 11:13:28AM +0100, Heinrich Schuchardt wrote:
>> On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
>>> 1. Create a public/private key pair
>>> $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
>>>           -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
>>>
>>> 2. Create a certificate containing the public key
>>> $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
>>>
>>> 3. Dump QEMU virt board DTB
>>> $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
>>>           -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
>>>
>>> 4. Create (unsigned) FIT image and put the public key into DTB, with
>>>      the 'required' property set, telling U-Boot that this key MUST be
>>>      verified for the image to be valid
>>> $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
>>>
>>> 5. Sign the FIT image
>>> $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
>>
>> Thanks for the description
>>
>> tools/fit_check_sign does not change any file. The signature is added in
>> step 4.
>
> You are right, I've taken the commands from a script I use to automate
> the whole procedure and I've just missed the verification step.
>
>> What seems to be missing in the U-Boot build system is the capability to
>> specify a public key in the configuation file to automatically include
>> the public key in the generated dtbs similar to Linux's
>> CONFIG_SYSTEM_TRUSTED_KEYS.
>
> That would be a nice addition. Currently it is only possible to pass
> the 'EXT_DTB' parameter to 'make' in order to provide the path to an
> external DTB file to be put in the U-Boot image.

I guess the first thing to do is to change mkimage such that we can add
a public key to a dtb without passing any kernel image:

     tools/mkimage -K filename.dtb -k keys

Currently this is not accepted by mkimage.

Next we can then integrate this command into the build process.

Best regards

Heinrich

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Cristian Ciocaltea]

On Wed, Dec 11, 2019 at 10:57:48AM +0100, Heinrich Schuchardt wrote:
> On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
> > On Tue, Dec 10, 2019 at 08:32:17PM +0100, Heinrich Schuchardt wrote:
> > > On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
> > > > Add support for booting EFI binaries contained in FIT images.
> > > > A typical usage scenario is chain-loading GRUB2 in a verified
> > > > boot environment.
> > > > 
> > > > Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>
> > > 
> > > Reading through the code it looks good. What I really need to do is
> > > analyze the address usage on the sandbox. To me it is unclear if
> > > images->fdt_addr is a physical address or an address in the address
> > > space of the sandbox.
> > > 
> > > Did you test this on the sandbox? You can use
> > > lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
> > > command for loading the FIT image.
> > 
> > I only tested on qemu, I've never used the sandbox, so it's a good
> > opportunity to give it a try.
> > 
> > > Shouldn't we add booting a UEFI FIT image to the Python test in
> > > test/py/tests/test_fit.py?
> > 
> > Unfortunately I'm not familiar with the testing framework (including
> > Python scripting), but I'll do my best to add such a test.
> > 
> > > doc/uImage.FIT/signature.txt describes that several properties of the
> > > RSA public key should be stored in the control device tree.
> > > Unfortunately no example is supplied in which format they should be
> > > stored. Could you send me an example, please.
> > > 
> > > I found the following
> > > 
> > > https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb
> > > 
> > > Is this an accurate description? Or how do you get the parameters from
> > > your RSA public key?
> > 
> > My test scenario involves the following steps:
> > 
> > 1. Create a public/private key pair
> > $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
> >          -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
> > 
> > 2. Create a certificate containing the public key
> > $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
> > 
> > 3. Dump QEMU virt board DTB
> > $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
> >          -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
> > 
> > 4. Create (unsigned) FIT image and put the public key into DTB, with
> >     the 'required' property set, telling U-Boot that this key MUST be
> >     verified for the image to be valid
> > $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
> > 
> > 5. Sign the FIT image
> > $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
> > 
> > 6. Run QEMU supplying the DTB containing the public key and the
> >     u-boot binary built with CONFIG_OF_BOARD
> > $ qemu-system-arm -nographic \
> >      -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
> >      -dtb ${BOARD_DTB} [...]
> > 
> > This is what I get after booting QEMU with the command above:
> > 
> > => fdt addr $fdtcontroladdr
> > => fdt print
> > / {
> >      [...]
> > 	signature {
> > 		key-dev {
> > 			required = "conf";
> > 			algo = "sha256,rsa2048";
> > 			rsa,r-squared = * 0x5ef05188 [0x00000100];
> > 			rsa,modulus = * 0x5ef05294 [0x00000100];
> > 			rsa,exponent = <0x00000000 0x00010001>;
> > 			rsa,n0-inverse = <0x649cd557>;
> > 			rsa,num-bits = <0x00000800>;
> > 			key-name-hint = "dev";
> > 		};
> > 	};
> >      [...]
> 
> See my patch
> 
> doc: fitImage: example of a signature node
> https://lists.denx.de/pipermail/u-boot/2019-December/393613.html
> 
> ---
> 
> Booting of the sandbox fails due to an incorrect address passed for the
> device tree:
> 
> => host load hostfs - $kernel_addr_r image.fit
> 26558 bytes read in 0 ms
> => bootm ${kernel_addr_r}#config-grub-fdt
> ## Loading kernel from FIT Image at 01000000 ...
>    Using 'config-grub-fdt' configuration
>    Verifying Hash Integrity ... OK
>    Trying 'helloworld' kernel subimage
>      Description:  Hello World
>      Created:      2019-12-11   9:19:22 UTC
>      Type:         Kernel Image (no loading done)
>      Compression:  uncompressed
>      Data Start:   0x010000cc
>      Data Size:    2733 Bytes = 2.7 KiB
>      Hash algo:    sha256
>      Hash value:
> 5c3ba35a1cb4abfe8a867ea6ac709574535794a7d7d03ba1ec2273b956d13983
>    Verifying Hash Integrity ... sha256+ OK
>    XIP Kernel Image (no loading done)
> ## Loading fdt from FIT Image at 01000000 ...
>    Using 'config-grub-fdt' configuration
>    Verifying Hash Integrity ... OK
>    Trying 'fdt-test' fdt subimage
>      Description:  QEMU DTB
>      Created:      2019-12-11   9:19:22 UTC
>      Type:         Flat Device Tree
>      Compression:  uncompressed
>      Data Start:   0x01000c74
>      Data Size:    19713 Bytes = 19.3 KiB
>      Architecture: ARM
>      Hash algo:    sha256
>      Hash value:
> 3e4f4e2b512f7a03a7f9ccecfb2b6bf7ceea75882370639460fd61502d903cd1
>    Verifying Hash Integrity ... sha256+ OK
>    Booting using the fdt blob at 0x1000c74
> Found 0 disks
> phys_to_virt: Cannot map sandbox address 11001c74 (SDRAM from 0 to 8000000)
> Aborted

I've checked the internal handling of the FDT images and it seems the
'ft_addr' attribute inside 'bootm_headers_t' structure points to a
mapped memory location. Since efi_install_fdt() assumes a physical
address (it calls map_sysmem() before accessing the data), it might be
enough to just reconvert 'ft_addr' via map_to_sysmem(), prior the call
to efi_install_fdt().

The issue is not present on ARM because the memory mapping utilities do
not actually perform any operations.

> Please, check both the FDT and the non-FDT case on the sandbox and
> resubmit the patch.

Yes, I'm going to (re)test on both QEMU and sandbox.

Should the upcoming patch set also include the suggested python test
or that can be added later as a separate patch?

Thanks.

> Best regards
> 
> Heinrich

[PATCH v2 2/4] bootm: Add a bootm command for type IH_OS_EFI

[Heinrich Schuchardt]

On 12/11/19 4:10 PM, Cristian Ciocaltea wrote:
> On Wed, Dec 11, 2019 at 10:57:48AM +0100, Heinrich Schuchardt wrote:
>> On 12/11/19 9:54 AM, Cristian Ciocaltea wrote:
>>> On Tue, Dec 10, 2019 at 08:32:17PM +0100, Heinrich Schuchardt wrote:
>>>> On 12/10/19 9:56 AM, Cristian Ciocaltea wrote:
>>>>> Add support for booting EFI binaries contained in FIT images.
>>>>> A typical usage scenario is chain-loading GRUB2 in a verified
>>>>> boot environment.
>>>>>
>>>>> Signed-off-by: Cristian Ciocaltea<cristian.ciocaltea@gmail.com>
>>>>
>>>> Reading through the code it looks good. What I really need to do is
>>>> analyze the address usage on the sandbox. To me it is unclear if
>>>> images->fdt_addr is a physical address or an address in the address
>>>> space of the sandbox.
>>>>
>>>> Did you test this on the sandbox? You can use
>>>> lib/efi_loader/helloworld.efi as a binary and the 'host load hostfs'
>>>> command for loading the FIT image.
>>>
>>> I only tested on qemu, I've never used the sandbox, so it's a good
>>> opportunity to give it a try.
>>>
>>>> Shouldn't we add booting a UEFI FIT image to the Python test in
>>>> test/py/tests/test_fit.py?
>>>
>>> Unfortunately I'm not familiar with the testing framework (including
>>> Python scripting), but I'll do my best to add such a test.
>>>
>>>> doc/uImage.FIT/signature.txt describes that several properties of the
>>>> RSA public key should be stored in the control device tree.
>>>> Unfortunately no example is supplied in which format they should be
>>>> stored. Could you send me an example, please.
>>>>
>>>> I found the following
>>>>
>>>> https://github.com/bn121rajesh/ipython-notebooks/blob/master/BehindTheScene/RSAPublicKeyParamsUBoot/rsa_public_key_params_uboot.ipynb
>>>>
>>>> Is this an accurate description? Or how do you get the parameters from
>>>> your RSA public key?
>>>
>>> My test scenario involves the following steps:
>>>
>>> 1. Create a public/private key pair
>>> $ openssl genpkey -algorithm RSA -out ${DEV_KEY} \
>>>           -pkeyopt rsa_keygen_bits:2048 -pkeyopt rsa_keygen_pubexp:65537
>>>
>>> 2. Create a certificate containing the public key
>>> $ openssl req -batch -new -x509 -key ${DEV_KEY} -out ${DEV_CRT}
>>>
>>> 3. Dump QEMU virt board DTB
>>> $ qemu-system-arm -nographic -M virt,dumpdtb=${BOARD_DTB} \
>>>           -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin [...]
>>>
>>> 4. Create (unsigned) FIT image and put the public key into DTB, with
>>>      the 'required' property set, telling U-Boot that this key MUST be
>>>      verified for the image to be valid
>>> $ mkimage -f ${FIT_ITS} -K ${BOARD_DTB} -k ${KEYS_DIR} -r ${FIT_IMG}
>>>
>>> 5. Sign the FIT image
>>> $ fit_check_sign -f ${FIT_IMG} -k ${BOARD_DTB}
>>>
>>> 6. Run QEMU supplying the DTB containing the public key and the
>>>      u-boot binary built with CONFIG_OF_BOARD
>>> $ qemu-system-arm -nographic \
>>>       -M virt -cpu cortex-a15 -smp 1 -m 512 -bios u-boot.bin \
>>>       -dtb ${BOARD_DTB} [...]
>>>
>>> This is what I get after booting QEMU with the command above:
>>>
>>> => fdt addr $fdtcontroladdr
>>> => fdt print
>>> / {
>>>       [...]
>>> 	signature {
>>> 		key-dev {
>>> 			required = "conf";
>>> 			algo = "sha256,rsa2048";
>>> 			rsa,r-squared = * 0x5ef05188 [0x00000100];
>>> 			rsa,modulus = * 0x5ef05294 [0x00000100];
>>> 			rsa,exponent = <0x00000000 0x00010001>;
>>> 			rsa,n0-inverse = <0x649cd557>;
>>> 			rsa,num-bits = <0x00000800>;
>>> 			key-name-hint = "dev";
>>> 		};
>>> 	};
>>>       [...]
>>
>> See my patch
>>
>> doc: fitImage: example of a signature node
>> https://lists.denx.de/pipermail/u-boot/2019-December/393613.html
>>
>> ---
>>
>> Booting of the sandbox fails due to an incorrect address passed for the
>> device tree:
>>
>> => host load hostfs - $kernel_addr_r image.fit
>> 26558 bytes read in 0 ms
>> => bootm ${kernel_addr_r}#config-grub-fdt
>> ## Loading kernel from FIT Image at 01000000 ...
>>     Using 'config-grub-fdt' configuration
>>     Verifying Hash Integrity ... OK
>>     Trying 'helloworld' kernel subimage
>>       Description:  Hello World
>>       Created:      2019-12-11   9:19:22 UTC
>>       Type:         Kernel Image (no loading done)
>>       Compression:  uncompressed
>>       Data Start:   0x010000cc
>>       Data Size:    2733 Bytes = 2.7 KiB
>>       Hash algo:    sha256
>>       Hash value:
>> 5c3ba35a1cb4abfe8a867ea6ac709574535794a7d7d03ba1ec2273b956d13983
>>     Verifying Hash Integrity ... sha256+ OK
>>     XIP Kernel Image (no loading done)
>> ## Loading fdt from FIT Image at 01000000 ...
>>     Using 'config-grub-fdt' configuration
>>     Verifying Hash Integrity ... OK
>>     Trying 'fdt-test' fdt subimage
>>       Description:  QEMU DTB
>>       Created:      2019-12-11   9:19:22 UTC
>>       Type:         Flat Device Tree
>>       Compression:  uncompressed
>>       Data Start:   0x01000c74
>>       Data Size:    19713 Bytes = 19.3 KiB
>>       Architecture: ARM
>>       Hash algo:    sha256
>>       Hash value:
>> 3e4f4e2b512f7a03a7f9ccecfb2b6bf7ceea75882370639460fd61502d903cd1
>>     Verifying Hash Integrity ... sha256+ OK
>>     Booting using the fdt blob at 0x1000c74
>> Found 0 disks
>> phys_to_virt: Cannot map sandbox address 11001c74 (SDRAM from 0 to 8000000)
>> Aborted
>
> I've checked the internal handling of the FDT images and it seems the
> 'ft_addr' attribute inside 'bootm_headers_t' structure points to a
> mapped memory location. Since efi_install_fdt() assumes a physical
> address (it calls map_sysmem() before accessing the data), it might be
> enough to just reconvert 'ft_addr' via map_to_sysmem(), prior the call
> to efi_install_fdt().

I will change my patch "efi_loader: export efi_install_fdt()" to expect
a pointer to addressable memory instead of a "physical" address. This
will avoid double conversions.

>
> The issue is not present on ARM because the memory mapping utilities do
> not actually perform any operations.
>
>> Please, check both the FDT and the non-FDT case on the sandbox and
>> resubmit the patch.
>
> Yes, I'm going to (re)test on both QEMU and sandbox.
>
> Should the upcoming patch set also include the suggested python test
> or that can be added later as a separate patch?

The patches will not be merged into U-Boot master before the v2020.01
release on January 6th. If you send a follow up patch until then, it is
also fine.

Best regards

Heinrich

>
> Thanks.
>
>> Best regards
>>
>> Heinrich
>