[PATCH 0/8] qemu: arm64: Add support for uefi firmware management protocol routines

[PATCH 0/8] qemu: arm64: Add support for uefi firmware management protocol routines

[Sughosh Ganu]

This series is based on the capsule update series sent by Takahiro
Akashi[1].

These routines have been tested on the qemu arm64 platform for
updating the u-boot firmware through the capsule-on-disk functionality
of the uefi spec. This series supports updating the u-boot binary
image on platforms booting from arm trusted firmware(tf-a), where the
u-boot image gets booted as the BL33 payload. Support has also been
added for authenticating the capsule file using the public key
contained in the root certificate stored as an efi variable.

The capsule file is placed on the efi system partition(esp), under the
EFI/UpdateCapsule directory. The BootNext and BootXXXX variables are
set accordingly by using the 'efidebug boot add' command. More details
added as part of the documentation patch.

[1] - https://lists.denx.de/pipermail/u-boot/2020-April/408760.html

Sughosh Ganu (8):
  semihosting: Change semihosting file operation functions into global
    symbols
  semihosting: Add support for writing to a file
  qemu: arm64: Add support for efi firmware management protocol routines
  efi_loader: Allow parsing of miscellaneous signature database
    variables
  efi_loader: Make the pkcs7 header parsing function an extern
  efi: capsule: Add support for uefi capsule authentication
  qemu: arm64: Add support for uefi capsule authentication
  qemu: arm64: Add documentation for capsule update

 arch/arm/lib/semihosting.c              |  48 ++++-
 board/emulation/qemu-arm/Kconfig        |  12 ++
 board/emulation/qemu-arm/Makefile       |   1 +
 board/emulation/qemu-arm/qemu_efi_fmp.c | 243 ++++++++++++++++++++++++
 doc/board/emulation/qemu-arm.rst        | 112 +++++++++++
 include/efi_api.h                       |  17 ++
 include/efi_loader.h                    |  13 +-
 include/semihosting.h                   |  14 ++
 lib/efi_loader/Kconfig                  |  16 ++
 lib/efi_loader/efi_capsule.c            | 126 ++++++++++++
 lib/efi_loader/efi_image_loader.c       |  11 +-
 lib/efi_loader/efi_signature.c          |  96 ++++++++--
 lib/efi_loader/efi_variable.c           |  91 +--------
 13 files changed, 692 insertions(+), 108 deletions(-)
 create mode 100644 board/emulation/qemu-arm/qemu_efi_fmp.c
 create mode 100644 include/semihosting.h

-- 
2.17.1

[PATCH 1/8] semihosting: Change semihosting file operation functions into global symbols

[Sughosh Ganu]

Change the semihosting file operation functions into external symbols
so that they can be called from outside the file. These functions
would be required to be called for implementing firmware update
functionality for the qemu arm64 platform.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 arch/arm/lib/semihosting.c |  7 ++++---
 include/semihosting.h      | 13 +++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)
 create mode 100644 include/semihosting.h

diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
index 2658026cf4..3aeda1303a 100644
--- a/arch/arm/lib/semihosting.c
+++ b/arch/arm/lib/semihosting.c
@@ -13,6 +13,7 @@
  */
 #include <common.h>
 #include <command.h>
+#include <semihosting.h>
 
 #define SYSOPEN		0x01
 #define SYSCLOSE	0x02
@@ -43,7 +44,7 @@ static noinline long smh_trap(unsigned int sysnum, void *addr)
  * Open a file on the host. Mode is "r" or "rb" currently. Returns a file
  * descriptor or -1 on error.
  */
-static long smh_open(const char *fname, char *modestr)
+long smh_open(const char *fname, char *modestr)
 {
 	long fd;
 	unsigned long mode;
@@ -82,7 +83,7 @@ static long smh_open(const char *fname, char *modestr)
 /*
  * Read 'len' bytes of file into 'memp'. Returns 0 on success, else failure
  */
-static long smh_read(long fd, void *memp, size_t len)
+long smh_read(long fd, void *memp, size_t len)
 {
 	long ret;
 	struct smh_read_s {
@@ -116,7 +117,7 @@ static long smh_read(long fd, void *memp, size_t len)
 /*
  * Close the file using the file descriptor
  */
-static long smh_close(long fd)
+long smh_close(long fd)
 {
 	long ret;
 
diff --git a/include/semihosting.h b/include/semihosting.h
new file mode 100644
index 0000000000..f1bf419275
--- /dev/null
+++ b/include/semihosting.h
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#if !defined _SEMIHOSTING_H_
+#define _SEMIHOSTING_H_
+
+long smh_open(const char *fname, char *modestr);
+long smh_read(long fd, void *memp, size_t len);
+long smh_close(long fd);
+
+#endif /* _SEMIHOSTING_H_ */
-- 
2.17.1

[PATCH 2/8] semihosting: Add support for writing to a file

[Sughosh Ganu]

Add a function to enable writing to a file. Currently, support is
added for writing to a binary file. This would be used for
implementing the firmware update functionality for the qemu arm64
platform.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 arch/arm/lib/semihosting.c | 41 ++++++++++++++++++++++++++++++++++++--
 include/semihosting.h      |  1 +
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
index 3aeda1303a..08181132d1 100644
--- a/arch/arm/lib/semihosting.c
+++ b/arch/arm/lib/semihosting.c
@@ -17,11 +17,18 @@
 
 #define SYSOPEN		0x01
 #define SYSCLOSE	0x02
+#define SYSWRITE	0x5
 #define SYSREAD		0x06
 #define SYSFLEN		0x0C
 
-#define MODE_READ	0x0
-#define MODE_READBIN	0x1
+#define MODE_READ		0x0
+#define MODE_READBIN		0x1
+#define MODE_READPLUS		0x2
+#define MODE_READPLUSBIN	0x3
+#define MODE_WRITE		0x4
+#define MODE_WRITEBIN		0x5
+#define MODE_WRITEPLUS		0x6
+#define MODE_WRITEPLUSBIN	0x7
 
 /*
  * Call the handler
@@ -61,6 +68,8 @@ long smh_open(const char *fname, char *modestr)
 		mode = MODE_READ;
 	} else if (!(strcmp(modestr, "rb"))) {
 		mode = MODE_READBIN;
+	} else if (!strcmp(modestr, "w+b")) {
+		mode = MODE_WRITEPLUSBIN;
 	} else {
 		printf("%s: ERROR mode \'%s\' not supported\n", __func__,
 		       modestr);
@@ -114,6 +123,34 @@ long smh_read(long fd, void *memp, size_t len)
 	return 0;
 }
 
+/*
+ * Write 'len' bytes into the file referenced by the fd. Returns 0 on success, else
+ * a negavite value for failure
+ */
+long smh_write(long fd, void *memp, size_t len)
+{
+	long ret;
+	struct smh_write_s {
+		long fd;
+		void *memp;
+		size_t len;
+	} write;
+
+	debug("%s: fd %ld, memp %p, len %zu\n", __func__, fd, memp, len);
+
+	write.fd = fd;
+	write.memp = memp;
+	write.len = len;
+
+	ret = smh_trap(SYSWRITE, &write);
+
+	if (ret > 0)
+		printf("%s: ERROR ret %ld, fd %ld, len %zu, memp %p\n",
+		       __func__, ret, fd, len, memp);
+
+	return ret == 0 ? 0 : -1;
+}
+
 /*
  * Close the file using the file descriptor
  */
diff --git a/include/semihosting.h b/include/semihosting.h
index f1bf419275..fa5cecddf2 100644
--- a/include/semihosting.h
+++ b/include/semihosting.h
@@ -8,6 +8,7 @@
 
 long smh_open(const char *fname, char *modestr);
 long smh_read(long fd, void *memp, size_t len);
+long smh_write(long fd, void *memp, size_t len);
 long smh_close(long fd);
 
 #endif /* _SEMIHOSTING_H_ */
-- 
2.17.1

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Sughosh Ganu]

Add support for the get_image_info and set_image routines, which are
part of the efi firmware management protocol.

The current implementation uses the set_image routine for updating the
u-boot binary image for the qemu arm64 platform. This is supported
using the capsule-on-disk feature of the uefi specification, wherein
the firmware image to be updated is placed on the efi system partition
as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
added for updating the u-boot image on platforms booting with arm
trusted firmware(tf-a), where the u-boot image gets booted as the BL33
payload(bl33.bin).

The feature can be enabled by the following config options

CONFIG_EFI_CAPSULE_ON_DISK=y
CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 board/emulation/qemu-arm/Kconfig        |  12 ++
 board/emulation/qemu-arm/Makefile       |   1 +
 board/emulation/qemu-arm/qemu_efi_fmp.c | 210 ++++++++++++++++++++++++
 3 files changed, 223 insertions(+)
 create mode 100644 board/emulation/qemu-arm/qemu_efi_fmp.c

diff --git a/board/emulation/qemu-arm/Kconfig b/board/emulation/qemu-arm/Kconfig
index 02ae4d9884..f1b2de8e41 100644
--- a/board/emulation/qemu-arm/Kconfig
+++ b/board/emulation/qemu-arm/Kconfig
@@ -11,3 +11,15 @@ config BOARD_SPECIFIC_OPTIONS # dummy
 	imply VIRTIO_BLK
 
 endif
+
+if TARGET_QEMU_ARM_64BIT && TFABOOT
+
+config EFI_FIRMWARE_MANAGEMENT_PROTOCOL
+	bool "EFI Firmware Management protocol for Qemu arm64 platform"
+	depends on EFI_CAPSULE_ON_DISK
+	default n
+	help
+	  Select this option for enabling firmware management protocol
+	  for qemu arm64 platform
+
+endif
diff --git a/board/emulation/qemu-arm/Makefile b/board/emulation/qemu-arm/Makefile
index a22d1237ff..c95ac6d233 100644
--- a/board/emulation/qemu-arm/Makefile
+++ b/board/emulation/qemu-arm/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0+
 
 obj-y	+= qemu-arm.o
+obj-$(CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL) += qemu_efi_fmp.o
diff --git a/board/emulation/qemu-arm/qemu_efi_fmp.c b/board/emulation/qemu-arm/qemu_efi_fmp.c
new file mode 100644
index 0000000000..9baea94e6c
--- /dev/null
+++ b/board/emulation/qemu-arm/qemu_efi_fmp.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#include <common.h>
+#include <charset.h>
+#include <efi_api.h>
+#include <efi_loader.h>
+#include <malloc.h>
+#include <semihosting.h>
+
+#define QEMU_UBOOT_IMAGE_INDEX	0x1
+#define QEMU_UBOOT_IMAGE	0x1
+
+#define UBOOT_FILE		"bl33.bin"
+
+#define EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID \
+	EFI_GUID(0xfb90808a, 0xba9a, 0x4d42, 0xb9, 0xa2, \
+		 0xa7, 0xa9, 0x37, 0x14, 0x4a, 0xee)
+
+/**
+ * qemu_arm64_fmp_get_image_info() - Implement get_image_info
+ *
+ * @this:			instance of the efi_firmware_management_protocol
+ * @image_info_size:		pointer to the size of image_info buffer
+ * @image_info:			pointer to buffer which contains info on the
+ *				image
+ * @desc_version:		pointer to version number of the
+ *				efi_firmware_image_descriptor
+ * @desc_count:			pointer to the number of descriptors of the
+ *				firmware images on the device
+ * @desc_size:			pointer to the size of an individual descriptor
+ * @package_version:		version for all firmware images on the device
+ * @package_version_name:	string representing the package version name
+ *
+ * Implement the get_image_info function of the
+ * efi_firmware_management_protocol for the platform.
+ *
+ * Return: status code
+ */
+static efi_status_t EFIAPI qemu_arm64_fmp_get_image_info(
+	struct efi_firmware_management_protocol *this,
+	efi_uintn_t *image_info_size,
+	struct efi_firmware_image_descriptor *image_info,
+	u32 *desc_version, u8 *desc_count,
+	efi_uintn_t *desc_size, u32 *package_version,
+	u16 **package_version_name)
+{
+	efi_status_t status = EFI_SUCCESS;
+	u16 *image_id_name;
+	const char *image_name = "Qemu Aarch64 U-Boot";
+	const efi_guid_t image_guid = EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID;
+
+	EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, image_info_size,
+		  image_info, desc_version, desc_count, desc_size,
+		  package_version, package_version_name);
+
+	/* Sanity checks */
+	if (*image_info_size && !image_info) {
+		status = EFI_INVALID_PARAMETER;
+		goto back;
+	}
+
+	if (*image_info_size &&
+	    (!desc_version || !desc_count || !desc_size)) {
+		status = EFI_INVALID_PARAMETER;
+		goto back;
+	}
+
+	if (*image_info_size < sizeof(*image_info)) {
+		*image_info_size = sizeof(*image_info);
+		status = EFI_BUFFER_TOO_SMALL;
+		goto back;
+	}
+
+	if (desc_version)
+		*desc_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
+
+	*desc_count = 0x1;
+	*desc_size = sizeof(*image_info);
+
+	if (package_version)
+		*package_version = 0xffffffff;
+
+	if (package_version_name)
+		*package_version_name = NULL;
+
+	image_info[0].image_type_id = image_guid;
+	image_info[0].image_id = QEMU_UBOOT_IMAGE;
+
+	image_id_name = malloc(40);
+	utf8_utf16_strcpy(&image_id_name, image_name);
+	image_info[0].image_id_name = image_id_name;
+
+	/* Todo: Get a mechanism to store version information */
+	image_info[0]. version = 0x1;
+	image_info[0].version_name = NULL;
+
+	/* Todo: Need to find a mechanism to get the image size */
+	image_info[0].size = 0;
+
+	image_info[0].attributes_supported =
+		EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
+	image_info[0].attributes_setting = EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
+
+	image_info[0].lowest_supported_image_version = 1;
+	image_info[0].last_attempt_version = 0;
+	image_info[0].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
+	image_info[0].hardware_instance = 0;
+
+back:
+	return EFI_EXIT(status);
+}
+
+/**
+ * qemu_arm64_fmp_set_image() - Implement set_image
+ *
+ * @this:			instance of the efi_firmware_management_protocol
+ * @image_index:		a unique number identifying the firmware image
+ * @image:			pointer to the buffer containing the firmware
+ *				image to be updated
+ * @image_size:			pointer to size of the firmware image
+ * @vendor_code:		pointer to the vendor specific update policy
+ * @completion:			function pointer that can be invoked to report
+ *				firmware update progress
+ * @abort_reason:		string providing details if update is aborted
+ *
+ * Implement the set_image function of the efi_firmware_management_protocol
+ * for the platform. This updates the firmware image and authenticates the
+ * capsule, if authentication is enabled
+ *
+ * Return: status code
+ */
+static efi_status_t EFIAPI qemu_arm64_fmp_set_image(
+	struct efi_firmware_management_protocol *this,
+	u8 image_index, const void *image,
+	efi_uintn_t image_size, const void *vendor_code,
+	efi_status_t (*progress)(efi_uintn_t completion),
+	u16 **abort_reason)
+{
+	long fd, ret;
+	efi_status_t status = EFI_SUCCESS;
+	char *mode = "w+b";
+
+	EFI_ENTRY("%p %d %p %ld %p %p %p\n", this, image_index, image,
+		  image_size, vendor_code, progress, abort_reason);
+
+	/*
+	 * Put a hack here to offset the size of
+	 * the FMP_PAYLOAD_HEADER that gets added
+	 * by the GenerateCapsule script in edk2.
+	 */
+	image += 0x10;
+	image_size -= 0x10;
+
+	/* Do all the sanity checks first */
+	if (!image) {
+		status = EFI_INVALID_PARAMETER;
+		goto back;
+	}
+
+	if (image_size == 0) {
+		status = EFI_INVALID_PARAMETER;
+		goto back;
+	}
+
+	if (image_index != QEMU_UBOOT_IMAGE_INDEX) {
+		status = EFI_INVALID_PARAMETER;
+		goto back;
+	}
+
+	/* Do the update */
+	fd = smh_open(UBOOT_FILE, mode);
+	if (fd == -1) {
+		printf("Unable to open the firmware image for writing\n");
+		status = EFI_DEVICE_ERROR;
+		goto back;
+	}
+
+	ret = smh_write(fd, (void *)image, image_size);
+	if (ret == -1) {
+		printf("Error writing to the firmware image!");
+		smh_close(fd);
+		status = EFI_DEVICE_ERROR;
+		goto back;
+	}
+
+	printf("Capsule Update Complete\n");
+	smh_close(fd);
+back:
+	return EFI_EXIT(status);
+}
+
+const struct efi_firmware_management_protocol efi_qemu_arm64_fmp = {
+	.get_image_info = qemu_arm64_fmp_get_image_info,
+	.set_image = qemu_arm64_fmp_set_image,
+};
+
+efi_status_t arch_efi_load_capsule_drivers(void)
+{
+	efi_status_t ret;
+
+	ret = EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root,
+					&efi_guid_firmware_management_protocol,
+					&efi_qemu_arm64_fmp,
+					NULL));
+
+	return ret;
+}
-- 
2.17.1

[PATCH 4/8] efi_loader: Allow parsing of miscellaneous signature database variables

[Sughosh Ganu]

The current function to parse the signature database(sigdb) only
allows parsing of secure boot related sigdb variables, namely PK, KEK,
db and dbx. Allow the function to parse any other sigdb
variables. This would be useful for the capsule authenticate feature,
which would be storing it's root certificate in a non secure-boot
sigdb. This is done by passing the vendor guid as a function argument.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_loader.h              |  3 ++-
 lib/efi_loader/efi_image_loader.c | 11 +++++++----
 lib/efi_loader/efi_signature.c    | 14 +++-----------
 lib/efi_loader/efi_variable.c     |  9 ++++++---
 4 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index ad99ab660f..b7638d5825 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -774,7 +774,8 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
 				  int nocheck);
 
 void efi_sigstore_free(struct efi_signature_store *sigstore);
-struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name);
+struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name,
+						     const efi_guid_t *vendor);
 
 bool efi_secure_boot_enabled(void);
 
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 6c270ce94f..e07dc290a3 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -418,13 +418,15 @@ static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
 	struct efi_signature_store *db = NULL, *dbx = NULL;
 	bool ret = false;
 
-	dbx = efi_sigstore_parse_sigdb(L"dbx");
+	dbx = efi_sigstore_parse_sigdb(L"dbx",
+				       &efi_guid_image_security_database);
 	if (!dbx) {
 		debug("Getting signature database(dbx) failed\n");
 		goto out;
 	}
 
-	db = efi_sigstore_parse_sigdb(L"db");
+	db = efi_sigstore_parse_sigdb(L"db",
+				      &efi_guid_image_security_database);
 	if (!db) {
 		debug("Getting signature database(db) failed\n");
 		goto out;
@@ -515,13 +517,14 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	/*
 	 * verify signature using db and dbx
 	 */
-	db = efi_sigstore_parse_sigdb(L"db");
+	db = efi_sigstore_parse_sigdb(L"db", &efi_guid_image_security_database);
 	if (!db) {
 		debug("Getting signature database(db) failed\n");
 		goto err;
 	}
 
-	dbx = efi_sigstore_parse_sigdb(L"dbx");
+	dbx = efi_sigstore_parse_sigdb(L"dbx",
+				       &efi_guid_image_security_database);
 	if (!dbx) {
 		debug("Getting signature database(dbx) failed\n");
 		goto err;
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 658e3547da..bf6f39aab2 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -714,30 +714,22 @@ err:
 /**
  * efi_sigstore_parse_sigdb - parse a signature database variable
  * @name:	Variable's name
+ * @vendor:	Vendor guid
  *
  * Read in a value of signature database variable pointed to by
  * @name, parse it and instantiate a signature store structure.
  *
  * Return:	Pointer to signature store on success, NULL on error
  */
-struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
+struct efi_signature_store *efi_sigstore_parse_sigdb(
+	u16 *name, const efi_guid_t *vendor)
 {
 	struct efi_signature_store *sigstore = NULL, *siglist;
 	struct efi_signature_list *esl;
-	const efi_guid_t *vendor;
 	void *db;
 	efi_uintn_t db_size;
 	efi_status_t ret;
 
-	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
-		vendor = &efi_global_variable_guid;
-	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
-		vendor = &efi_guid_image_security_database;
-	} else {
-		debug("unknown signature database, %ls\n", name);
-		return NULL;
-	}
-
 	/* retrieve variable data */
 	db_size = 0;
 	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 7df881a74b..6c2dd82306 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -604,14 +604,17 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
 	if (u16_strcmp(variable, L"PK") == 0 ||
 	    u16_strcmp(variable, L"KEK") == 0) {
 		/* with PK */
-		truststore = efi_sigstore_parse_sigdb(L"PK");
+		truststore = efi_sigstore_parse_sigdb(L"PK",
+			&efi_global_variable_guid);
 		if (!truststore)
 			goto err;
 	} else if (u16_strcmp(variable, L"db") == 0 ||
 		   u16_strcmp(variable, L"dbx") == 0) {
 		/* with PK and KEK */
-		truststore = efi_sigstore_parse_sigdb(L"KEK");
-		truststore2 = efi_sigstore_parse_sigdb(L"PK");
+		truststore = efi_sigstore_parse_sigdb(L"KEK",
+			&efi_global_variable_guid);
+		truststore2 = efi_sigstore_parse_sigdb(L"PK",
+			&efi_global_variable_guid);
 
 		if (!truststore) {
 			if (!truststore2)
-- 
2.17.1

[PATCH 5/8] efi_loader: Make the pkcs7 header parsing function an extern

[Sughosh Ganu]

The pkcs7 header parsing functionality is pretty generic, and can be
used by other features like capsule authentication. Make the function
as an extern, also changing it's name to efi_parse_pkcs7_header.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_loader.h           |  2 +
 lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_variable.c  | 82 +---------------------------------
 3 files changed, 82 insertions(+), 80 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index b7638d5825..8d923451ce 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void);
 
 bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		     WIN_CERTIFICATE **auth, size_t *auth_len);
+struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen);
+
 #endif /* CONFIG_EFI_SECURE_BOOT */
 
 #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index bf6f39aab2..9897f5418e 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
 
 #ifdef CONFIG_EFI_SECURE_BOOT
 
+static u8 pkcs7_hdr[] = {
+	/* SEQUENCE */
+	0x30, 0x82, 0x05, 0xc7,
+	/* OID: pkcs7-signedData */
+	0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
+	/* Context Structured? */
+	0xa0, 0x82, 0x05, 0xb8,
+};
+
+/**
+ * efi_parse_pkcs7_header - parse a signature in variable
+ * @buf:	Pointer to the payload's value
+ * @buflen:	Length of @buf
+ *
+ * Parse a signature embedded in variable's value and instantiate
+ * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
+ * pkcs7's signedData, some header needed be prepended for correctly
+ * parsing authentication data, particularly for variable's.
+ *
+ * Return:	Pointer to pkcs7_message structure on success, NULL on error
+ */
+struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen)
+{
+	u8 *ebuf;
+	size_t ebuflen, len;
+	struct pkcs7_message *msg;
+
+	/*
+	 * This is the best assumption to check if the binary is
+	 * already in a form of pkcs7's signedData.
+	 */
+	if (buflen > sizeof(pkcs7_hdr) &&
+	    !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
+		msg = pkcs7_parse_message(buf, buflen);
+		goto out;
+	}
+
+	/*
+	 * Otherwise, we should add a dummy prefix sequence for pkcs7
+	 * message parser to be able to process.
+	 * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
+	 * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
+	 * TODO:
+	 * The header should be composed in a more refined manner.
+	 */
+	debug("Makeshift prefix added to authentication data\n");
+	ebuflen = sizeof(pkcs7_hdr) + buflen;
+	if (ebuflen <= 0x7f) {
+		debug("Data is too short\n");
+		return NULL;
+	}
+
+	ebuf = malloc(ebuflen);
+	if (!ebuf) {
+		debug("Out of memory\n");
+		return NULL;
+	}
+
+	memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
+	memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
+	len = ebuflen - 4;
+	ebuf[2] = (len >> 8) & 0xff;
+	ebuf[3] = len & 0xff;
+	len = ebuflen - 0x13;
+	ebuf[0x11] = (len >> 8) & 0xff;
+	ebuf[0x12] = len & 0xff;
+
+	msg = pkcs7_parse_message(ebuf, ebuflen);
+
+	free(ebuf);
+
+out:
+	if (IS_ERR(msg))
+		return NULL;
+
+	return msg;
+}
+
 /**
  * efi_hash_regions - calculate a hash value
  * @regs:	List of regions
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 6c2dd82306..be34a2cadd 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void)
 	return efi_secure_boot;
 }
 
-#ifdef CONFIG_EFI_SECURE_BOOT
-static u8 pkcs7_hdr[] = {
-	/* SEQUENCE */
-	0x30, 0x82, 0x05, 0xc7,
-	/* OID: pkcs7-signedData */
-	0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
-	/* Context Structured? */
-	0xa0, 0x82, 0x05, 0xb8,
-};
-
-/**
- * efi_variable_parse_signature - parse a signature in variable
- * @buf:	Pointer to variable's value
- * @buflen:	Length of @buf
- *
- * Parse a signature embedded in variable's value and instantiate
- * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
- * pkcs7's signedData, some header needed be prepended for correctly
- * parsing authentication data, particularly for variable's.
- *
- * Return:	Pointer to pkcs7_message structure on success, NULL on error
- */
-static struct pkcs7_message *efi_variable_parse_signature(const void *buf,
-							  size_t buflen)
-{
-	u8 *ebuf;
-	size_t ebuflen, len;
-	struct pkcs7_message *msg;
-
-	/*
-	 * This is the best assumption to check if the binary is
-	 * already in a form of pkcs7's signedData.
-	 */
-	if (buflen > sizeof(pkcs7_hdr) &&
-	    !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
-		msg = pkcs7_parse_message(buf, buflen);
-		goto out;
-	}
-
-	/*
-	 * Otherwise, we should add a dummy prefix sequence for pkcs7
-	 * message parser to be able to process.
-	 * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
-	 * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
-	 * TODO:
-	 * The header should be composed in a more refined manner.
-	 */
-	debug("Makeshift prefix added to authentication data\n");
-	ebuflen = sizeof(pkcs7_hdr) + buflen;
-	if (ebuflen <= 0x7f) {
-		debug("Data is too short\n");
-		return NULL;
-	}
-
-	ebuf = malloc(ebuflen);
-	if (!ebuf) {
-		debug("Out of memory\n");
-		return NULL;
-	}
-
-	memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
-	memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
-	len = ebuflen - 4;
-	ebuf[2] = (len >> 8) & 0xff;
-	ebuf[3] = len & 0xff;
-	len = ebuflen - 0x13;
-	ebuf[0x11] = (len >> 8) & 0xff;
-	ebuf[0x12] = len & 0xff;
-
-	msg = pkcs7_parse_message(ebuf, ebuflen);
-
-	free(ebuf);
-
-out:
-	if (IS_ERR(msg))
-		return NULL;
-
-	return msg;
-}
+#ifdef CONFIG_SECURE_BOOT
 
 /**
  * efi_variable_authenticate - authenticate a variable
@@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
 	/* variable's signature list */
 	if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info))
 		goto err;
-	var_sig = efi_variable_parse_signature(auth->auth_info.cert_data,
+	var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data,
 					       auth->auth_info.hdr.dwLength
 						   - sizeof(auth->auth_info));
 	if (IS_ERR(var_sig)) {
-- 
2.17.1

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Sughosh Ganu]

Add support for authenticating uefi capsules. Most of the signature
verification functionality is shared with the uefi secure boot
feature.

The root certificate containing the public key used for the signature
verification is stored as an efi variable, similar to the variables
used for uefi secure boot. The root certificate is stored as an efi
signature list(esl) file -- this file contains the x509 certificate
which is the root certificate.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_api.h              |  17 +++++
 include/efi_loader.h           |   8 ++-
 lib/efi_loader/Kconfig         |  16 +++++
 lib/efi_loader/efi_capsule.c   | 126 +++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_signature.c |   4 +-
 5 files changed, 167 insertions(+), 4 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index e518ffa838..8dfa479db4 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -1794,6 +1794,23 @@ struct efi_variable_authentication_2 {
 	struct win_certificate_uefi_guid auth_info;
 } __attribute__((__packed__));
 
+/**
+ * efi_firmware_image_authentication - Capsule authentication method
+ * descriptor
+ *
+ * This structure describes an authentication information for
+ * a capsule with IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED set
+ * and should be included as part of the capsule.
+ * Only EFI_CERT_TYPE_PKCS7_GUID is accepted.
+ *
+ * @monotonic_count: Count to prevent replay
+ * @auth_info:	Authentication info
+ */
+struct efi_firmware_image_authentication {
+	uint64_t monotonic_count;
+	struct win_certificate_uefi_guid auth_info;
+} __attribute__((__packed__));
+
 /**
  * efi_signature_data - A format of signature
  *
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 8d923451ce..897710ae3f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -708,7 +708,7 @@ void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
 unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
 efi_status_t efi_bootmgr_load(efi_handle_t *handle);
 
-#ifdef CONFIG_EFI_SECURE_BOOT
+#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 #include <image.h>
 
 /**
@@ -783,7 +783,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 		     WIN_CERTIFICATE **auth, size_t *auth_len);
 struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen);
 
-#endif /* CONFIG_EFI_SECURE_BOOT */
+#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */
 
 #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
 /* Capsule update */
@@ -798,6 +798,10 @@ efi_status_t EFIAPI efi_query_capsule_caps(
 		u32 *reset_type);
 #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
 
+efi_status_t efi_capsule_authenticate(const void *capsule,
+				      efi_uintn_t capsule_size,
+				      void **image, efi_uintn_t *image_size);
+
 #ifdef CONFIG_EFI_CAPSULE_ON_DISK
 #define EFI_CAPSULE_DIR L"\\EFI\\UpdateCapsule\\"
 
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index ec2976ceba..245deabbed 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
 	help
 	  Define storage device, say 1:1, for storing FIT image
 
+config EFI_CAPSULE_AUTHENTICATE
+	bool "Update Capsule authentication"
+	depends on EFI_HAVE_CAPSULE_SUPPORT
+	depends on EFI_CAPSULE_ON_DISK
+	depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
+	select SHA256
+	select RSA
+	select RSA_VERIFY
+	select RSA_VERIFY_WITH_PKEY
+	select X509_CERTIFICATE_PARSER
+	select PKCS7_MESSAGE_PARSER
+	default n
+	help
+	  Select this option if you want to enable capsule
+	  authentication
+
 config EFI_DEVICE_PATH_TO_TEXT
 	bool "Device path to text protocol"
 	default y
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 931d363edc..a265d36ff0 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -12,6 +12,10 @@
 #include <malloc.h>
 #include <mapmem.h>
 #include <sort.h>
+#include "../lib/crypto/pkcs7_parser.h"
+
+#include <crypto/pkcs7.h>
+#include <linux/err.h>
 
 const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
 static const efi_guid_t efi_guid_firmware_management_capsule_id =
@@ -245,6 +249,128 @@ out:
 
 	return ret;
 }
+
+#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
+
+const efi_guid_t efi_guid_capsule_root_cert_guid =
+	EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
+
+__weak u16 *efi_get_truststore_name(void)
+{
+	return L"CRT";
+}
+
+__weak const efi_guid_t *efi_get_truststore_vendor(void)
+{
+
+	return &efi_guid_capsule_root_cert_guid;
+}
+
+/**
+ * efi_capsule_authenticate() - Authenticate a uefi capsule
+ *
+ * @capsule:		Capsule file with the authentication
+ *			header
+ * @capsule_size:	Size of the entire capsule
+ * @image:		pointer to the image payload minus the
+ *			authentication header
+ * @image_size:		size of the image payload
+ *
+ * Authenticate the capsule signature with the public key contained
+ * in the root certificate stored as an efi environment variable
+ *
+ * Return: EFI_SUCCESS on successfull authentication or
+ *	   EFI_SECURITY_VIOLATION on authentication failure
+ */
+efi_status_t efi_capsule_authenticate(const void *capsule,
+				      efi_uintn_t capsule_size,
+				      void **image, efi_uintn_t *image_size)
+{
+	uint64_t monotonic_count;
+	struct efi_signature_store *truststore;
+	struct pkcs7_message *capsule_sig;
+	struct efi_image_regions *regs;
+	struct efi_firmware_image_authentication *auth_hdr;
+	efi_status_t status;
+
+	status = EFI_SECURITY_VIOLATION;
+	capsule_sig = NULL;
+	truststore = NULL;
+	regs = NULL;
+
+	/* Sanity checks */
+	if (capsule == NULL || capsule_size == 0)
+		goto out;
+
+	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
+	if (capsule_size < sizeof(*auth_hdr))
+		goto out;
+
+	if (auth_hdr->auth_info.hdr.dwLength <=
+	    offsetof(struct win_certificate_uefi_guid, cert_data))
+		goto out;
+
+	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
+		goto out;
+
+	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
+		auth_hdr->auth_info.hdr.dwLength;
+	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
+				      sizeof(auth_hdr->monotonic_count);
+	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
+	       sizeof(monotonic_count));
+
+	/* data to be digested */
+	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2, 1);
+	if (!regs)
+		goto out;
+
+	regs->max = 2;
+	efi_image_region_add(regs, (uint8_t *)*image,
+			     (uint8_t *)*image + *image_size, 1);
+
+	efi_image_region_add(regs, (uint8_t *)&monotonic_count,
+			     (uint8_t *)&monotonic_count + sizeof(monotonic_count),
+			     1);
+
+	capsule_sig = efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
+					     auth_hdr->auth_info.hdr.dwLength
+					     - sizeof(auth_hdr->auth_info));
+	if (IS_ERR(capsule_sig)) {
+		debug("Parsing variable's pkcs7 header failed\n");
+		capsule_sig = NULL;
+		goto out;
+	}
+
+	truststore = efi_sigstore_parse_sigdb(efi_get_truststore_name(),
+					      efi_get_truststore_vendor());
+	if (!truststore)
+		goto out;
+
+	/* verify signature */
+	if (efi_signature_verify_with_sigdb(regs, capsule_sig, truststore, NULL)) {
+		debug("Verified\n");
+	} else {
+		debug("Verifying variable's signature failed\n");
+		goto out;
+	}
+
+	status = EFI_SUCCESS;
+
+out:
+	efi_sigstore_free(truststore);
+	pkcs7_free_message(capsule_sig);
+	free(regs);
+
+	return status;
+}
+#else
+efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
+				      void **image, efi_uintn_t *image_size)
+{
+	return EFI_UNSUPPORTED;
+}
+#endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
 #else
 static efi_status_t efi_capsule_update_firmware(
 		struct efi_capsule_header *capsule_data)
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 9897f5418e..4c722e0da9 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -23,7 +23,7 @@ const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
 const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
 const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
 
-#ifdef CONFIG_EFI_SECURE_BOOT
+#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
 
 static u8 pkcs7_hdr[] = {
 	/* SEQUENCE */
@@ -871,4 +871,4 @@ err:
 
 	return NULL;
 }
-#endif /* CONFIG_EFI_SECURE_BOOT */
+#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */
-- 
2.17.1

[PATCH 7/8] qemu: arm64: Add support for uefi capsule authentication

[Sughosh Ganu]

Add support for uefi capsule authentication feature for the qemu arm64
platform. This feature is enabled by setting the environment variable
"capsule_authentication_enabled".

The following configs are needed for enabling uefi capsule update and
capsule authentication features on the platform.

CONFIG_EFI_CAPSULE_ON_DISK=y
CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
CONFIG_EFI_CAPSULE_AUTHENTICATE=y

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 board/emulation/qemu-arm/qemu_efi_fmp.c | 49 +++++++++++++++++++++----
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/board/emulation/qemu-arm/qemu_efi_fmp.c b/board/emulation/qemu-arm/qemu_efi_fmp.c
index 9baea94e6c..b58843f8fb 100644
--- a/board/emulation/qemu-arm/qemu_efi_fmp.c
+++ b/board/emulation/qemu-arm/qemu_efi_fmp.c
@@ -101,9 +101,15 @@ static efi_status_t EFIAPI qemu_arm64_fmp_get_image_info(
 	image_info[0].size = 0;
 
 	image_info[0].attributes_supported =
-		EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
+		EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE |
+		EFI_IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
 	image_info[0].attributes_setting = EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
 
+	/* Check if the capsule authentication is enabled */
+	if (env_get("capsule_authentication_enabled"))
+		image_info[0].attributes_setting |=
+			EFI_IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
+
 	image_info[0].lowest_supported_image_version = 1;
 	image_info[0].last_attempt_version = 0;
 	image_info[0].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
@@ -142,17 +148,12 @@ static efi_status_t EFIAPI qemu_arm64_fmp_set_image(
 	long fd, ret;
 	efi_status_t status = EFI_SUCCESS;
 	char *mode = "w+b";
+	void *capsule_payload;
+	efi_uintn_t capsule_payload_size;
 
 	EFI_ENTRY("%p %d %p %ld %p %p %p\n", this, image_index, image,
 		  image_size, vendor_code, progress, abort_reason);
 
-	/*
-	 * Put a hack here to offset the size of
-	 * the FMP_PAYLOAD_HEADER that gets added
-	 * by the GenerateCapsule script in edk2.
-	 */
-	image += 0x10;
-	image_size -= 0x10;
 
 	/* Do all the sanity checks first */
 	if (!image) {
@@ -170,6 +171,38 @@ static efi_status_t EFIAPI qemu_arm64_fmp_set_image(
 		goto back;
 	}
 
+	/* Authenticate the capsule if authentication enabled */
+	if (IS_ENABLED(CONFIG_EFI_CAPSULE_AUTHENTICATE) &&
+	    env_get("capsule_authentication_enabled")) {
+		capsule_payload = NULL;
+		capsule_payload_size = 0;
+		status = efi_capsule_authenticate(image, image_size,
+						  &capsule_payload,
+						  &capsule_payload_size);
+
+		if (status == EFI_SECURITY_VIOLATION) {
+			printf("Capsule authentication check failed. Aborting update\n");
+			goto back;
+		} else if (status != EFI_SUCCESS) {
+			goto back;
+		}
+
+		debug("Capsule authentication successfull\n");
+		image = capsule_payload;
+		image_size = capsule_payload_size;
+	} else {
+		debug("Capsule authentication disabled. ");
+		debug("Updating capsule without authenticating.\n");
+	}
+
+	/*
+	 * Put a hack here to offset the size of
+	 * the FMP_PAYLOAD_HEADER that gets added
+	 * by the GenerateCapsule script in edk2.
+	 */
+	image += 0x10;
+	image_size -= 0x10;
+
 	/* Do the update */
 	fd = smh_open(UBOOT_FILE, mode);
 	if (fd == -1) {
-- 
2.17.1

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Sughosh Ganu]

Add documentation highlighting the steps for using the uefi capsule
update feature for updating the u-boot firmware image.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 doc/board/emulation/qemu-arm.rst | 112 +++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/doc/board/emulation/qemu-arm.rst b/doc/board/emulation/qemu-arm.rst
index ca751d4af4..8649d593ed 100644
--- a/doc/board/emulation/qemu-arm.rst
+++ b/doc/board/emulation/qemu-arm.rst
@@ -80,3 +80,115 @@ can be enabled with the following command line parameters:
     -drive if=none,file=disk.img,id=mydisk -device nvme,drive=mydisk,serial=foo
 
 These have been tested in QEMU 2.9.0 but should work in at least 2.5.0 as well.
+
+Enabling Uefi Capsule Update feature
+------------------------------------
+
+Support has been added for the uefi capsule update feature which
+enables updating the u-boot image using the uefi firmware management
+protocol (fmp). The capsules are not passed to the firmware through
+the UpdateCapsule runtime service. Instead, capsule-on-disk
+functionality is used for fetching the capsule from the EFI System
+Partition (ESP). Currently, support has been added for the arm64
+target booting with arm trusted firmware. The This feature is enabled
+with the following configs::
+
+    CONFIG_EFI_CAPSULE_ON_DISK=y
+    CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
+    CONFIG_CMD_EFIDEBUG=y
+
+The capsule file can be generated by using the GenerateCapsule.py
+script in edk2::
+
+    $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
+    <capsule_file_name> --fw-version <val> --lsv <val> --guid \
+    fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose --update-image-index \
+    <val> --verbose <u-boot.bin>
+
+
+As per the uefi specification, the capsule file needs to be placed on
+the EFI System Partition, under the EFI/UpdateCapsule/ directory. The
+EFI System Partition can be a virtio-blk-device.
+
+Before initiating the firmware update, the efi variables BootNext and
+BootXXXX need to be set. The BootXXXX variable needs to be pointing to
+the EFI System Partition which contains the capsule file. The
+BootNext and BootXXXX variables can be set using the efidebug
+command::
+
+    => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name>
+    => efidebug boot next 0
+
+The OsIndications efi variable needs to be set with the
+EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED flag set::
+
+    => setenv -e -nv -bs -rt OsIndications =0x04
+    => saveenv
+
+The capsule update function will be invoked on subsequent boot as part
+of the main_loop function. The updated u-boot image will be booted on
+subsequent boot.
+
+
+Enabling Capsule Authentication
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The uefi specification defines a way of authenticating the capsule to
+be updated by verifying the capsule signature. The capsule signature
+is computed and prepended to the capsule payload at the time of
+capsule generation. This signature is then verified by using the
+public key stored as part of the X509 certificate. This certificate is
+in the form of an efi signature list (esl) file, which is stored as an
+efi variable.
+
+The capsule authentication feature can be enabled through the
+following config, in addition to the configs listed above for capsule
+update::
+
+    CONFIG_EFI_CAPSULE_AUTHENTICATE=y
+
+The esl file can be generated as follows:
+
+1. Install utility commands on your host
+    * openssl
+    * efitools
+
+2. Create signing keys and certificate files on your host::
+
+        $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \
+                -keyout CRT.key -out CRT.crt -nodes -days 365
+        $ cert-to-efi-sig-list CRT.crt CRT.esl
+
+        $ openssl x509 -in CRT.crt -out CRT.cer -outform DER
+        $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem
+
+        $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt
+        $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem
+
+3. Store the esl file generated above as an efi variable::
+
+        => fatload virtio 0:1 <load_addr> EFI/CRT.esl
+        => setenv -e -nv -bs -rt -i <load_addr>,$filesize CRT
+
+        => setenv capsule_authentication_enabled 1
+        => setenv -e -nv -bs -rt OsIndication =0x04
+        => saveenv
+
+Setting the environment variable capsule_authentication_enabled
+enables the capsule authentication.
+
+4. The capsule file can be generated by using the GenerateCapsule.py
+   script in edk2::
+
+        $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
+	<capsule_file_name> --monotonic-count <val> --fw-version \
+	<val> --lsv <val> --guid \
+	fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose \
+	--update-image-index <val> --signer-private-cert \
+	/path/to/CRT.pem --trusted-public-cert \
+	/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \
+	<u-boot.bin>
+
+Once the capsule has been generated, use the same instructions as
+mentioned above for placing the capsule on the EFI System Partition
+and subsequently to initiate the update.
-- 
2.17.1

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Heinrich Schuchardt]

On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> Add documentation highlighting the steps for using the uefi capsule
> update feature for updating the u-boot firmware image.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

UEFI capsule updates should be architecture independent. I would expect
that the submitted code should work for x86, ARM, and RISC-V. Why does
this documentation live under the ARM emulation tree?

Best regards

Heinrich

> ---
>  doc/board/emulation/qemu-arm.rst | 112 +++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>
> diff --git a/doc/board/emulation/qemu-arm.rst b/doc/board/emulation/qemu-arm.rst
> index ca751d4af4..8649d593ed 100644
> --- a/doc/board/emulation/qemu-arm.rst
> +++ b/doc/board/emulation/qemu-arm.rst
> @@ -80,3 +80,115 @@ can be enabled with the following command line parameters:
>      -drive if=none,file=disk.img,id=mydisk -device nvme,drive=mydisk,serial=foo
>
>  These have been tested in QEMU 2.9.0 but should work in at least 2.5.0 as well.
> +
> +Enabling Uefi Capsule Update feature
> +------------------------------------
> +
> +Support has been added for the uefi capsule update feature which
> +enables updating the u-boot image using the uefi firmware management
> +protocol (fmp). The capsules are not passed to the firmware through
> +the UpdateCapsule runtime service. Instead, capsule-on-disk
> +functionality is used for fetching the capsule from the EFI System
> +Partition (ESP). Currently, support has been added for the arm64
> +target booting with arm trusted firmware. The This feature is enabled
> +with the following configs::
> +
> +    CONFIG_EFI_CAPSULE_ON_DISK=y
> +    CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> +    CONFIG_CMD_EFIDEBUG=y
> +
> +The capsule file can be generated by using the GenerateCapsule.py
> +script in edk2::
> +
> +    $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
> +    <capsule_file_name> --fw-version <val> --lsv <val> --guid \
> +    fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose --update-image-index \
> +    <val> --verbose <u-boot.bin>
> +
> +
> +As per the uefi specification, the capsule file needs to be placed on
> +the EFI System Partition, under the EFI/UpdateCapsule/ directory. The
> +EFI System Partition can be a virtio-blk-device.
> +
> +Before initiating the firmware update, the efi variables BootNext and
> +BootXXXX need to be set. The BootXXXX variable needs to be pointing to
> +the EFI System Partition which contains the capsule file. The
> +BootNext and BootXXXX variables can be set using the efidebug
> +command::
> +
> +    => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name>
> +    => efidebug boot next 0
> +
> +The OsIndications efi variable needs to be set with the
> +EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED flag set::
> +
> +    => setenv -e -nv -bs -rt OsIndications =0x04
> +    => saveenv
> +
> +The capsule update function will be invoked on subsequent boot as part
> +of the main_loop function. The updated u-boot image will be booted on
> +subsequent boot.
> +
> +
> +Enabling Capsule Authentication
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The uefi specification defines a way of authenticating the capsule to
> +be updated by verifying the capsule signature. The capsule signature
> +is computed and prepended to the capsule payload at the time of
> +capsule generation. This signature is then verified by using the
> +public key stored as part of the X509 certificate. This certificate is
> +in the form of an efi signature list (esl) file, which is stored as an
> +efi variable.
> +
> +The capsule authentication feature can be enabled through the
> +following config, in addition to the configs listed above for capsule
> +update::
> +
> +    CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> +
> +The esl file can be generated as follows:
> +
> +1. Install utility commands on your host
> +    * openssl
> +    * efitools
> +
> +2. Create signing keys and certificate files on your host::
> +
> +        $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \
> +                -keyout CRT.key -out CRT.crt -nodes -days 365
> +        $ cert-to-efi-sig-list CRT.crt CRT.esl
> +
> +        $ openssl x509 -in CRT.crt -out CRT.cer -outform DER
> +        $ openssl x509 -inform DER -in CRT.cer -outform PEM -out CRT.pub.pem
> +
> +        $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt
> +        $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem
> +
> +3. Store the esl file generated above as an efi variable::
> +
> +        => fatload virtio 0:1 <load_addr> EFI/CRT.esl
> +        => setenv -e -nv -bs -rt -i <load_addr>,$filesize CRT
> +
> +        => setenv capsule_authentication_enabled 1
> +        => setenv -e -nv -bs -rt OsIndication =0x04
> +        => saveenv
> +
> +Setting the environment variable capsule_authentication_enabled
> +enables the capsule authentication.
> +
> +4. The capsule file can be generated by using the GenerateCapsule.py
> +   script in edk2::
> +
> +        $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
> +	<capsule_file_name> --monotonic-count <val> --fw-version \
> +	<val> --lsv <val> --guid \
> +	fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose \
> +	--update-image-index <val> --signer-private-cert \
> +	/path/to/CRT.pem --trusted-public-cert \
> +	/path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \
> +	<u-boot.bin>
> +
> +Once the capsule has been generated, use the same instructions as
> +mentioned above for placing the capsule on the EFI System Partition
> +and subsequently to initiate the update.
>

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> Add support for the get_image_info and set_image routines, which are
> part of the efi firmware management protocol.
>
> The current implementation uses the set_image routine for updating the
> u-boot binary image for the qemu arm64 platform. This is supported
> using the capsule-on-disk feature of the uefi specification, wherein
> the firmware image to be updated is placed on the efi system partition
> as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
> added for updating the u-boot image on platforms booting with arm
> trusted firmware(tf-a), where the u-boot image gets booted as the BL33
> payload(bl33.bin).
>
> The feature can be enabled by the following config options
>
> CONFIG_EFI_CAPSULE_ON_DISK=y
> CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>

U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
RISC-V. Please, come up with an architecture independent solution.

Best regards

Heinrich

> ---
>  board/emulation/qemu-arm/Kconfig        |  12 ++
>  board/emulation/qemu-arm/Makefile       |   1 +
>  board/emulation/qemu-arm/qemu_efi_fmp.c | 210 ++++++++++++++++++++++++
>  3 files changed, 223 insertions(+)
>  create mode 100644 board/emulation/qemu-arm/qemu_efi_fmp.c
>
> diff --git a/board/emulation/qemu-arm/Kconfig b/board/emulation/qemu-arm/Kconfig
> index 02ae4d9884..f1b2de8e41 100644
> --- a/board/emulation/qemu-arm/Kconfig
> +++ b/board/emulation/qemu-arm/Kconfig
> @@ -11,3 +11,15 @@ config BOARD_SPECIFIC_OPTIONS # dummy
>  	imply VIRTIO_BLK
>
>  endif
> +
> +if TARGET_QEMU_ARM_64BIT && TFABOOT
> +
> +config EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> +	bool "EFI Firmware Management protocol for Qemu arm64 platform"
> +	depends on EFI_CAPSULE_ON_DISK
> +	default n
> +	help
> +	  Select this option for enabling firmware management protocol
> +	  for qemu arm64 platform
> +
> +endif
> diff --git a/board/emulation/qemu-arm/Makefile b/board/emulation/qemu-arm/Makefile
> index a22d1237ff..c95ac6d233 100644
> --- a/board/emulation/qemu-arm/Makefile
> +++ b/board/emulation/qemu-arm/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0+
>
>  obj-y	+= qemu-arm.o
> +obj-$(CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL) += qemu_efi_fmp.o
> diff --git a/board/emulation/qemu-arm/qemu_efi_fmp.c b/board/emulation/qemu-arm/qemu_efi_fmp.c
> new file mode 100644
> index 0000000000..9baea94e6c
> --- /dev/null
> +++ b/board/emulation/qemu-arm/qemu_efi_fmp.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <charset.h>
> +#include <efi_api.h>
> +#include <efi_loader.h>
> +#include <malloc.h>
> +#include <semihosting.h>
> +
> +#define QEMU_UBOOT_IMAGE_INDEX	0x1
> +#define QEMU_UBOOT_IMAGE	0x1
> +
> +#define UBOOT_FILE		"bl33.bin"
> +
> +#define EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID \
> +	EFI_GUID(0xfb90808a, 0xba9a, 0x4d42, 0xb9, 0xa2, \
> +		 0xa7, 0xa9, 0x37, 0x14, 0x4a, 0xee)
> +
> +/**
> + * qemu_arm64_fmp_get_image_info() - Implement get_image_info
> + *
> + * @this:			instance of the efi_firmware_management_protocol
> + * @image_info_size:		pointer to the size of image_info buffer
> + * @image_info:			pointer to buffer which contains info on the
> + *				image
> + * @desc_version:		pointer to version number of the
> + *				efi_firmware_image_descriptor
> + * @desc_count:			pointer to the number of descriptors of the
> + *				firmware images on the device
> + * @desc_size:			pointer to the size of an individual descriptor
> + * @package_version:		version for all firmware images on the device
> + * @package_version_name:	string representing the package version name
> + *
> + * Implement the get_image_info function of the
> + * efi_firmware_management_protocol for the platform.
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI qemu_arm64_fmp_get_image_info(
> +	struct efi_firmware_management_protocol *this,
> +	efi_uintn_t *image_info_size,
> +	struct efi_firmware_image_descriptor *image_info,
> +	u32 *desc_version, u8 *desc_count,
> +	efi_uintn_t *desc_size, u32 *package_version,
> +	u16 **package_version_name)
> +{
> +	efi_status_t status = EFI_SUCCESS;
> +	u16 *image_id_name;
> +	const char *image_name = "Qemu Aarch64 U-Boot";
> +	const efi_guid_t image_guid = EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID;
> +
> +	EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, image_info_size,
> +		  image_info, desc_version, desc_count, desc_size,
> +		  package_version, package_version_name);
> +
> +	/* Sanity checks */
> +	if (*image_info_size && !image_info) {
> +		status = EFI_INVALID_PARAMETER;
> +		goto back;
> +	}
> +
> +	if (*image_info_size &&
> +	    (!desc_version || !desc_count || !desc_size)) {
> +		status = EFI_INVALID_PARAMETER;
> +		goto back;
> +	}
> +
> +	if (*image_info_size < sizeof(*image_info)) {
> +		*image_info_size = sizeof(*image_info);
> +		status = EFI_BUFFER_TOO_SMALL;
> +		goto back;
> +	}
> +
> +	if (desc_version)
> +		*desc_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
> +
> +	*desc_count = 0x1;
> +	*desc_size = sizeof(*image_info);
> +
> +	if (package_version)
> +		*package_version = 0xffffffff;
> +
> +	if (package_version_name)
> +		*package_version_name = NULL;
> +
> +	image_info[0].image_type_id = image_guid;
> +	image_info[0].image_id = QEMU_UBOOT_IMAGE;
> +
> +	image_id_name = malloc(40);
> +	utf8_utf16_strcpy(&image_id_name, image_name);
> +	image_info[0].image_id_name = image_id_name;
> +
> +	/* Todo: Get a mechanism to store version information */
> +	image_info[0]. version = 0x1;
> +	image_info[0].version_name = NULL;
> +
> +	/* Todo: Need to find a mechanism to get the image size */
> +	image_info[0].size = 0;
> +
> +	image_info[0].attributes_supported =
> +		EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> +	image_info[0].attributes_setting = EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> +
> +	image_info[0].lowest_supported_image_version = 1;
> +	image_info[0].last_attempt_version = 0;
> +	image_info[0].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +	image_info[0].hardware_instance = 0;
> +
> +back:
> +	return EFI_EXIT(status);
> +}
> +
> +/**
> + * qemu_arm64_fmp_set_image() - Implement set_image
> + *
> + * @this:			instance of the efi_firmware_management_protocol
> + * @image_index:		a unique number identifying the firmware image
> + * @image:			pointer to the buffer containing the firmware
> + *				image to be updated
> + * @image_size:			pointer to size of the firmware image
> + * @vendor_code:		pointer to the vendor specific update policy
> + * @completion:			function pointer that can be invoked to report
> + *				firmware update progress
> + * @abort_reason:		string providing details if update is aborted
> + *
> + * Implement the set_image function of the efi_firmware_management_protocol
> + * for the platform. This updates the firmware image and authenticates the
> + * capsule, if authentication is enabled
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI qemu_arm64_fmp_set_image(
> +	struct efi_firmware_management_protocol *this,
> +	u8 image_index, const void *image,
> +	efi_uintn_t image_size, const void *vendor_code,
> +	efi_status_t (*progress)(efi_uintn_t completion),
> +	u16 **abort_reason)
> +{
> +	long fd, ret;
> +	efi_status_t status = EFI_SUCCESS;
> +	char *mode = "w+b";
> +
> +	EFI_ENTRY("%p %d %p %ld %p %p %p\n", this, image_index, image,
> +		  image_size, vendor_code, progress, abort_reason);
> +
> +	/*
> +	 * Put a hack here to offset the size of
> +	 * the FMP_PAYLOAD_HEADER that gets added
> +	 * by the GenerateCapsule script in edk2.
> +	 */
> +	image += 0x10;
> +	image_size -= 0x10;
> +
> +	/* Do all the sanity checks first */
> +	if (!image) {
> +		status = EFI_INVALID_PARAMETER;
> +		goto back;
> +	}
> +
> +	if (image_size == 0) {
> +		status = EFI_INVALID_PARAMETER;
> +		goto back;
> +	}
> +
> +	if (image_index != QEMU_UBOOT_IMAGE_INDEX) {
> +		status = EFI_INVALID_PARAMETER;
> +		goto back;
> +	}
> +
> +	/* Do the update */
> +	fd = smh_open(UBOOT_FILE, mode);
> +	if (fd == -1) {
> +		printf("Unable to open the firmware image for writing\n");
> +		status = EFI_DEVICE_ERROR;
> +		goto back;
> +	}
> +
> +	ret = smh_write(fd, (void *)image, image_size);
> +	if (ret == -1) {
> +		printf("Error writing to the firmware image!");
> +		smh_close(fd);
> +		status = EFI_DEVICE_ERROR;
> +		goto back;
> +	}
> +
> +	printf("Capsule Update Complete\n");
> +	smh_close(fd);
> +back:
> +	return EFI_EXIT(status);
> +}
> +
> +const struct efi_firmware_management_protocol efi_qemu_arm64_fmp = {
> +	.get_image_info = qemu_arm64_fmp_get_image_info,
> +	.set_image = qemu_arm64_fmp_set_image,
> +};
> +
> +efi_status_t arch_efi_load_capsule_drivers(void)
> +{
> +	efi_status_t ret;
> +
> +	ret = EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root,
> +					&efi_guid_firmware_management_protocol,
> +					&efi_qemu_arm64_fmp,
> +					NULL));
> +
> +	return ret;
> +}
>

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Sughosh Ganu]

On Fri, 1 May 2020 at 00:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> > Add documentation highlighting the steps for using the uefi capsule
> > update feature for updating the u-boot firmware image.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> UEFI capsule updates should be architecture independent. I would expect
> that the submitted code should work for x86, ARM, and RISC-V. Why does
> this documentation live under the ARM emulation tree?
>

While the implementation of the core capsule update functionality is indeed
architecture agnostic, this series is for implementing the routines of the
firmware management protocol, which is very much platform specific -- the
routines to perform the actual firmware update would be very much tied to
the platform for which the firmware is being updated. So Takahiro's patch
series, which adds the core capsule update changes is architecture
independent, while this series is adding the routines for the firmware
management protocol, which would be very much platform specific.

-sughosh


>
> Best regards
>
> Heinrich
>
> > ---
> >  doc/board/emulation/qemu-arm.rst | 112 +++++++++++++++++++++++++++++++
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/doc/board/emulation/qemu-arm.rst
> b/doc/board/emulation/qemu-arm.rst
> > index ca751d4af4..8649d593ed 100644
> > --- a/doc/board/emulation/qemu-arm.rst
> > +++ b/doc/board/emulation/qemu-arm.rst
> > @@ -80,3 +80,115 @@ can be enabled with the following command line
> parameters:
> >      -drive if=none,file=disk.img,id=mydisk -device
> nvme,drive=mydisk,serial=foo
> >
> >  These have been tested in QEMU 2.9.0 but should work in at least 2.5.0
> as well.
> > +
> > +Enabling Uefi Capsule Update feature
> > +------------------------------------
> > +
> > +Support has been added for the uefi capsule update feature which
> > +enables updating the u-boot image using the uefi firmware management
> > +protocol (fmp). The capsules are not passed to the firmware through
> > +the UpdateCapsule runtime service. Instead, capsule-on-disk
> > +functionality is used for fetching the capsule from the EFI System
> > +Partition (ESP). Currently, support has been added for the arm64
> > +target booting with arm trusted firmware. The This feature is enabled
> > +with the following configs::
> > +
> > +    CONFIG_EFI_CAPSULE_ON_DISK=y
> > +    CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> > +    CONFIG_CMD_EFIDEBUG=y
> > +
> > +The capsule file can be generated by using the GenerateCapsule.py
> > +script in edk2::
> > +
> > +    $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
> > +    <capsule_file_name> --fw-version <val> --lsv <val> --guid \
> > +    fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose --update-image-index
> \
> > +    <val> --verbose <u-boot.bin>
> > +
> > +
> > +As per the uefi specification, the capsule file needs to be placed on
> > +the EFI System Partition, under the EFI/UpdateCapsule/ directory. The
> > +EFI System Partition can be a virtio-blk-device.
> > +
> > +Before initiating the firmware update, the efi variables BootNext and
> > +BootXXXX need to be set. The BootXXXX variable needs to be pointing to
> > +the EFI System Partition which contains the capsule file. The
> > +BootNext and BootXXXX variables can be set using the efidebug
> > +command::
> > +
> > +    => efidebug boot add 0 Boot0000 virtio 0:1 <capsule_file_name>
> > +    => efidebug boot next 0
> > +
> > +The OsIndications efi variable needs to be set with the
> > +EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED flag set::
> > +
> > +    => setenv -e -nv -bs -rt OsIndications =0x04
> > +    => saveenv
> > +
> > +The capsule update function will be invoked on subsequent boot as part
> > +of the main_loop function. The updated u-boot image will be booted on
> > +subsequent boot.
> > +
> > +
> > +Enabling Capsule Authentication
> > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The uefi specification defines a way of authenticating the capsule to
> > +be updated by verifying the capsule signature. The capsule signature
> > +is computed and prepended to the capsule payload at the time of
> > +capsule generation. This signature is then verified by using the
> > +public key stored as part of the X509 certificate. This certificate is
> > +in the form of an efi signature list (esl) file, which is stored as an
> > +efi variable.
> > +
> > +The capsule authentication feature can be enabled through the
> > +following config, in addition to the configs listed above for capsule
> > +update::
> > +
> > +    CONFIG_EFI_CAPSULE_AUTHENTICATE=y
> > +
> > +The esl file can be generated as follows:
> > +
> > +1. Install utility commands on your host
> > +    * openssl
> > +    * efitools
> > +
> > +2. Create signing keys and certificate files on your host::
> > +
> > +        $ openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=CRT/ \
> > +                -keyout CRT.key -out CRT.crt -nodes -days 365
> > +        $ cert-to-efi-sig-list CRT.crt CRT.esl
> > +
> > +        $ openssl x509 -in CRT.crt -out CRT.cer -outform DER
> > +        $ openssl x509 -inform DER -in CRT.cer -outform PEM -out
> CRT.pub.pem
> > +
> > +        $ openssl pkcs12 -export -out CRT.pfx -inkey CRT.key -in CRT.crt
> > +        $ openssl pkcs12 -in CRT.pfx -nodes -out CRT.pem
> > +
> > +3. Store the esl file generated above as an efi variable::
> > +
> > +        => fatload virtio 0:1 <load_addr> EFI/CRT.esl
> > +        => setenv -e -nv -bs -rt -i <load_addr>,$filesize CRT
> > +
> > +        => setenv capsule_authentication_enabled 1
> > +        => setenv -e -nv -bs -rt OsIndication =0x04
> > +        => saveenv
> > +
> > +Setting the environment variable capsule_authentication_enabled
> > +enables the capsule authentication.
> > +
> > +4. The capsule file can be generated by using the GenerateCapsule.py
> > +   script in edk2::
> > +
> > +        $ ./BaseTools/BinWrappers/PosixLike/GenerateCapsule -e -o \
> > +     <capsule_file_name> --monotonic-count <val> --fw-version \
> > +     <val> --lsv <val> --guid \
> > +     fb90808a-ba9a-4d42-b9a2-a7a937144aee --verbose \
> > +     --update-image-index <val> --signer-private-cert \
> > +     /path/to/CRT.pem --trusted-public-cert \
> > +     /path/to/CRT.pub.pem --other-public-cert /path/to/CRT.pub.pem \
> > +     <u-boot.bin>
> > +
> > +Once the capsule has been generated, use the same instructions as
> > +mentioned above for placing the capsule on the EFI System Partition
> > +and subsequently to initiate the update.
> >
>
>

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Sughosh Ganu]

On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> > Add support for the get_image_info and set_image routines, which are
> > part of the efi firmware management protocol.
> >
> > The current implementation uses the set_image routine for updating the
> > u-boot binary image for the qemu arm64 platform. This is supported
> > using the capsule-on-disk feature of the uefi specification, wherein
> > the firmware image to be updated is placed on the efi system partition
> > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
> > added for updating the u-boot image on platforms booting with arm
> > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
> > payload(bl33.bin).
> >
> > The feature can be enabled by the following config options
> >
> > CONFIG_EFI_CAPSULE_ON_DISK=y
> > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>
> U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
> RISC-V. Please, come up with an architecture independent solution.
>

Please check the explanation that I gave in the other mail. If you check
the patch series, the actual capsule authentication logic has been kept
architecture agnostic, in efi_capsule.c. The fmp protocol is very much
intended for allowing platforms to define their firmware update routines.
Edk2 also has platform specific implementation of the fmp protocol under
the edk2-platforms directory.

-sughosh


>
> Best regards
>
> Heinrich
>
> > ---
> >  board/emulation/qemu-arm/Kconfig        |  12 ++
> >  board/emulation/qemu-arm/Makefile       |   1 +
> >  board/emulation/qemu-arm/qemu_efi_fmp.c | 210 ++++++++++++++++++++++++
> >  3 files changed, 223 insertions(+)
> >  create mode 100644 board/emulation/qemu-arm/qemu_efi_fmp.c
> >
> > diff --git a/board/emulation/qemu-arm/Kconfig
> b/board/emulation/qemu-arm/Kconfig
> > index 02ae4d9884..f1b2de8e41 100644
> > --- a/board/emulation/qemu-arm/Kconfig
> > +++ b/board/emulation/qemu-arm/Kconfig
> > @@ -11,3 +11,15 @@ config BOARD_SPECIFIC_OPTIONS # dummy
> >       imply VIRTIO_BLK
> >
> >  endif
> > +
> > +if TARGET_QEMU_ARM_64BIT && TFABOOT
> > +
> > +config EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > +     bool "EFI Firmware Management protocol for Qemu arm64 platform"
> > +     depends on EFI_CAPSULE_ON_DISK
> > +     default n
> > +     help
> > +       Select this option for enabling firmware management protocol
> > +       for qemu arm64 platform
> > +
> > +endif
> > diff --git a/board/emulation/qemu-arm/Makefile
> b/board/emulation/qemu-arm/Makefile
> > index a22d1237ff..c95ac6d233 100644
> > --- a/board/emulation/qemu-arm/Makefile
> > +++ b/board/emulation/qemu-arm/Makefile
> > @@ -1,3 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0+
> >
> >  obj-y        += qemu-arm.o
> > +obj-$(CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL) += qemu_efi_fmp.o
> > diff --git a/board/emulation/qemu-arm/qemu_efi_fmp.c
> b/board/emulation/qemu-arm/qemu_efi_fmp.c
> > new file mode 100644
> > index 0000000000..9baea94e6c
> > --- /dev/null
> > +++ b/board/emulation/qemu-arm/qemu_efi_fmp.c
> > @@ -0,0 +1,210 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2020, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <charset.h>
> > +#include <efi_api.h>
> > +#include <efi_loader.h>
> > +#include <malloc.h>
> > +#include <semihosting.h>
> > +
> > +#define QEMU_UBOOT_IMAGE_INDEX       0x1
> > +#define QEMU_UBOOT_IMAGE     0x1
> > +
> > +#define UBOOT_FILE           "bl33.bin"
> > +
> > +#define EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID \
> > +     EFI_GUID(0xfb90808a, 0xba9a, 0x4d42, 0xb9, 0xa2, \
> > +              0xa7, 0xa9, 0x37, 0x14, 0x4a, 0xee)
> > +
> > +/**
> > + * qemu_arm64_fmp_get_image_info() - Implement get_image_info
> > + *
> > + * @this:                    instance of the
> efi_firmware_management_protocol
> > + * @image_info_size:         pointer to the size of image_info buffer
> > + * @image_info:                      pointer to buffer which contains
> info on the
> > + *                           image
> > + * @desc_version:            pointer to version number of the
> > + *                           efi_firmware_image_descriptor
> > + * @desc_count:                      pointer to the number of
> descriptors of the
> > + *                           firmware images on the device
> > + * @desc_size:                       pointer to the size of an
> individual descriptor
> > + * @package_version:         version for all firmware images on the
> device
> > + * @package_version_name:    string representing the package version
> name
> > + *
> > + * Implement the get_image_info function of the
> > + * efi_firmware_management_protocol for the platform.
> > + *
> > + * Return: status code
> > + */
> > +static efi_status_t EFIAPI qemu_arm64_fmp_get_image_info(
> > +     struct efi_firmware_management_protocol *this,
> > +     efi_uintn_t *image_info_size,
> > +     struct efi_firmware_image_descriptor *image_info,
> > +     u32 *desc_version, u8 *desc_count,
> > +     efi_uintn_t *desc_size, u32 *package_version,
> > +     u16 **package_version_name)
> > +{
> > +     efi_status_t status = EFI_SUCCESS;
> > +     u16 *image_id_name;
> > +     const char *image_name = "Qemu Aarch64 U-Boot";
> > +     const efi_guid_t image_guid =
> EFI_FMP_QEMU_ARM64_UBOOT_CAPSULE_ID_GUID;
> > +
> > +     EFI_ENTRY("%p %p %p %p %p %p %p %p\n", this, image_info_size,
> > +               image_info, desc_version, desc_count, desc_size,
> > +               package_version, package_version_name);
> > +
> > +     /* Sanity checks */
> > +     if (*image_info_size && !image_info) {
> > +             status = EFI_INVALID_PARAMETER;
> > +             goto back;
> > +     }
> > +
> > +     if (*image_info_size &&
> > +         (!desc_version || !desc_count || !desc_size)) {
> > +             status = EFI_INVALID_PARAMETER;
> > +             goto back;
> > +     }
> > +
> > +     if (*image_info_size < sizeof(*image_info)) {
> > +             *image_info_size = sizeof(*image_info);
> > +             status = EFI_BUFFER_TOO_SMALL;
> > +             goto back;
> > +     }
> > +
> > +     if (desc_version)
> > +             *desc_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION;
> > +
> > +     *desc_count = 0x1;
> > +     *desc_size = sizeof(*image_info);
> > +
> > +     if (package_version)
> > +             *package_version = 0xffffffff;
> > +
> > +     if (package_version_name)
> > +             *package_version_name = NULL;
> > +
> > +     image_info[0].image_type_id = image_guid;
> > +     image_info[0].image_id = QEMU_UBOOT_IMAGE;
> > +
> > +     image_id_name = malloc(40);
> > +     utf8_utf16_strcpy(&image_id_name, image_name);
> > +     image_info[0].image_id_name = image_id_name;
> > +
> > +     /* Todo: Get a mechanism to store version information */
> > +     image_info[0]. version = 0x1;
> > +     image_info[0].version_name = NULL;
> > +
> > +     /* Todo: Need to find a mechanism to get the image size */
> > +     image_info[0].size = 0;
> > +
> > +     image_info[0].attributes_supported =
> > +             EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> > +     image_info[0].attributes_setting =
> EFI_IMAGE_ATTRIBUTE_IMAGE_UPDATABLE;
> > +
> > +     image_info[0].lowest_supported_image_version = 1;
> > +     image_info[0].last_attempt_version = 0;
> > +     image_info[0].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +     image_info[0].hardware_instance = 0;
> > +
> > +back:
> > +     return EFI_EXIT(status);
> > +}
> > +
> > +/**
> > + * qemu_arm64_fmp_set_image() - Implement set_image
> > + *
> > + * @this:                    instance of the
> efi_firmware_management_protocol
> > + * @image_index:             a unique number identifying the firmware
> image
> > + * @image:                   pointer to the buffer containing the
> firmware
> > + *                           image to be updated
> > + * @image_size:                      pointer to size of the firmware
> image
> > + * @vendor_code:             pointer to the vendor specific update
> policy
> > + * @completion:                      function pointer that can be
> invoked to report
> > + *                           firmware update progress
> > + * @abort_reason:            string providing details if update is
> aborted
> > + *
> > + * Implement the set_image function of the
> efi_firmware_management_protocol
> > + * for the platform. This updates the firmware image and authenticates
> the
> > + * capsule, if authentication is enabled
> > + *
> > + * Return: status code
> > + */
> > +static efi_status_t EFIAPI qemu_arm64_fmp_set_image(
> > +     struct efi_firmware_management_protocol *this,
> > +     u8 image_index, const void *image,
> > +     efi_uintn_t image_size, const void *vendor_code,
> > +     efi_status_t (*progress)(efi_uintn_t completion),
> > +     u16 **abort_reason)
> > +{
> > +     long fd, ret;
> > +     efi_status_t status = EFI_SUCCESS;
> > +     char *mode = "w+b";
> > +
> > +     EFI_ENTRY("%p %d %p %ld %p %p %p\n", this, image_index, image,
> > +               image_size, vendor_code, progress, abort_reason);
> > +
> > +     /*
> > +      * Put a hack here to offset the size of
> > +      * the FMP_PAYLOAD_HEADER that gets added
> > +      * by the GenerateCapsule script in edk2.
> > +      */
> > +     image += 0x10;
> > +     image_size -= 0x10;
> > +
> > +     /* Do all the sanity checks first */
> > +     if (!image) {
> > +             status = EFI_INVALID_PARAMETER;
> > +             goto back;
> > +     }
> > +
> > +     if (image_size == 0) {
> > +             status = EFI_INVALID_PARAMETER;
> > +             goto back;
> > +     }
> > +
> > +     if (image_index != QEMU_UBOOT_IMAGE_INDEX) {
> > +             status = EFI_INVALID_PARAMETER;
> > +             goto back;
> > +     }
> > +
> > +     /* Do the update */
> > +     fd = smh_open(UBOOT_FILE, mode);
> > +     if (fd == -1) {
> > +             printf("Unable to open the firmware image for writing\n");
> > +             status = EFI_DEVICE_ERROR;
> > +             goto back;
> > +     }
> > +
> > +     ret = smh_write(fd, (void *)image, image_size);
> > +     if (ret == -1) {
> > +             printf("Error writing to the firmware image!");
> > +             smh_close(fd);
> > +             status = EFI_DEVICE_ERROR;
> > +             goto back;
> > +     }
> > +
> > +     printf("Capsule Update Complete\n");
> > +     smh_close(fd);
> > +back:
> > +     return EFI_EXIT(status);
> > +}
> > +
> > +const struct efi_firmware_management_protocol efi_qemu_arm64_fmp = {
> > +     .get_image_info = qemu_arm64_fmp_get_image_info,
> > +     .set_image = qemu_arm64_fmp_set_image,
> > +};
> > +
> > +efi_status_t arch_efi_load_capsule_drivers(void)
> > +{
> > +     efi_status_t ret;
> > +
> > +     ret = EFI_CALL(efi_install_multiple_protocol_interfaces(&efi_root,
> > +
>  &efi_guid_firmware_management_protocol,
> > +                                     &efi_qemu_arm64_fmp,
> > +                                     NULL));
> > +
> > +     return ret;
> > +}
> >
>
>

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Tom Rini]

On Fri, May 01, 2020 at 12:38:45AM +0530, Sughosh Ganu wrote:
> On Fri, 1 May 2020 at 00:07, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> 
> > On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> > > Add documentation highlighting the steps for using the uefi capsule
> > > update feature for updating the u-boot firmware image.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >
> > UEFI capsule updates should be architecture independent. I would expect
> > that the submitted code should work for x86, ARM, and RISC-V. Why does
> > this documentation live under the ARM emulation tree?
> >
> 
> While the implementation of the core capsule update functionality is indeed
> architecture agnostic, this series is for implementing the routines of the
> firmware management protocol, which is very much platform specific -- the
> routines to perform the actual firmware update would be very much tied to
> the platform for which the firmware is being updated. So Takahiro's patch
> series, which adds the core capsule update changes is architecture
> independent, while this series is adding the routines for the firmware
> management protocol, which would be very much platform specific.

Since we're talking QEMU here, how much of this can be easily dropped in
to QEMU x86_64 and QEMU RISC-V?  If not almost all of it, why?  Can it
be reworked as such?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200430/2286b017/attachment.sig>

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Sughosh Ganu]

On Fri, 1 May 2020 at 00:57, Tom Rini <trini@konsulko.com> wrote:

> On Fri, May 01, 2020 at 12:38:45AM +0530, Sughosh Ganu wrote:
> > On Fri, 1 May 2020 at 00:07, Heinrich Schuchardt <xypron.glpk@gmx.de>
> wrote:
> >
> > > On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> > > > Add documentation highlighting the steps for using the uefi capsule
> > > > update feature for updating the u-boot firmware image.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > >
> > > UEFI capsule updates should be architecture independent. I would expect
> > > that the submitted code should work for x86, ARM, and RISC-V. Why does
> > > this documentation live under the ARM emulation tree?
> > >
> >
> > While the implementation of the core capsule update functionality is
> indeed
> > architecture agnostic, this series is for implementing the routines of
> the
> > firmware management protocol, which is very much platform specific -- the
> > routines to perform the actual firmware update would be very much tied to
> > the platform for which the firmware is being updated. So Takahiro's patch
> > series, which adds the core capsule update changes is architecture
> > independent, while this series is adding the routines for the firmware
> > management protocol, which would be very much platform specific.
>
> Since we're talking QEMU here, how much of this can be easily dropped in
> to QEMU x86_64 and QEMU RISC-V?  If not almost all of it, why?  Can it
> be reworked as such?
>

I don't think it would be too difficult to extend it on other
architectures, provided there is some mechanism to access and overwrite the
u-boot binary file from the qemu target. It is currently being done using
the semihosting interface for the arm architecture. I am not aware if there
is an interface like semihosting for accessing the u-boot binary on the
other architectures that you mentioned. Will check on this.

-sughosh

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>
> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>     > Add support for the get_image_info and set_image routines, which are
>     > part of the efi firmware management protocol.
>     >
>     > The current implementation uses the set_image routine for updating the
>     > u-boot binary image for the qemu arm64 platform. This is supported
>     > using the capsule-on-disk feature of the uefi specification, wherein
>     > the firmware image to be updated is placed on the efi system partition
>     > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
>     > added for updating the u-boot image on platforms booting with arm
>     > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
>     > payload(bl33.bin).
>     >
>     > The feature can be enabled by the following config options
>     >
>     > CONFIG_EFI_CAPSULE_ON_DISK=y
>     > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>     >
>     > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>>
>
>     U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
>     RISC-V. Please, come up with an architecture independent solution.
>
>
> Please check the explanation that I gave in the other mail. If you check
> the patch series, the actual capsule authentication logic has been kept
> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
> intended for allowing platforms to define their firmware update
> routines. Edk2 also has platform specific implementation of the fmp
> protocol under the edk2-platforms directory.
>
> -sughosh
> ?
>

My idea is that for most platforms it will be enough to have a common
FMP implementation that consumes a capsule

* with one or more binaries
* a media device path, a start address, and a truncation flag
  for each of the binaries

The protocol implementation then will write the binaries to the device
paths:

* to an SD-Card or eMMC exposing the Block IO protocol
  for most devices
* to a file in case of the Raspberry Pi or the Sandbox or QEMU
  (and truncate it if the truncation flag is set)

If for some devices like a SPI flash we do not have a media device path
yet, then the only platform specific bit would be the block device
driver exposing the media device path.

Same with a semi-hosted file: just add a driver exposing it as a media
path with an EFI_BLOCK_IO_PROTOCOL.

For security reasons it may be advisable to make the device read-only
when reaching ExitBootServices() or even better before the first
execution of StartImage(). For this purpose we could use the Reset()
service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
service in the EFI_BLOCK_IO_PROTOCOL.

Best regards

Heinrich

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Grant Likely]

On 01/05/2020 10:33, Heinrich Schuchardt wrote:
> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>
>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>> <mailto:xypron.glpk@gmx.de>> wrote:
>>
>>      On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>      > Add support for the get_image_info and set_image routines, which are
>>      > part of the efi firmware management protocol.
>>      >
>>      > The current implementation uses the set_image routine for updating the
>>      > u-boot binary image for the qemu arm64 platform. This is supported
>>      > using the capsule-on-disk feature of the uefi specification, wherein
>>      > the firmware image to be updated is placed on the efi system partition
>>      > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
>>      > added for updating the u-boot image on platforms booting with arm
>>      > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
>>      > payload(bl33.bin).
>>      >
>>      > The feature can be enabled by the following config options
>>      >
>>      > CONFIG_EFI_CAPSULE_ON_DISK=y
>>      > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>      >
>>      > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>      <mailto:sughosh.ganu@linaro.org>>
>>
>>      U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
>>      RISC-V. Please, come up with an architecture independent solution.
>>
>>
>> Please check the explanation that I gave in the other mail. If you check
>> the patch series, the actual capsule authentication logic has been kept
>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>> intended for allowing platforms to define their firmware update
>> routines. Edk2 also has platform specific implementation of the fmp
>> protocol under the edk2-platforms directory.
>>
>> -sughosh
>>
>>
>
> My idea is that for most platforms it will be enough to have a common
> FMP implementation that consumes a capsule
>
> * with one or more binaries
> * a media device path, a start address, and a truncation flag
>    for each of the binaries
>
> The protocol implementation then will write the binaries to the device
> paths:
>
> * to an SD-Card or eMMC exposing the Block IO protocol
>    for most devices
> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>    (and truncate it if the truncation flag is set)

Does U-Boot have a common device path protocol that can be backed by
either a block device or a file on a filesystem? I didn't think it did.

In the mean time, there are at least three backends that the FMP is
going to have to deal with; the two you list above (block device & file)
and SMC backed when updating firmware is managed by the secure world.
This first implementation only handles the file-backed use case. Can we
start with that limitation and refactor when the block-device and SMC
use cases are added in? I would hate to see this functionality held up
on having to refactor other functionality in U-Boot.

> If for some devices like a SPI flash we do not have a media device path
> yet, then the only platform specific bit would be the block device
> driver exposing the media device path.
>
> Same with a semi-hosted file: just add a driver exposing it as a media
> path with an EFI_BLOCK_IO_PROTOCOL.

Sughosh and I chatted about this and took a look a the semihosting
driver. Right now it is a standalone component implementing only the
smhload command. It looks like it easily maps onto fstype_info
operations which would be a better fit than the block IO protocol. Am I
correct in assuming that would also make semihosting available to the
EFI_FILE_PROTOCOL? The FMP backend code could be common for all
filesystem targets, with the only platform specific bit being the path
to the firmware files.

How does that sound?

g.

> For security reasons it may be advisable to make the device read-only
> when reaching ExitBootServices() or even better before the first
> execution of StartImage(). For this purpose we could use the Reset()
> service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
> service in the EFI_BLOCK_IO_PROTOCOL.
>
> Best regards
>
> Heinrich
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 05.05.20 13:15, Grant Likely wrote:
>
>
> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>
>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>
>>> ???? On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>> ???? > Add support for the get_image_info and set_image routines,
>>> which are
>>> ???? > part of the efi firmware management protocol.
>>> ???? >
>>> ???? > The current implementation uses the set_image routine for
>>> updating the
>>> ???? > u-boot binary image for the qemu arm64 platform. This is
>>> supported
>>> ???? > using the capsule-on-disk feature of the uefi specification,
>>> wherein
>>> ???? > the firmware image to be updated is placed on the efi system
>>> partition
>>> ???? > as a efi capsule under EFI/UpdateCapsule/ directory. Support
>>> has been
>>> ???? > added for updating the u-boot image on platforms booting with arm
>>> ???? > trusted firmware(tf-a), where the u-boot image gets booted as
>>> the BL33
>>> ???? > payload(bl33.bin).
>>> ???? >
>>> ???? > The feature can be enabled by the following config options
>>> ???? >
>>> ???? > CONFIG_EFI_CAPSULE_ON_DISK=y
>>> ???? > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>> ???? >
>>> ???? > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>> ???? <mailto:sughosh.ganu@linaro.org>>
>>>
>>> ???? U-Boot's UEFI subsystem should work in the same way for x86,
>>> ARM, and
>>> ???? RISC-V. Please, come up with an architecture independent solution.
>>>
>>>
>>> Please check the explanation that I gave in the other mail. If you check
>>> the patch series, the actual capsule authentication logic has been kept
>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>>> intended for allowing platforms to define their firmware update
>>> routines. Edk2 also has platform specific implementation of the fmp
>>> protocol under the edk2-platforms directory.
>>>
>>> -sughosh
>>>
>>>
>>
>> My idea is that for most platforms it will be enough to have a common
>> FMP implementation that consumes a capsule
>>
>> * with one or more binaries
>> * a media device path, a start address, and a truncation flag
>> ?? for each of the binaries
>>
>> The protocol implementation then will write the binaries to the device
>> paths:
>>
>> * to an SD-Card or eMMC exposing the Block IO protocol
>> ?? for most devices
>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>> ?? (and truncate it if the truncation flag is set)
>
> Does U-Boot have a common device path protocol that can be backed by
> either a block device or a file on a filesystem? I didn't think it did.

A block device, a partition, and a file all can be described by an UEFI
media device path.

>
> In the mean time, there are at least three backends that the FMP is
> going to have to deal with; the two you list above (block device & file)
> and SMC backed when updating firmware is managed by the secure world.
> This first implementation only handles the file-backed use case. Can we
> start with that limitation and refactor when the block-device and SMC
> use cases are added in? I would hate to see this functionality held up
> on having to refactor other functionality in U-Boot.

I would prefer one single FMP driver for all SMC use cases. Everything
device specific should be handled in the secure world.

Is there already a protocol defined for the communication of capsule
updates between the firmware and the secure monitor, e.g. in EDK2?

Would we simply use the UpdateCapsule() call parameters and pass them
via an SMC call?

>
>> If for some devices like a SPI flash we do not have a media device path
>> yet, then the only platform specific bit would be the block device
>> driver exposing the media device path.
>>
>> Same with a semi-hosted file: just add a driver exposing it as a media
>> path with an EFI_BLOCK_IO_PROTOCOL.
>
> Sughosh and I chatted about this and took a look a the semihosting
> driver. Right now it is a standalone component implementing only the
> smhload command. It looks like it easily maps onto fstype_info
> operations which would be a better fit than the block IO protocol. Am I
> correct in assuming that would also make semihosting available to the
> EFI_FILE_PROTOCOL? The FMP backend code could be common for all
> filesystem targets, with the only platform specific bit being the path
> to the firmware files.

According to my call with Sughosh the whole semihosting thing is about
providing a testing possibility not about any real use case.

On QEMU you can easily mount an image as block device using parameters
of the qemu-system-* command. On the mounted image/block-device you can
test both file and block device based firmware updates. On the sandbox
you could use the 'host bind' command for mounting an image.

For creating a Python unit test using the sandbox is the best fit.
Takahiro already did this for his secure boot patches.

Best regards

Heinrich

>
> How does that sound?
>
> g.
>
>> For security reasons it may be advisable to make the device read-only
>> when reaching ExitBootServices() or even better before the first
>> execution of StartImage(). For this purpose we could use the Reset()
>> service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
>> service in the EFI_BLOCK_IO_PROTOCOL.
>>
>> Best regards
>>
>> Heinrich
>>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Grant Likely]

On 05/05/2020 18:04, Heinrich Schuchardt wrote:
> On 05.05.20 13:15, Grant Likely wrote:
>>
>>
>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>>
>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>>
>>>>       On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>       > Add support for the get_image_info and set_image routines,
>>>> which are
>>>>       > part of the efi firmware management protocol.
>>>>       >
>>>>       > The current implementation uses the set_image routine for
>>>> updating the
>>>>       > u-boot binary image for the qemu arm64 platform. This is
>>>> supported
>>>>       > using the capsule-on-disk feature of the uefi specification,
>>>> wherein
>>>>       > the firmware image to be updated is placed on the efi system
>>>> partition
>>>>       > as a efi capsule under EFI/UpdateCapsule/ directory. Support
>>>> has been
>>>>       > added for updating the u-boot image on platforms booting with arm
>>>>       > trusted firmware(tf-a), where the u-boot image gets booted as
>>>> the BL33
>>>>       > payload(bl33.bin).
>>>>       >
>>>>       > The feature can be enabled by the following config options
>>>>       >
>>>>       > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>       > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>>       >
>>>>       > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>>       <mailto:sughosh.ganu@linaro.org>>
>>>>
>>>>       U-Boot's UEFI subsystem should work in the same way for x86,
>>>> ARM, and
>>>>       RISC-V. Please, come up with an architecture independent solution.
>>>>
>>>>
>>>> Please check the explanation that I gave in the other mail. If you check
>>>> the patch series, the actual capsule authentication logic has been kept
>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>>>> intended for allowing platforms to define their firmware update
>>>> routines. Edk2 also has platform specific implementation of the fmp
>>>> protocol under the edk2-platforms directory.
>>>>
>>>> -sughosh
>>>>
>>>>
>>>
>>> My idea is that for most platforms it will be enough to have a common
>>> FMP implementation that consumes a capsule
>>>
>>> * with one or more binaries
>>> * a media device path, a start address, and a truncation flag
>>>     for each of the binaries
>>>
>>> The protocol implementation then will write the binaries to the device
>>> paths:
>>>
>>> * to an SD-Card or eMMC exposing the Block IO protocol
>>>     for most devices
>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>>     (and truncate it if the truncation flag is set)
>>
>> Does U-Boot have a common device path protocol that can be backed by
>> either a block device or a file on a filesystem? I didn't think it did.
>
> A block device, a partition, and a file all can be described by an UEFI
> media device path.

Sure, from a UEFI media path, but does the underlying U-Boot
implementation have that abstraction?

>> In the mean time, there are at least three backends that the FMP is
>> going to have to deal with; the two you list above (block device & file)
>> and SMC backed when updating firmware is managed by the secure world.
>> This first implementation only handles the file-backed use case. Can we
>> start with that limitation and refactor when the block-device and SMC
>> use cases are added in? I would hate to see this functionality held up
>> on having to refactor other functionality in U-Boot.
>
> I would prefer one single FMP driver for all SMC use cases. Everything
> device specific should be handled in the secure world.

Not all platforms will be able to put firmware update into the secure
world. For instance, Not many Arm v7 platforms have trustzone accessible
to open source developers. On non-secure platforms (e.g., anything that
loads firmware from a regular filesystem on SD or eMMC) it doesn't make
much sense to loop out to secure firmware when U-Boot owns the
filesystem and the secure world would then need to coordinate with
U-Boot to commit the writes.

All of the code can certainly be in the same location, but I do think
there are three distinct generic backends for firmware updates:
- normal-world file-backed (using filesystem)
- normal-world block-backed (offsets from start of device)
- secure device backed (needs to go into secure world for unpacking and
processing regardless)

There doesn't need to be platform-specific code in any of those back
ends, but they do have different behaviour.

>
> Is there already a protocol defined for the communication of capsule
> updates between the firmware and the secure monitor, e.g. in EDK2?

Nothing defined yet (see below)

> Would we simply use the UpdateCapsule() call parameters and pass them
> via an SMC call?

If secure world is handling the update? Yes, I think a thin
UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
runtime becomes feasible on U-Boot. There are a couple of people inside
Arm looking at possible interfaces. In this situation there is very
little done in normal-world at all.

[...]

>
> According to my call with Sughosh the whole semihosting thing is about
> providing a testing possibility not about any real use case.

Yes, it's for testing, but it is particularly valuable testing because
it allows the host filesystem to be exposed into QEMU. Exposing
semihosting as a generic fstype_info in U-Boot is generically useful
apart from this entire discussion!  :-)

If we rework the semihosting code to be just another FS driver, then I
think it is just an implementation detail on the file-backed firmware
update path.

> On QEMU you can easily mount an image as block device using parameters
> of the qemu-system-* command. On the mounted image/block-device you can
> test both file and block device based firmware updates. On the sandbox
> you could use the 'host bind' command for mounting an image.

Similar to the above, this would be an implementation detail on the
block-device backed firmware update path. We need both.

g.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 05.05.20 19:23, Grant Likely wrote:
>
>
> On 05/05/2020 18:04, Heinrich Schuchardt wrote:
>> On 05.05.20 13:15, Grant Likely wrote:
>>>
>>>
>>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>>>
>>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>>>
>>>>> ????? On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>> ????? > Add support for the get_image_info and set_image routines,
>>>>> which are
>>>>> ????? > part of the efi firmware management protocol.
>>>>> ????? >
>>>>> ????? > The current implementation uses the set_image routine for
>>>>> updating the
>>>>> ????? > u-boot binary image for the qemu arm64 platform. This is
>>>>> supported
>>>>> ????? > using the capsule-on-disk feature of the uefi specification,
>>>>> wherein
>>>>> ????? > the firmware image to be updated is placed on the efi system
>>>>> partition
>>>>> ????? > as a efi capsule under EFI/UpdateCapsule/ directory. Support
>>>>> has been
>>>>> ????? > added for updating the u-boot image on platforms booting
>>>>> with arm
>>>>> ????? > trusted firmware(tf-a), where the u-boot image gets booted as
>>>>> the BL33
>>>>> ????? > payload(bl33.bin).
>>>>> ????? >
>>>>> ????? > The feature can be enabled by the following config options
>>>>> ????? >
>>>>> ????? > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>> ????? > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>>> ????? >
>>>>> ????? > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>>> ????? <mailto:sughosh.ganu@linaro.org>>
>>>>>
>>>>> ????? U-Boot's UEFI subsystem should work in the same way for x86,
>>>>> ARM, and
>>>>> ????? RISC-V. Please, come up with an architecture independent
>>>>> solution.
>>>>>
>>>>>
>>>>> Please check the explanation that I gave in the other mail. If you
>>>>> check
>>>>> the patch series, the actual capsule authentication logic has been
>>>>> kept
>>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>>>>> intended for allowing platforms to define their firmware update
>>>>> routines. Edk2 also has platform specific implementation of the fmp
>>>>> protocol under the edk2-platforms directory.
>>>>>
>>>>> -sughosh
>>>>>
>>>>>
>>>>
>>>> My idea is that for most platforms it will be enough to have a common
>>>> FMP implementation that consumes a capsule
>>>>
>>>> * with one or more binaries
>>>> * a media device path, a start address, and a truncation flag
>>>> ??? for each of the binaries
>>>>
>>>> The protocol implementation then will write the binaries to the device
>>>> paths:
>>>>
>>>> * to an SD-Card or eMMC exposing the Block IO protocol
>>>> ??? for most devices
>>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>>> ??? (and truncate it if the truncation flag is set)
>>>
>>> Does U-Boot have a common device path protocol that can be backed by
>>> either a block device or a file on a filesystem? I didn't think it did.
>>
>> A block device, a partition, and a file all can be described by an UEFI
>> media device path.
>
> Sure, from a UEFI media path, but does the underlying U-Boot
> implementation have that abstraction?


With a file media path we can find the partition device path on which
the Simple file protocol is already installed by U-Boot.

>
>>> In the mean time, there are at least three backends that the FMP is
>>> going to have to deal with; the two you list above (block device & file)
>>> and SMC backed when updating firmware is managed by the secure world.
>>> This first implementation only handles the file-backed use case. Can we
>>> start with that limitation and refactor when the block-device and SMC
>>> use cases are added in? I would hate to see this functionality held up
>>> on having to refactor other functionality in U-Boot.
>>
>> I would prefer one single FMP driver for all SMC use cases. Everything
>> device specific should be handled in the secure world.
>
> Not all platforms will be able to put firmware update into the secure
> world. For instance, Not many Arm v7 platforms have trustzone accessible
> to open source developers. On non-secure platforms (e.g., anything that
> loads firmware from a regular filesystem on SD or eMMC) it doesn't make
> much sense to loop out to secure firmware when U-Boot owns the
> filesystem and the secure world would then need to coordinate with
> U-Boot to commit the writes.
>
> All of the code can certainly be in the same location, but I do think
> there are three distinct generic backends for firmware updates:
> - normal-world file-backed (using filesystem)
> - normal-world block-backed (offsets from start of device)

These two could be in one instance of the FMP protocol.

> - secure device backed (needs to go into secure world for unpacking and
> processing regardless)
>
> There doesn't need to be platform-specific code in any of those back
> ends, but they do have different behaviour.
>
>>
>> Is there already a protocol defined for the communication of capsule
>> updates between the firmware and the secure monitor, e.g. in EDK2?
>
> Nothing defined yet (see below)
>
>> Would we simply use the UpdateCapsule() call parameters and pass them
>> via an SMC call?
>
> If secure world is handling the update? Yes, I think a thin
> UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
> runtime becomes feasible on U-Boot. There are a couple of people inside
> Arm looking at possible interfaces. In this situation there is very
> little done in normal-world at all.
>
> [...]
>
>>
>> According to my call with Sughosh the whole semihosting thing is about
>> providing a testing possibility not about any real use case.
>
> Yes, it's for testing, but it is particularly valuable testing because
> it allows the host filesystem to be exposed into QEMU. Exposing
> semihosting as a generic fstype_info in U-Boot is generically useful
> apart from this entire discussion!? :-)
>
> If we rework the semihosting code to be just another FS driver, then I
> think it is just an implementation detail on the file-backed firmware
> update path.

In this case it could be completely separate from this patch series.

Where do you see the benefit of semi-hosting compared to mounting an image?

Best regards

Heinrich

>
>> On QEMU you can easily mount an image as block device using parameters
>> of the qemu-system-* command. On the mounted image/block-device you can
>> test both file and block device based firmware updates. On the sandbox
>> you could use the 'host bind' command for mounting an image.
>
> Similar to the above, this would be an implementation detail on the
> block-device backed firmware update path. We need both.
>
> g.
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Grant Likely]

On 05/05/2020 18:57, Heinrich Schuchardt wrote:
> On 05.05.20 19:23, Grant Likely wrote:
>>
>>
>> On 05/05/2020 18:04, Heinrich Schuchardt wrote:
>>> On 05.05.20 13:15, Grant Likely wrote:
>>>>
>>>>
>>>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>>>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>>>>
>>>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>>>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>>>>
>>>>>>        On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>>>        > Add support for the get_image_info and set_image routines,
>>>>>> which are
>>>>>>        > part of the efi firmware management protocol.
>>>>>>        >
>>>>>>        > The current implementation uses the set_image routine for
>>>>>> updating the
>>>>>>        > u-boot binary image for the qemu arm64 platform. This is
>>>>>> supported
>>>>>>        > using the capsule-on-disk feature of the uefi specification,
>>>>>> wherein
>>>>>>        > the firmware image to be updated is placed on the efi system
>>>>>> partition
>>>>>>        > as a efi capsule under EFI/UpdateCapsule/ directory. Support
>>>>>> has been
>>>>>>        > added for updating the u-boot image on platforms booting
>>>>>> with arm
>>>>>>        > trusted firmware(tf-a), where the u-boot image gets booted as
>>>>>> the BL33
>>>>>>        > payload(bl33.bin).
>>>>>>        >
>>>>>>        > The feature can be enabled by the following config options
>>>>>>        >
>>>>>>        > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>>>        > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>>>>        >
>>>>>>        > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>>>>        <mailto:sughosh.ganu@linaro.org>>
>>>>>>
>>>>>>        U-Boot's UEFI subsystem should work in the same way for x86,
>>>>>> ARM, and
>>>>>>        RISC-V. Please, come up with an architecture independent
>>>>>> solution.
>>>>>>
>>>>>>
>>>>>> Please check the explanation that I gave in the other mail. If you
>>>>>> check
>>>>>> the patch series, the actual capsule authentication logic has been
>>>>>> kept
>>>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>>>>>> intended for allowing platforms to define their firmware update
>>>>>> routines. Edk2 also has platform specific implementation of the fmp
>>>>>> protocol under the edk2-platforms directory.
>>>>>>
>>>>>> -sughosh
>>>>>>
>>>>>>
>>>>>
>>>>> My idea is that for most platforms it will be enough to have a common
>>>>> FMP implementation that consumes a capsule
>>>>>
>>>>> * with one or more binaries
>>>>> * a media device path, a start address, and a truncation flag
>>>>>      for each of the binaries
>>>>>
>>>>> The protocol implementation then will write the binaries to the device
>>>>> paths:
>>>>>
>>>>> * to an SD-Card or eMMC exposing the Block IO protocol
>>>>>      for most devices
>>>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>>>>      (and truncate it if the truncation flag is set)
>>>>
>>>> Does U-Boot have a common device path protocol that can be backed by
>>>> either a block device or a file on a filesystem? I didn't think it did.
>>>
>>> A block device, a partition, and a file all can be described by an UEFI
>>> media device path.
>>
>> Sure, from a UEFI media path, but does the underlying U-Boot
>> implementation have that abstraction?
>
>
> With a file media path we can find the partition device path on which
> the Simple file protocol is already installed by U-Boot.
>
>>
>>>> In the mean time, there are at least three backends that the FMP is
>>>> going to have to deal with; the two you list above (block device & file)
>>>> and SMC backed when updating firmware is managed by the secure world.
>>>> This first implementation only handles the file-backed use case. Can we
>>>> start with that limitation and refactor when the block-device and SMC
>>>> use cases are added in? I would hate to see this functionality held up
>>>> on having to refactor other functionality in U-Boot.
>>>
>>> I would prefer one single FMP driver for all SMC use cases. Everything
>>> device specific should be handled in the secure world.
>>
>> Not all platforms will be able to put firmware update into the secure
>> world. For instance, Not many Arm v7 platforms have trustzone accessible
>> to open source developers. On non-secure platforms (e.g., anything that
>> loads firmware from a regular filesystem on SD or eMMC) it doesn't make
>> much sense to loop out to secure firmware when U-Boot owns the
>> filesystem and the secure world would then need to coordinate with
>> U-Boot to commit the writes.
>>
>> All of the code can certainly be in the same location, but I do think
>> there are three distinct generic backends for firmware updates:
>> - normal-world file-backed (using filesystem)
>> - normal-world block-backed (offsets from start of device)
>
> These two could be in one instance of the FMP protocol.
>
>> - secure device backed (needs to go into secure world for unpacking and
>> processing regardless)
>>
>> There doesn't need to be platform-specific code in any of those back
>> ends, but they do have different behaviour.
>>
>>>
>>> Is there already a protocol defined for the communication of capsule
>>> updates between the firmware and the secure monitor, e.g. in EDK2?
>>
>> Nothing defined yet (see below)
>>
>>> Would we simply use the UpdateCapsule() call parameters and pass them
>>> via an SMC call?
>>
>> If secure world is handling the update? Yes, I think a thin
>> UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
>> runtime becomes feasible on U-Boot. There are a couple of people inside
>> Arm looking at possible interfaces. In this situation there is very
>> little done in normal-world at all.
>>
>> [...]
>>
>>>
>>> According to my call with Sughosh the whole semihosting thing is about
>>> providing a testing possibility not about any real use case.
>>
>> Yes, it's for testing, but it is particularly valuable testing because
>> it allows the host filesystem to be exposed into QEMU. Exposing
>> semihosting as a generic fstype_info in U-Boot is generically useful
>> apart from this entire discussion!  :-)
>>
>> If we rework the semihosting code to be just another FS driver, then I
>> think it is just an implementation detail on the file-backed firmware
>> update path.
>
> In this case it could be completely separate from this patch series.
>
> Where do you see the benefit of semi-hosting compared to mounting an image?

Both are important. We need test benches for both file and block device
update scenarios.

g.

>
> Best regards
>
> Heinrich
>
>>
>>> On QEMU you can easily mount an image as block device using parameters
>>> of the qemu-system-* command. On the mounted image/block-device you can
>>> test both file and block device based firmware updates. On the sandbox
>>> you could use the 'host bind' command for mounting an image.
>>
>> Similar to the above, this would be an implementation detail on the
>> block-device backed firmware update path. We need both.
>>
>> g.
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose the
>> contents to any other person, use it for any purpose, or store or copy
>> the information in any medium. Thank you.
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Akashi Takahiro]

On Fri, May 01, 2020 at 11:17:27AM +0530, Sughosh Ganu wrote:
> On Fri, 1 May 2020 at 00:57, Tom Rini <trini@konsulko.com> wrote:
> 
> > On Fri, May 01, 2020 at 12:38:45AM +0530, Sughosh Ganu wrote:
> > > On Fri, 1 May 2020 at 00:07, Heinrich Schuchardt <xypron.glpk@gmx.de>
> > wrote:
> > >
> > > > On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> > > > > Add documentation highlighting the steps for using the uefi capsule
> > > > > update feature for updating the u-boot firmware image.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > >
> > > > UEFI capsule updates should be architecture independent. I would expect
> > > > that the submitted code should work for x86, ARM, and RISC-V. Why does
> > > > this documentation live under the ARM emulation tree?
> > > >
> > >
> > > While the implementation of the core capsule update functionality is
> > indeed
> > > architecture agnostic, this series is for implementing the routines of
> > the
> > > firmware management protocol, which is very much platform specific -- the
> > > routines to perform the actual firmware update would be very much tied to
> > > the platform for which the firmware is being updated. So Takahiro's patch
> > > series, which adds the core capsule update changes is architecture
> > > independent, while this series is adding the routines for the firmware
> > > management protocol, which would be very much platform specific.
> >
> > Since we're talking QEMU here, how much of this can be easily dropped in
> > to QEMU x86_64 and QEMU RISC-V?  If not almost all of it, why?  Can it
> > be reworked as such?
> >
> 
> I don't think it would be too difficult to extend it on other
> architectures, provided there is some mechanism to access and overwrite the
> u-boot binary file from the qemu target. It is currently being done using
> the semihosting interface for the arm architecture. I am not aware if there
> is an interface like semihosting for accessing the u-boot binary on the
> other architectures that you mentioned. Will check on this.

Obviously, another choice would be my FIT-based FMP[1]
as it uses update_tftp(), more specifically dfu_tftp_write(),
internally.

[1] https://lists.denx.de/pipermail/u-boot/2020-April/408767.html

Thanks,
-Takahiro Akashi


> -sughosh

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Akashi Takahiro]

On Fri, May 01, 2020 at 11:33:42AM +0200, Heinrich Schuchardt wrote:
> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
> >
> > On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> >     > Add support for the get_image_info and set_image routines, which are
> >     > part of the efi firmware management protocol.
> >     >
> >     > The current implementation uses the set_image routine for updating the
> >     > u-boot binary image for the qemu arm64 platform. This is supported
> >     > using the capsule-on-disk feature of the uefi specification, wherein
> >     > the firmware image to be updated is placed on the efi system partition
> >     > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
> >     > added for updating the u-boot image on platforms booting with arm
> >     > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
> >     > payload(bl33.bin).
> >     >
> >     > The feature can be enabled by the following config options
> >     >
> >     > CONFIG_EFI_CAPSULE_ON_DISK=y
> >     > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> >     >
> >     > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
> >     <mailto:sughosh.ganu@linaro.org>>
> >
> >     U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
> >     RISC-V. Please, come up with an architecture independent solution.
> >
> >
> > Please check the explanation that I gave in the other mail. If you check
> > the patch series, the actual capsule authentication logic has been kept
> > architecture agnostic, in efi_capsule.c. The fmp protocol is very much
> > intended for allowing platforms to define their firmware update
> > routines. Edk2 also has platform specific implementation of the fmp
> > protocol under the edk2-platforms directory.
> >
> > -sughosh
> > ?
> >
> 
> My idea is that for most platforms it will be enough to have a common
> FMP implementation that consumes a capsule
> 
> * with one or more binaries

Does this assumption apply to most platforms?
If so ("one"),

> * a media device path, a start address, and a truncation flag
>   for each of the binaries

my FIT-based patch[1] meets this assumption and there already
are backend drivers for many media (but not for semihosting :)
as dfu.
(I see little reason to re-invent another set of backend drivers.)

[1] https://lists.denx.de/pipermail/u-boot/2020-April/408767.html


> The protocol implementation then will write the binaries to the device
> paths:
> 
> * to an SD-Card or eMMC exposing the Block IO protocol
>   for most devices
> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>   (and truncate it if the truncation flag is set)
> 
> If for some devices like a SPI flash we do not have a media device path
> yet, then the only platform specific bit would be the block device
> driver exposing the media device path.
> 
> Same with a semi-hosted file: just add a driver exposing it as a media
> path with an EFI_BLOCK_IO_PROTOCOL.
> 
> For security reasons it may be advisable to make the device read-only
> when reaching ExitBootServices() or even better before the first
> execution of StartImage(). For this purpose we could use the Reset()
> service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
> service in the EFI_BLOCK_IO_PROTOCOL.
> 
> Best regards
> 
> Heinrich

[PATCH 5/8] efi_loader: Make the pkcs7 header parsing function an extern

[Akashi Takahiro]

On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote:
> The pkcs7 header parsing functionality is pretty generic, and can be
> used by other features like capsule authentication. Make the function
> as an extern, also changing it's name to efi_parse_pkcs7_header.

The patch itself is fine to me, but is "pkcs7 header" a common term?

-Takahiro Akashi

> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/efi_loader.h           |  2 +
>  lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_variable.c  | 82 +---------------------------------
>  3 files changed, 82 insertions(+), 80 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index b7638d5825..8d923451ce 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void);
>  
>  bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>  		     WIN_CERTIFICATE **auth, size_t *auth_len);
> +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen);
> +
>  #endif /* CONFIG_EFI_SECURE_BOOT */
>  
>  #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index bf6f39aab2..9897f5418e 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
>  
>  #ifdef CONFIG_EFI_SECURE_BOOT
>  
> +static u8 pkcs7_hdr[] = {
> +	/* SEQUENCE */
> +	0x30, 0x82, 0x05, 0xc7,
> +	/* OID: pkcs7-signedData */
> +	0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> +	/* Context Structured? */
> +	0xa0, 0x82, 0x05, 0xb8,
> +};
> +
> +/**
> + * efi_parse_pkcs7_header - parse a signature in variable
> + * @buf:	Pointer to the payload's value
> + * @buflen:	Length of @buf
> + *
> + * Parse a signature embedded in variable's value and instantiate
> + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> + * pkcs7's signedData, some header needed be prepended for correctly
> + * parsing authentication data, particularly for variable's.
> + *
> + * Return:	Pointer to pkcs7_message structure on success, NULL on error
> + */
> +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen)
> +{
> +	u8 *ebuf;
> +	size_t ebuflen, len;
> +	struct pkcs7_message *msg;
> +
> +	/*
> +	 * This is the best assumption to check if the binary is
> +	 * already in a form of pkcs7's signedData.
> +	 */
> +	if (buflen > sizeof(pkcs7_hdr) &&
> +	    !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> +		msg = pkcs7_parse_message(buf, buflen);
> +		goto out;
> +	}
> +
> +	/*
> +	 * Otherwise, we should add a dummy prefix sequence for pkcs7
> +	 * message parser to be able to process.
> +	 * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> +	 * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> +	 * TODO:
> +	 * The header should be composed in a more refined manner.
> +	 */
> +	debug("Makeshift prefix added to authentication data\n");
> +	ebuflen = sizeof(pkcs7_hdr) + buflen;
> +	if (ebuflen <= 0x7f) {
> +		debug("Data is too short\n");
> +		return NULL;
> +	}
> +
> +	ebuf = malloc(ebuflen);
> +	if (!ebuf) {
> +		debug("Out of memory\n");
> +		return NULL;
> +	}
> +
> +	memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> +	memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> +	len = ebuflen - 4;
> +	ebuf[2] = (len >> 8) & 0xff;
> +	ebuf[3] = len & 0xff;
> +	len = ebuflen - 0x13;
> +	ebuf[0x11] = (len >> 8) & 0xff;
> +	ebuf[0x12] = len & 0xff;
> +
> +	msg = pkcs7_parse_message(ebuf, ebuflen);
> +
> +	free(ebuf);
> +
> +out:
> +	if (IS_ERR(msg))
> +		return NULL;
> +
> +	return msg;
> +}
> +
>  /**
>   * efi_hash_regions - calculate a hash value
>   * @regs:	List of regions
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 6c2dd82306..be34a2cadd 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void)
>  	return efi_secure_boot;
>  }
>  
> -#ifdef CONFIG_EFI_SECURE_BOOT
> -static u8 pkcs7_hdr[] = {
> -	/* SEQUENCE */
> -	0x30, 0x82, 0x05, 0xc7,
> -	/* OID: pkcs7-signedData */
> -	0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> -	/* Context Structured? */
> -	0xa0, 0x82, 0x05, 0xb8,
> -};
> -
> -/**
> - * efi_variable_parse_signature - parse a signature in variable
> - * @buf:	Pointer to variable's value
> - * @buflen:	Length of @buf
> - *
> - * Parse a signature embedded in variable's value and instantiate
> - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> - * pkcs7's signedData, some header needed be prepended for correctly
> - * parsing authentication data, particularly for variable's.
> - *
> - * Return:	Pointer to pkcs7_message structure on success, NULL on error
> - */
> -static struct pkcs7_message *efi_variable_parse_signature(const void *buf,
> -							  size_t buflen)
> -{
> -	u8 *ebuf;
> -	size_t ebuflen, len;
> -	struct pkcs7_message *msg;
> -
> -	/*
> -	 * This is the best assumption to check if the binary is
> -	 * already in a form of pkcs7's signedData.
> -	 */
> -	if (buflen > sizeof(pkcs7_hdr) &&
> -	    !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> -		msg = pkcs7_parse_message(buf, buflen);
> -		goto out;
> -	}
> -
> -	/*
> -	 * Otherwise, we should add a dummy prefix sequence for pkcs7
> -	 * message parser to be able to process.
> -	 * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> -	 * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> -	 * TODO:
> -	 * The header should be composed in a more refined manner.
> -	 */
> -	debug("Makeshift prefix added to authentication data\n");
> -	ebuflen = sizeof(pkcs7_hdr) + buflen;
> -	if (ebuflen <= 0x7f) {
> -		debug("Data is too short\n");
> -		return NULL;
> -	}
> -
> -	ebuf = malloc(ebuflen);
> -	if (!ebuf) {
> -		debug("Out of memory\n");
> -		return NULL;
> -	}
> -
> -	memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> -	memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> -	len = ebuflen - 4;
> -	ebuf[2] = (len >> 8) & 0xff;
> -	ebuf[3] = len & 0xff;
> -	len = ebuflen - 0x13;
> -	ebuf[0x11] = (len >> 8) & 0xff;
> -	ebuf[0x12] = len & 0xff;
> -
> -	msg = pkcs7_parse_message(ebuf, ebuflen);
> -
> -	free(ebuf);
> -
> -out:
> -	if (IS_ERR(msg))
> -		return NULL;
> -
> -	return msg;
> -}
> +#ifdef CONFIG_SECURE_BOOT
>  
>  /**
>   * efi_variable_authenticate - authenticate a variable
> @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
>  	/* variable's signature list */
>  	if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info))
>  		goto err;
> -	var_sig = efi_variable_parse_signature(auth->auth_info.cert_data,
> +	var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data,
>  					       auth->auth_info.hdr.dwLength
>  						   - sizeof(auth->auth_info));
>  	if (IS_ERR(var_sig)) {
> -- 
> 2.17.1
> 

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Akashi Takahiro]

Sughosh,

On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> Add support for authenticating uefi capsules. Most of the signature
> verification functionality is shared with the uefi secure boot
> feature.
> 
> The root certificate containing the public key used for the signature
> verification is stored as an efi variable, similar to the variables
> used for uefi secure boot. The root certificate is stored as an efi
> signature list(esl) file -- this file contains the x509 certificate
> which is the root certificate.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  include/efi_api.h              |  17 +++++
>  include/efi_loader.h           |   8 ++-
>  lib/efi_loader/Kconfig         |  16 +++++
>  lib/efi_loader/efi_capsule.c   | 126 +++++++++++++++++++++++++++++++++
>  lib/efi_loader/efi_signature.c |   4 +-
>  5 files changed, 167 insertions(+), 4 deletions(-)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index e518ffa838..8dfa479db4 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -1794,6 +1794,23 @@ struct efi_variable_authentication_2 {
>  	struct win_certificate_uefi_guid auth_info;
>  } __attribute__((__packed__));
>  
> +/**
> + * efi_firmware_image_authentication - Capsule authentication method
> + * descriptor
> + *
> + * This structure describes an authentication information for
> + * a capsule with IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED set
> + * and should be included as part of the capsule.
> + * Only EFI_CERT_TYPE_PKCS7_GUID is accepted.
> + *
> + * @monotonic_count: Count to prevent replay
> + * @auth_info:	Authentication info
> + */
> +struct efi_firmware_image_authentication {
> +	uint64_t monotonic_count;
> +	struct win_certificate_uefi_guid auth_info;
> +} __attribute__((__packed__));
> +
>  /**
>   * efi_signature_data - A format of signature
>   *
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 8d923451ce..897710ae3f 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -708,7 +708,7 @@ void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>  unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
>  efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>  
> -#ifdef CONFIG_EFI_SECURE_BOOT
> +#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
>  #include <image.h>
>  
>  /**
> @@ -783,7 +783,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>  		     WIN_CERTIFICATE **auth, size_t *auth_len);
>  struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen);
>  
> -#endif /* CONFIG_EFI_SECURE_BOOT */
> +#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */
>  
>  #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
>  /* Capsule update */
> @@ -798,6 +798,10 @@ efi_status_t EFIAPI efi_query_capsule_caps(
>  		u32 *reset_type);
>  #endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT */
>  
> +efi_status_t efi_capsule_authenticate(const void *capsule,
> +				      efi_uintn_t capsule_size,
> +				      void **image, efi_uintn_t *image_size);
> +
>  #ifdef CONFIG_EFI_CAPSULE_ON_DISK
>  #define EFI_CAPSULE_DIR L"\\EFI\\UpdateCapsule\\"
>  
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index ec2976ceba..245deabbed 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
>  	help
>  	  Define storage device, say 1:1, for storing FIT image
>  
> +config EFI_CAPSULE_AUTHENTICATE
> +	bool "Update Capsule authentication"
> +	depends on EFI_HAVE_CAPSULE_SUPPORT
> +	depends on EFI_CAPSULE_ON_DISK
> +	depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL

Do we need those two configurations?

> +	select SHA256
> +	select RSA
> +	select RSA_VERIFY
> +	select RSA_VERIFY_WITH_PKEY
> +	select X509_CERTIFICATE_PARSER
> +	select PKCS7_MESSAGE_PARSER
> +	default n
> +	help
> +	  Select this option if you want to enable capsule
> +	  authentication
> +
>  config EFI_DEVICE_PATH_TO_TEXT
>  	bool "Device path to text protocol"
>  	default y
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 931d363edc..a265d36ff0 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -12,6 +12,10 @@
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <sort.h>
> +#include "../lib/crypto/pkcs7_parser.h"
> +

As you might notice, the location was changed by
my recent patch.

> +#include <crypto/pkcs7.h>
> +#include <linux/err.h>
>  
>  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
>  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> @@ -245,6 +249,128 @@ out:
>  
>  	return ret;
>  }
> +
> +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> +
> +const efi_guid_t efi_guid_capsule_root_cert_guid =
> +	EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> +
> +__weak u16 *efi_get_truststore_name(void)
> +{
> +	return L"CRT";

This variable is not defined by UEFI specification, isn't it?
How can this variable be protected?

> +}
> +
> +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> +{
> +
> +	return &efi_guid_capsule_root_cert_guid;
> +}
> +
> +/**
> + * efi_capsule_authenticate() - Authenticate a uefi capsule
> + *
> + * @capsule:		Capsule file with the authentication
> + *			header
> + * @capsule_size:	Size of the entire capsule
> + * @image:		pointer to the image payload minus the
> + *			authentication header
> + * @image_size:		size of the image payload
> + *
> + * Authenticate the capsule signature with the public key contained
> + * in the root certificate stored as an efi environment variable
> + *
> + * Return: EFI_SUCCESS on successfull authentication or
> + *	   EFI_SECURITY_VIOLATION on authentication failure
> + */
> +efi_status_t efi_capsule_authenticate(const void *capsule,
> +				      efi_uintn_t capsule_size,
> +				      void **image, efi_uintn_t *image_size)
> +{
> +	uint64_t monotonic_count;
> +	struct efi_signature_store *truststore;
> +	struct pkcs7_message *capsule_sig;
> +	struct efi_image_regions *regs;
> +	struct efi_firmware_image_authentication *auth_hdr;
> +	efi_status_t status;
> +
> +	status = EFI_SECURITY_VIOLATION;
> +	capsule_sig = NULL;
> +	truststore = NULL;
> +	regs = NULL;
> +
> +	/* Sanity checks */
> +	if (capsule == NULL || capsule_size == 0)
> +		goto out;
> +
> +	auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> +	if (capsule_size < sizeof(*auth_hdr))
> +		goto out;
> +
> +	if (auth_hdr->auth_info.hdr.dwLength <=
> +	    offsetof(struct win_certificate_uefi_guid, cert_data))
> +		goto out;
> +
> +	if (guidcmp(&auth_hdr->auth_info.cert_type, &efi_guid_cert_type_pkcs7))
> +		goto out;
> +
> +	*image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> +		auth_hdr->auth_info.hdr.dwLength;
> +	*image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> +				      sizeof(auth_hdr->monotonic_count);
> +	memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> +	       sizeof(monotonic_count));
> +
> +	/* data to be digested */
> +	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2, 1);
> +	if (!regs)
> +		goto out;
> +
> +	regs->max = 2;
> +	efi_image_region_add(regs, (uint8_t *)*image,
> +			     (uint8_t *)*image + *image_size, 1);
> +
> +	efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> +			     (uint8_t *)&monotonic_count + sizeof(monotonic_count),
> +			     1);

Is the order of regions to be calculated correct?
It seems that 'monotonic_count' precedes 'image' itself
in a capsule header.

> +
> +	capsule_sig = efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> +					     auth_hdr->auth_info.hdr.dwLength
> +					     - sizeof(auth_hdr->auth_info));
> +	if (IS_ERR(capsule_sig)) {

As Patrick reported, ex-efi_variable_parse_signature()
returns NULL on error cases, this check should be "!capsule_sig."

Thanks,
-Takahiro Akashi

> +		debug("Parsing variable's pkcs7 header failed\n");
> +		capsule_sig = NULL;
> +		goto out;
> +	}
> +
> +	truststore = efi_sigstore_parse_sigdb(efi_get_truststore_name(),
> +					      efi_get_truststore_vendor());
> +	if (!truststore)
> +		goto out;
> +
> +	/* verify signature */
> +	if (efi_signature_verify_with_sigdb(regs, capsule_sig, truststore, NULL)) {
> +		debug("Verified\n");
> +	} else {
> +		debug("Verifying variable's signature failed\n");
> +		goto out;
> +	}
> +
> +	status = EFI_SUCCESS;
> +
> +out:
> +	efi_sigstore_free(truststore);
> +	pkcs7_free_message(capsule_sig);
> +	free(regs);
> +
> +	return status;
> +}
> +#else
> +efi_status_t efi_capsule_authenticate(const void *capsule, efi_uintn_t capsule_size,
> +				      void **image, efi_uintn_t *image_size)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +#endif /* CONFIG_EFI_CAPSULE_AUTHENTICATE */
>  #else
>  static efi_status_t efi_capsule_update_firmware(
>  		struct efi_capsule_header *capsule_data)
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 9897f5418e..4c722e0da9 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -23,7 +23,7 @@ const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
>  const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
>  const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
>  
> -#ifdef CONFIG_EFI_SECURE_BOOT
> +#if defined(CONFIG_EFI_SECURE_BOOT) || defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
>  
>  static u8 pkcs7_hdr[] = {
>  	/* SEQUENCE */
> @@ -871,4 +871,4 @@ err:
>  
>  	return NULL;
>  }
> -#endif /* CONFIG_EFI_SECURE_BOOT */
> +#endif /* CONFIG_EFI_SECURE_BOOT || CONFIG_EFI_CAPSULE_AUTHENTICATE */
> -- 
> 2.17.1
> 

[PATCH 5/8] efi_loader: Make the pkcs7 header parsing function an extern

[Sughosh Ganu]

On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi@linaro.org>
wrote:

> On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote:
> > The pkcs7 header parsing functionality is pretty generic, and can be
> > used by other features like capsule authentication. Make the function
> > as an extern, also changing it's name to efi_parse_pkcs7_header.
>
> The patch itself is fine to me, but is "pkcs7 header" a common term?
>

I haven't come across it in any other code base. I used it since in the
concept of a capsule, the signature is prepended to the capsule payload. If
you can think of a better name, please suggest so. I will change it in the
next version.

-sughosh


>
> -Takahiro Akashi
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  include/efi_loader.h           |  2 +
> >  lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++
> >  lib/efi_loader/efi_variable.c  | 82 +---------------------------------
> >  3 files changed, 82 insertions(+), 80 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index b7638d5825..8d923451ce 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void);
> >
> >  bool efi_image_parse(void *efi, size_t len, struct efi_image_regions
> **regp,
> >                    WIN_CERTIFICATE **auth, size_t *auth_len);
> > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t
> buflen);
> > +
> >  #endif /* CONFIG_EFI_SECURE_BOOT */
> >
> >  #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
> > diff --git a/lib/efi_loader/efi_signature.c
> b/lib/efi_loader/efi_signature.c
> > index bf6f39aab2..9897f5418e 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 =
> EFI_CERT_X509_SHA256_GUID;
> >
> >  #ifdef CONFIG_EFI_SECURE_BOOT
> >
> > +static u8 pkcs7_hdr[] = {
> > +     /* SEQUENCE */
> > +     0x30, 0x82, 0x05, 0xc7,
> > +     /* OID: pkcs7-signedData */
> > +     0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> > +     /* Context Structured? */
> > +     0xa0, 0x82, 0x05, 0xb8,
> > +};
> > +
> > +/**
> > + * efi_parse_pkcs7_header - parse a signature in variable
> > + * @buf:     Pointer to the payload's value
> > + * @buflen:  Length of @buf
> > + *
> > + * Parse a signature embedded in variable's value and instantiate
> > + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> > + * pkcs7's signedData, some header needed be prepended for correctly
> > + * parsing authentication data, particularly for variable's.
> > + *
> > + * Return:   Pointer to pkcs7_message structure on success, NULL on
> error
> > + */
> > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t
> buflen)
> > +{
> > +     u8 *ebuf;
> > +     size_t ebuflen, len;
> > +     struct pkcs7_message *msg;
> > +
> > +     /*
> > +      * This is the best assumption to check if the binary is
> > +      * already in a form of pkcs7's signedData.
> > +      */
> > +     if (buflen > sizeof(pkcs7_hdr) &&
> > +         !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> > +             msg = pkcs7_parse_message(buf, buflen);
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Otherwise, we should add a dummy prefix sequence for pkcs7
> > +      * message parser to be able to process.
> > +      * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> > +      * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> > +      * TODO:
> > +      * The header should be composed in a more refined manner.
> > +      */
> > +     debug("Makeshift prefix added to authentication data\n");
> > +     ebuflen = sizeof(pkcs7_hdr) + buflen;
> > +     if (ebuflen <= 0x7f) {
> > +             debug("Data is too short\n");
> > +             return NULL;
> > +     }
> > +
> > +     ebuf = malloc(ebuflen);
> > +     if (!ebuf) {
> > +             debug("Out of memory\n");
> > +             return NULL;
> > +     }
> > +
> > +     memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> > +     memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> > +     len = ebuflen - 4;
> > +     ebuf[2] = (len >> 8) & 0xff;
> > +     ebuf[3] = len & 0xff;
> > +     len = ebuflen - 0x13;
> > +     ebuf[0x11] = (len >> 8) & 0xff;
> > +     ebuf[0x12] = len & 0xff;
> > +
> > +     msg = pkcs7_parse_message(ebuf, ebuflen);
> > +
> > +     free(ebuf);
> > +
> > +out:
> > +     if (IS_ERR(msg))
> > +             return NULL;
> > +
> > +     return msg;
> > +}
> > +
> >  /**
> >   * efi_hash_regions - calculate a hash value
> >   * @regs:    List of regions
> > diff --git a/lib/efi_loader/efi_variable.c
> b/lib/efi_loader/efi_variable.c
> > index 6c2dd82306..be34a2cadd 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void)
> >       return efi_secure_boot;
> >  }
> >
> > -#ifdef CONFIG_EFI_SECURE_BOOT
> > -static u8 pkcs7_hdr[] = {
> > -     /* SEQUENCE */
> > -     0x30, 0x82, 0x05, 0xc7,
> > -     /* OID: pkcs7-signedData */
> > -     0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> > -     /* Context Structured? */
> > -     0xa0, 0x82, 0x05, 0xb8,
> > -};
> > -
> > -/**
> > - * efi_variable_parse_signature - parse a signature in variable
> > - * @buf:     Pointer to variable's value
> > - * @buflen:  Length of @buf
> > - *
> > - * Parse a signature embedded in variable's value and instantiate
> > - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> > - * pkcs7's signedData, some header needed be prepended for correctly
> > - * parsing authentication data, particularly for variable's.
> > - *
> > - * Return:   Pointer to pkcs7_message structure on success, NULL on
> error
> > - */
> > -static struct pkcs7_message *efi_variable_parse_signature(const void
> *buf,
> > -                                                       size_t buflen)
> > -{
> > -     u8 *ebuf;
> > -     size_t ebuflen, len;
> > -     struct pkcs7_message *msg;
> > -
> > -     /*
> > -      * This is the best assumption to check if the binary is
> > -      * already in a form of pkcs7's signedData.
> > -      */
> > -     if (buflen > sizeof(pkcs7_hdr) &&
> > -         !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> > -             msg = pkcs7_parse_message(buf, buflen);
> > -             goto out;
> > -     }
> > -
> > -     /*
> > -      * Otherwise, we should add a dummy prefix sequence for pkcs7
> > -      * message parser to be able to process.
> > -      * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> > -      * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> > -      * TODO:
> > -      * The header should be composed in a more refined manner.
> > -      */
> > -     debug("Makeshift prefix added to authentication data\n");
> > -     ebuflen = sizeof(pkcs7_hdr) + buflen;
> > -     if (ebuflen <= 0x7f) {
> > -             debug("Data is too short\n");
> > -             return NULL;
> > -     }
> > -
> > -     ebuf = malloc(ebuflen);
> > -     if (!ebuf) {
> > -             debug("Out of memory\n");
> > -             return NULL;
> > -     }
> > -
> > -     memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> > -     memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> > -     len = ebuflen - 4;
> > -     ebuf[2] = (len >> 8) & 0xff;
> > -     ebuf[3] = len & 0xff;
> > -     len = ebuflen - 0x13;
> > -     ebuf[0x11] = (len >> 8) & 0xff;
> > -     ebuf[0x12] = len & 0xff;
> > -
> > -     msg = pkcs7_parse_message(ebuf, ebuflen);
> > -
> > -     free(ebuf);
> > -
> > -out:
> > -     if (IS_ERR(msg))
> > -             return NULL;
> > -
> > -     return msg;
> > -}
> > +#ifdef CONFIG_SECURE_BOOT
> >
> >  /**
> >   * efi_variable_authenticate - authenticate a variable
> > @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16
> *variable,
> >       /* variable's signature list */
> >       if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info))
> >               goto err;
> > -     var_sig = efi_variable_parse_signature(auth->auth_info.cert_data,
> > +     var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data,
> >                                              auth->auth_info.hdr.dwLength
> >                                                  -
> sizeof(auth->auth_info));
> >       if (IS_ERR(var_sig)) {
> > --
> > 2.17.1
> >
>

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Sughosh Ganu]

On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.akashi@linaro.org>
wrote:

> Sughosh,
>
> On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> > Add support for authenticating uefi capsules. Most of the signature
> > verification functionality is shared with the uefi secure boot
> > feature.
> >
> > The root certificate containing the public key used for the signature
> > verification is stored as an efi variable, similar to the variables
> > used for uefi secure boot. The root certificate is stored as an efi
> > signature list(esl) file -- this file contains the x509 certificate
> > which is the root certificate.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  include/efi_api.h              |  17 +++++
> >  include/efi_loader.h           |   8 ++-
> >  lib/efi_loader/Kconfig         |  16 +++++
> >  lib/efi_loader/efi_capsule.c   | 126 +++++++++++++++++++++++++++++++++
> >  lib/efi_loader/efi_signature.c |   4 +-
> >  5 files changed, 167 insertions(+), 4 deletions(-)
> >
>

<snip>

> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index ec2976ceba..245deabbed 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
> >       help
> >         Define storage device, say 1:1, for storing FIT image
> >
> > +config EFI_CAPSULE_AUTHENTICATE
> > +     bool "Update Capsule authentication"
> > +     depends on EFI_HAVE_CAPSULE_SUPPORT
> > +     depends on EFI_CAPSULE_ON_DISK
> > +     depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
>
> Do we need those two configurations?
>

Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change.


>
> > +     select SHA256
> > +     select RSA
> > +     select RSA_VERIFY
> > +     select RSA_VERIFY_WITH_PKEY
> > +     select X509_CERTIFICATE_PARSER
> > +     select PKCS7_MESSAGE_PARSER
> > +     default n
> > +     help
> > +       Select this option if you want to enable capsule
> > +       authentication
> > +
> >  config EFI_DEVICE_PATH_TO_TEXT
> >       bool "Device path to text protocol"
> >       default y
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 931d363edc..a265d36ff0 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -12,6 +12,10 @@
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <sort.h>
> > +#include "../lib/crypto/pkcs7_parser.h"
> > +
>
> As you might notice, the location was changed by
> my recent patch.
>

Will check and update.


>
> > +#include <crypto/pkcs7.h>
> > +#include <linux/err.h>
> >
> >  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> >  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> > @@ -245,6 +249,128 @@ out:
> >
> >       return ret;
> >  }
> > +
> > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > +
> > +const efi_guid_t efi_guid_capsule_root_cert_guid =
> > +     EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > +
> > +__weak u16 *efi_get_truststore_name(void)
> > +{
> > +     return L"CRT";
>
> This variable is not defined by UEFI specification, isn't it?
> How can this variable be protected?
>

This is not part of the uefi specification. The uefi specification does not
mandate any particular method for storing the root certificate to be used
for capsule authentication. The edk2 code gets the root certificate through
a pcd. I had tried using KEK variable for storing the root certificate, and
had also come up with an implementation. However, the addition/deletion and
update of the secure variables is very closely tied with the secure boot
feature and the secure boot state of the system, which is the reason why i
dropped that solution and used a separate variable, which can be defined by
the vendor to store the root certificate.


>
> > +}
> > +
> > +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> > +{
> > +
> > +     return &efi_guid_capsule_root_cert_guid;
> > +}
> > +
> > +/**
> > + * efi_capsule_authenticate() - Authenticate a uefi capsule
> > + *
> > + * @capsule:         Capsule file with the authentication
> > + *                   header
> > + * @capsule_size:    Size of the entire capsule
> > + * @image:           pointer to the image payload minus the
> > + *                   authentication header
> > + * @image_size:              size of the image payload
> > + *
> > + * Authenticate the capsule signature with the public key contained
> > + * in the root certificate stored as an efi environment variable
> > + *
> > + * Return: EFI_SUCCESS on successfull authentication or
> > + *      EFI_SECURITY_VIOLATION on authentication failure
> > + */
> > +efi_status_t efi_capsule_authenticate(const void *capsule,
> > +                                   efi_uintn_t capsule_size,
> > +                                   void **image, efi_uintn_t
> *image_size)
> > +{
> > +     uint64_t monotonic_count;
> > +     struct efi_signature_store *truststore;
> > +     struct pkcs7_message *capsule_sig;
> > +     struct efi_image_regions *regs;
> > +     struct efi_firmware_image_authentication *auth_hdr;
> > +     efi_status_t status;
> > +
> > +     status = EFI_SECURITY_VIOLATION;
> > +     capsule_sig = NULL;
> > +     truststore = NULL;
> > +     regs = NULL;
> > +
> > +     /* Sanity checks */
> > +     if (capsule == NULL || capsule_size == 0)
> > +             goto out;
> > +
> > +     auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > +     if (capsule_size < sizeof(*auth_hdr))
> > +             goto out;
> > +
> > +     if (auth_hdr->auth_info.hdr.dwLength <=
> > +         offsetof(struct win_certificate_uefi_guid, cert_data))
> > +             goto out;
> > +
> > +     if (guidcmp(&auth_hdr->auth_info.cert_type,
> &efi_guid_cert_type_pkcs7))
> > +             goto out;
> > +
> > +     *image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> > +             auth_hdr->auth_info.hdr.dwLength;
> > +     *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > +                                   sizeof(auth_hdr->monotonic_count);
> > +     memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> > +            sizeof(monotonic_count));
> > +
> > +     /* data to be digested */
> > +     regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2, 1);
> > +     if (!regs)
> > +             goto out;
> > +
> > +     regs->max = 2;
> > +     efi_image_region_add(regs, (uint8_t *)*image,
> > +                          (uint8_t *)*image + *image_size, 1);
> > +
> > +     efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> > +                          (uint8_t *)&monotonic_count +
> sizeof(monotonic_count),
> > +                          1);
>
> Is the order of regions to be calculated correct?
> It seems that 'monotonic_count' precedes 'image' itself
> in a capsule header.
>

Does that have any impact on the hash value computed. I did not see any
difference in the hash value computed due to the order in which the regions
are added. But that could be because of the way the hash value gets
computed at the time of building the capsule. I will check this out.


>
> > +
> > +     capsule_sig = efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> > +
> auth_hdr->auth_info.hdr.dwLength
> > +                                          -
> sizeof(auth_hdr->auth_info));
> > +     if (IS_ERR(capsule_sig)) {
>
> As Patrick reported, ex-efi_variable_parse_signature()
> returns NULL on error cases, this check should be "!capsule_sig."
>

Will change.

-sughosh

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 5/7/20 4:33 AM, Akashi Takahiro wrote:
> On Fri, May 01, 2020 at 11:33:42AM +0200, Heinrich Schuchardt wrote:
>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>
>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>
>>>     On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>     > Add support for the get_image_info and set_image routines, which are
>>>     > part of the efi firmware management protocol.
>>>     >
>>>     > The current implementation uses the set_image routine for updating the
>>>     > u-boot binary image for the qemu arm64 platform. This is supported
>>>     > using the capsule-on-disk feature of the uefi specification, wherein
>>>     > the firmware image to be updated is placed on the efi system partition
>>>     > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
>>>     > added for updating the u-boot image on platforms booting with arm
>>>     > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
>>>     > payload(bl33.bin).
>>>     >
>>>     > The feature can be enabled by the following config options
>>>     >
>>>     > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>     > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>     >
>>>     > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>     <mailto:sughosh.ganu@linaro.org>>
>>>
>>>     U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
>>>     RISC-V. Please, come up with an architecture independent solution.
>>>
>>>
>>> Please check the explanation that I gave in the other mail. If you check
>>> the patch series, the actual capsule authentication logic has been kept
>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
>>> intended for allowing platforms to define their firmware update
>>> routines. Edk2 also has platform specific implementation of the fmp
>>> protocol under the edk2-platforms directory.
>>>
>>> -sughosh
>>> ?
>>>
>>
>> My idea is that for most platforms it will be enough to have a common
>> FMP implementation that consumes a capsule
>>
>> * with one or more binaries
>
> Does this assumption apply to most platforms?
> If so ("one"),

Raspberry uses a file in the first partition which must be FAT to store
U-Boot. The file name of U-Boot is indicated in file config.txt to the
primary boot loader.

On all other devices I own U-Boot is installed by command 'dd' writing
to the SD-Card somewhere after the DOS partition table. (When using a
GUID partition table often you have to shorten it or relocated it to
after U-Boot.) Some of the devices could alternativley use eMMC for
U-Boot (e.g. Odroid C2).

For reference have a look at
doc/README.rockchip
https://a-delacruz.github.io/ubuntu/rpi3-setup-64bit-uboot.html

Best regards

Heinrich

>
>> * a media device path, a start address, and a truncation flag
>>   for each of the binaries
>
> my FIT-based patch[1] meets this assumption and there already
> are backend drivers for many media (but not for semihosting :)
> as dfu.
> (I see little reason to re-invent another set of backend drivers.)
>
> [1] https://lists.denx.de/pipermail/u-boot/2020-April/408767.html
>
>
>> The protocol implementation then will write the binaries to the device
>> paths:
>>
>> * to an SD-Card or eMMC exposing the Block IO protocol
>>   for most devices
>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>   (and truncate it if the truncation flag is set)
>>
>> If for some devices like a SPI flash we do not have a media device path
>> yet, then the only platform specific bit would be the block device
>> driver exposing the media device path.
>>
>> Same with a semi-hosted file: just add a driver exposing it as a media
>> path with an EFI_BLOCK_IO_PROTOCOL.
>>
>> For security reasons it may be advisable to make the device read-only
>> when reaching ExitBootServices() or even better before the first
>> execution of StartImage(). For this purpose we could use the Reset()
>> service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
>> service in the EFI_BLOCK_IO_PROTOCOL.
>>
>> Best regards
>>
>> Heinrich

[PATCH 8/8] qemu: arm64: Add documentation for capsule update

[Heinrich Schuchardt]

On 5/7/20 4:10 AM, Akashi Takahiro wrote:
> On Fri, May 01, 2020 at 11:17:27AM +0530, Sughosh Ganu wrote:
>> On Fri, 1 May 2020 at 00:57, Tom Rini <trini@konsulko.com> wrote:
>>
>>> On Fri, May 01, 2020 at 12:38:45AM +0530, Sughosh Ganu wrote:
>>>> On Fri, 1 May 2020 at 00:07, Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> wrote:
>>>>
>>>>> On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>>> Add documentation highlighting the steps for using the uefi capsule
>>>>>> update feature for updating the u-boot firmware image.
>>>>>>
>>>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>>>>
>>>>> UEFI capsule updates should be architecture independent. I would expect
>>>>> that the submitted code should work for x86, ARM, and RISC-V. Why does
>>>>> this documentation live under the ARM emulation tree?
>>>>>
>>>>
>>>> While the implementation of the core capsule update functionality is
>>> indeed
>>>> architecture agnostic, this series is for implementing the routines of
>>> the
>>>> firmware management protocol, which is very much platform specific -- the
>>>> routines to perform the actual firmware update would be very much tied to
>>>> the platform for which the firmware is being updated. So Takahiro's patch
>>>> series, which adds the core capsule update changes is architecture
>>>> independent, while this series is adding the routines for the firmware
>>>> management protocol, which would be very much platform specific.
>>>
>>> Since we're talking QEMU here, how much of this can be easily dropped in
>>> to QEMU x86_64 and QEMU RISC-V?  If not almost all of it, why?  Can it
>>> be reworked as such?
>>>
>>
>> I don't think it would be too difficult to extend it on other
>> architectures, provided there is some mechanism to access and overwrite the
>> u-boot binary file from the qemu target. It is currently being done using
>> the semihosting interface for the arm architecture. I am not aware if there
>> is an interface like semihosting for accessing the u-boot binary on the
>> other architectures that you mentioned. Will check on this.
>
> Obviously, another choice would be my FIT-based FMP[1]
> as it uses update_tftp(), more specifically dfu_tftp_write(),
> internally.
>
> [1] https://lists.denx.de/pipermail/u-boot/2020-April/408767.html
>
> Thanks,
> -Takahiro Akashi

Please, try to align. We should avoid alternative implementations were
not needed. We could also have a chat on Zoom together.

Best regards

Heinrich

>
>
>> -sughosh

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Akashi Takahiro]

On Thu, May 07, 2020 at 10:47:47PM +0200, Heinrich Schuchardt wrote:
> On 5/7/20 4:33 AM, Akashi Takahiro wrote:
> > On Fri, May 01, 2020 at 11:33:42AM +0200, Heinrich Schuchardt wrote:
> >> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
> >>>
> >>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
> >>> <mailto:xypron.glpk@gmx.de>> wrote:
> >>>
> >>>     On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> >>>     > Add support for the get_image_info and set_image routines, which are
> >>>     > part of the efi firmware management protocol.
> >>>     >
> >>>     > The current implementation uses the set_image routine for updating the
> >>>     > u-boot binary image for the qemu arm64 platform. This is supported
> >>>     > using the capsule-on-disk feature of the uefi specification, wherein
> >>>     > the firmware image to be updated is placed on the efi system partition
> >>>     > as a efi capsule under EFI/UpdateCapsule/ directory. Support has been
> >>>     > added for updating the u-boot image on platforms booting with arm
> >>>     > trusted firmware(tf-a), where the u-boot image gets booted as the BL33
> >>>     > payload(bl33.bin).
> >>>     >
> >>>     > The feature can be enabled by the following config options
> >>>     >
> >>>     > CONFIG_EFI_CAPSULE_ON_DISK=y
> >>>     > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> >>>     >
> >>>     > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
> >>>     <mailto:sughosh.ganu@linaro.org>>
> >>>
> >>>     U-Boot's UEFI subsystem should work in the same way for x86, ARM, and
> >>>     RISC-V. Please, come up with an architecture independent solution.
> >>>
> >>>
> >>> Please check the explanation that I gave in the other mail. If you check
> >>> the patch series, the actual capsule authentication logic has been kept
> >>> architecture agnostic, in efi_capsule.c. The fmp protocol is very much
> >>> intended for allowing platforms to define their firmware update
> >>> routines. Edk2 also has platform specific implementation of the fmp
> >>> protocol under the edk2-platforms directory.
> >>>
> >>> -sughosh
> >>> ?
> >>>
> >>
> >> My idea is that for most platforms it will be enough to have a common
> >> FMP implementation that consumes a capsule
> >>
> >> * with one or more binaries
> >
> > Does this assumption apply to most platforms?
> > If so ("one"),
> 
> Raspberry uses a file in the first partition which must be FAT to store
> U-Boot. The file name of U-Boot is indicated in file config.txt to the
> primary boot loader.
> 
> On all other devices I own U-Boot is installed by command 'dd' writing
> to the SD-Card somewhere after the DOS partition table. (When using a
> GUID partition table often you have to shorten it or relocated it to
> after U-Boot.) Some of the devices could alternativley use eMMC for
> U-Boot (e.g. Odroid C2).

"Firmware" doesn't always mean U-Boot binary.
What I had in my mind is that it can be
  - storage for U-Boot environment variables 
  - firmware for other peripherals, or even
  - kernel(/initfs/dtb)
(Remember that FIT format potentially allows for holding them.)
So I believe that it's totally up to systems.

-Takahiro Akashi

> For reference have a look at
> doc/README.rockchip
> https://a-delacruz.github.io/ubuntu/rpi3-setup-64bit-uboot.html
> 
> Best regards
> 
> Heinrich
> 
> >
> >> * a media device path, a start address, and a truncation flag
> >>   for each of the binaries
> >
> > my FIT-based patch[1] meets this assumption and there already
> > are backend drivers for many media (but not for semihosting :)
> > as dfu.
> > (I see little reason to re-invent another set of backend drivers.)
> >
> > [1] https://lists.denx.de/pipermail/u-boot/2020-April/408767.html
> >
> >
> >> The protocol implementation then will write the binaries to the device
> >> paths:
> >>
> >> * to an SD-Card or eMMC exposing the Block IO protocol
> >>   for most devices
> >> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
> >>   (and truncate it if the truncation flag is set)
> >>
> >> If for some devices like a SPI flash we do not have a media device path
> >> yet, then the only platform specific bit would be the block device
> >> driver exposing the media device path.
> >>
> >> Same with a semi-hosted file: just add a driver exposing it as a media
> >> path with an EFI_BLOCK_IO_PROTOCOL.
> >>
> >> For security reasons it may be advisable to make the device read-only
> >> when reaching ExitBootServices() or even better before the first
> >> execution of StartImage(). For this purpose we could use the Reset()
> >> service of the EFI_BLOCK_IO_PROTOCOL or provide a U-Boot specific
> >> service in the EFI_BLOCK_IO_PROTOCOL.
> >>
> >> Best regards
> >>
> >> Heinrich
> 

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Akashi Takahiro]

On Thu, May 07, 2020 at 05:20:35PM +0530, Sughosh Ganu wrote:
> On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > Sughosh,
> >
> > On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> > > Add support for authenticating uefi capsules. Most of the signature
> > > verification functionality is shared with the uefi secure boot
> > > feature.
> > >
> > > The root certificate containing the public key used for the signature
> > > verification is stored as an efi variable, similar to the variables
> > > used for uefi secure boot. The root certificate is stored as an efi
> > > signature list(esl) file -- this file contains the x509 certificate
> > > which is the root certificate.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >  include/efi_api.h              |  17 +++++
> > >  include/efi_loader.h           |   8 ++-
> > >  lib/efi_loader/Kconfig         |  16 +++++
> > >  lib/efi_loader/efi_capsule.c   | 126 +++++++++++++++++++++++++++++++++
> > >  lib/efi_loader/efi_signature.c |   4 +-
> > >  5 files changed, 167 insertions(+), 4 deletions(-)
> > >
> >
> 
> <snip>
> 
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > index ec2976ceba..245deabbed 100644
> > > --- a/lib/efi_loader/Kconfig
> > > +++ b/lib/efi_loader/Kconfig
> > > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
> > >       help
> > >         Define storage device, say 1:1, for storing FIT image
> > >
> > > +config EFI_CAPSULE_AUTHENTICATE
> > > +     bool "Update Capsule authentication"
> > > +     depends on EFI_HAVE_CAPSULE_SUPPORT
> > > +     depends on EFI_CAPSULE_ON_DISK
> > > +     depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> >
> > Do we need those two configurations?
> >
> 
> Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change.

Actually I don't think we need EFI_CAPSULE_ON_DISK neither.

> 
> >
> > > +     select SHA256
> > > +     select RSA
> > > +     select RSA_VERIFY
> > > +     select RSA_VERIFY_WITH_PKEY
> > > +     select X509_CERTIFICATE_PARSER
> > > +     select PKCS7_MESSAGE_PARSER
> > > +     default n
> > > +     help
> > > +       Select this option if you want to enable capsule
> > > +       authentication
> > > +
> > >  config EFI_DEVICE_PATH_TO_TEXT
> > >       bool "Device path to text protocol"
> > >       default y
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 931d363edc..a265d36ff0 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -12,6 +12,10 @@
> > >  #include <malloc.h>
> > >  #include <mapmem.h>
> > >  #include <sort.h>
> > > +#include "../lib/crypto/pkcs7_parser.h"
> > > +
> >
> > As you might notice, the location was changed by
> > my recent patch.
> >
> 
> Will check and update.
> 
> 
> >
> > > +#include <crypto/pkcs7.h>
> > > +#include <linux/err.h>
> > >
> > >  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> > >  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> > > @@ -245,6 +249,128 @@ out:
> > >
> > >       return ret;
> > >  }
> > > +
> > > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > > +
> > > +const efi_guid_t efi_guid_capsule_root_cert_guid =
> > > +     EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > > +
> > > +__weak u16 *efi_get_truststore_name(void)
> > > +{
> > > +     return L"CRT";
> >
> > This variable is not defined by UEFI specification, isn't it?
> > How can this variable be protected?
> >
> 
> This is not part of the uefi specification. The uefi specification does not
> mandate any particular method for storing the root certificate to be used
> for capsule authentication. The edk2 code gets the root certificate through
> a pcd. I had tried using KEK variable for storing the root certificate, and
> had also come up with an implementation. However, the addition/deletion and
> update of the secure variables is very closely tied with the secure boot
> feature and the secure boot state of the system, which is the reason why i
> dropped that solution and used a separate variable, which can be defined by
> the vendor to store the root certificate.

My concern is that, without any protection, the value of this variable
can be modified by a mal-user.
(One simple solution would be to set this variable read-only.)

> 
> >
> > > +}
> > > +
> > > +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> > > +{
> > > +
> > > +     return &efi_guid_capsule_root_cert_guid;
> > > +}
> > > +
> > > +/**
> > > + * efi_capsule_authenticate() - Authenticate a uefi capsule
> > > + *
> > > + * @capsule:         Capsule file with the authentication
> > > + *                   header
> > > + * @capsule_size:    Size of the entire capsule
> > > + * @image:           pointer to the image payload minus the
> > > + *                   authentication header
> > > + * @image_size:              size of the image payload
> > > + *
> > > + * Authenticate the capsule signature with the public key contained
> > > + * in the root certificate stored as an efi environment variable
> > > + *
> > > + * Return: EFI_SUCCESS on successfull authentication or
> > > + *      EFI_SECURITY_VIOLATION on authentication failure
> > > + */
> > > +efi_status_t efi_capsule_authenticate(const void *capsule,
> > > +                                   efi_uintn_t capsule_size,
> > > +                                   void **image, efi_uintn_t
> > *image_size)
> > > +{
> > > +     uint64_t monotonic_count;
> > > +     struct efi_signature_store *truststore;
> > > +     struct pkcs7_message *capsule_sig;
> > > +     struct efi_image_regions *regs;
> > > +     struct efi_firmware_image_authentication *auth_hdr;
> > > +     efi_status_t status;
> > > +
> > > +     status = EFI_SECURITY_VIOLATION;
> > > +     capsule_sig = NULL;
> > > +     truststore = NULL;
> > > +     regs = NULL;
> > > +
> > > +     /* Sanity checks */
> > > +     if (capsule == NULL || capsule_size == 0)
> > > +             goto out;
> > > +
> > > +     auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > > +     if (capsule_size < sizeof(*auth_hdr))
> > > +             goto out;
> > > +
> > > +     if (auth_hdr->auth_info.hdr.dwLength <=
> > > +         offsetof(struct win_certificate_uefi_guid, cert_data))
> > > +             goto out;
> > > +
> > > +     if (guidcmp(&auth_hdr->auth_info.cert_type,
> > &efi_guid_cert_type_pkcs7))
> > > +             goto out;
> > > +
> > > +     *image = (uint8_t *)capsule + sizeof(auth_hdr->monotonic_count) +
> > > +             auth_hdr->auth_info.hdr.dwLength;
> > > +     *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > > +                                   sizeof(auth_hdr->monotonic_count);
> > > +     memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> > > +            sizeof(monotonic_count));
> > > +
> > > +     /* data to be digested */
> > > +     regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2, 1);
> > > +     if (!regs)
> > > +             goto out;
> > > +
> > > +     regs->max = 2;
> > > +     efi_image_region_add(regs, (uint8_t *)*image,
> > > +                          (uint8_t *)*image + *image_size, 1);
> > > +
> > > +     efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> > > +                          (uint8_t *)&monotonic_count +
> > sizeof(monotonic_count),
> > > +                          1);
> >
> > Is the order of regions to be calculated correct?
> > It seems that 'monotonic_count' precedes 'image' itself
> > in a capsule header.
> >
> 
> Does that have any impact on the hash value computed.

Not 100% sure, but if it doesn't, it cannot guarantee uniqueness
of digest values.

Thanks,
-Takahiro Akashi

> I did not see any
> difference in the hash value computed due to the order in which the regions
> are added. But that could be because of the way the hash value gets
> computed at the time of building the capsule. I will check this out.
> 
> 
> >
> > > +
> > > +     capsule_sig = efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> > > +
> > auth_hdr->auth_info.hdr.dwLength
> > > +                                          -
> > sizeof(auth_hdr->auth_info));
> > > +     if (IS_ERR(capsule_sig)) {
> >
> > As Patrick reported, ex-efi_variable_parse_signature()
> > returns NULL on error cases, this check should be "!capsule_sig."
> >
> 
> Will change.
> 
> -sughosh

[PATCH 5/8] efi_loader: Make the pkcs7 header parsing function an extern

[Akashi Takahiro]

On Thu, May 07, 2020 at 04:48:30PM +0530, Sughosh Ganu wrote:
> On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote:
> > > The pkcs7 header parsing functionality is pretty generic, and can be
> > > used by other features like capsule authentication. Make the function
> > > as an extern, also changing it's name to efi_parse_pkcs7_header.
> >
> > The patch itself is fine to me, but is "pkcs7 header" a common term?
> >
> 
> I haven't come across it in any other code base. I used it since in the
> concept of a capsule, the signature is prepended to the capsule payload. If
> you can think of a better name, please suggest so. I will change it in the
> next version.

Simply, efi_parse_pkcs7_signature()?

In addition, please update the function description, which still
mentions "variable."

-Takahiro Akashi

> -sughosh
> 
> 
> >
> > -Takahiro Akashi
> >
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > >  include/efi_loader.h           |  2 +
> > >  lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++
> > >  lib/efi_loader/efi_variable.c  | 82 +---------------------------------
> > >  3 files changed, 82 insertions(+), 80 deletions(-)
> > >
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index b7638d5825..8d923451ce 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void);
> > >
> > >  bool efi_image_parse(void *efi, size_t len, struct efi_image_regions
> > **regp,
> > >                    WIN_CERTIFICATE **auth, size_t *auth_len);
> > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t
> > buflen);
> > > +
> > >  #endif /* CONFIG_EFI_SECURE_BOOT */
> > >
> > >  #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT
> > > diff --git a/lib/efi_loader/efi_signature.c
> > b/lib/efi_loader/efi_signature.c
> > > index bf6f39aab2..9897f5418e 100644
> > > --- a/lib/efi_loader/efi_signature.c
> > > +++ b/lib/efi_loader/efi_signature.c
> > > @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 =
> > EFI_CERT_X509_SHA256_GUID;
> > >
> > >  #ifdef CONFIG_EFI_SECURE_BOOT
> > >
> > > +static u8 pkcs7_hdr[] = {
> > > +     /* SEQUENCE */
> > > +     0x30, 0x82, 0x05, 0xc7,
> > > +     /* OID: pkcs7-signedData */
> > > +     0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> > > +     /* Context Structured? */
> > > +     0xa0, 0x82, 0x05, 0xb8,
> > > +};
> > > +
> > > +/**
> > > + * efi_parse_pkcs7_header - parse a signature in variable
> > > + * @buf:     Pointer to the payload's value
> > > + * @buflen:  Length of @buf
> > > + *
> > > + * Parse a signature embedded in variable's value and instantiate
> > > + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> > > + * pkcs7's signedData, some header needed be prepended for correctly
> > > + * parsing authentication data, particularly for variable's.
> > > + *
> > > + * Return:   Pointer to pkcs7_message structure on success, NULL on
> > error
> > > + */
> > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t
> > buflen)
> > > +{
> > > +     u8 *ebuf;
> > > +     size_t ebuflen, len;
> > > +     struct pkcs7_message *msg;
> > > +
> > > +     /*
> > > +      * This is the best assumption to check if the binary is
> > > +      * already in a form of pkcs7's signedData.
> > > +      */
> > > +     if (buflen > sizeof(pkcs7_hdr) &&
> > > +         !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> > > +             msg = pkcs7_parse_message(buf, buflen);
> > > +             goto out;
> > > +     }
> > > +
> > > +     /*
> > > +      * Otherwise, we should add a dummy prefix sequence for pkcs7
> > > +      * message parser to be able to process.
> > > +      * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> > > +      * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> > > +      * TODO:
> > > +      * The header should be composed in a more refined manner.
> > > +      */
> > > +     debug("Makeshift prefix added to authentication data\n");
> > > +     ebuflen = sizeof(pkcs7_hdr) + buflen;
> > > +     if (ebuflen <= 0x7f) {
> > > +             debug("Data is too short\n");
> > > +             return NULL;
> > > +     }
> > > +
> > > +     ebuf = malloc(ebuflen);
> > > +     if (!ebuf) {
> > > +             debug("Out of memory\n");
> > > +             return NULL;
> > > +     }
> > > +
> > > +     memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> > > +     memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> > > +     len = ebuflen - 4;
> > > +     ebuf[2] = (len >> 8) & 0xff;
> > > +     ebuf[3] = len & 0xff;
> > > +     len = ebuflen - 0x13;
> > > +     ebuf[0x11] = (len >> 8) & 0xff;
> > > +     ebuf[0x12] = len & 0xff;
> > > +
> > > +     msg = pkcs7_parse_message(ebuf, ebuflen);
> > > +
> > > +     free(ebuf);
> > > +
> > > +out:
> > > +     if (IS_ERR(msg))
> > > +             return NULL;
> > > +
> > > +     return msg;
> > > +}
> > > +
> > >  /**
> > >   * efi_hash_regions - calculate a hash value
> > >   * @regs:    List of regions
> > > diff --git a/lib/efi_loader/efi_variable.c
> > b/lib/efi_loader/efi_variable.c
> > > index 6c2dd82306..be34a2cadd 100644
> > > --- a/lib/efi_loader/efi_variable.c
> > > +++ b/lib/efi_loader/efi_variable.c
> > > @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void)
> > >       return efi_secure_boot;
> > >  }
> > >
> > > -#ifdef CONFIG_EFI_SECURE_BOOT
> > > -static u8 pkcs7_hdr[] = {
> > > -     /* SEQUENCE */
> > > -     0x30, 0x82, 0x05, 0xc7,
> > > -     /* OID: pkcs7-signedData */
> > > -     0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02,
> > > -     /* Context Structured? */
> > > -     0xa0, 0x82, 0x05, 0xb8,
> > > -};
> > > -
> > > -/**
> > > - * efi_variable_parse_signature - parse a signature in variable
> > > - * @buf:     Pointer to variable's value
> > > - * @buflen:  Length of @buf
> > > - *
> > > - * Parse a signature embedded in variable's value and instantiate
> > > - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only
> > > - * pkcs7's signedData, some header needed be prepended for correctly
> > > - * parsing authentication data, particularly for variable's.
> > > - *
> > > - * Return:   Pointer to pkcs7_message structure on success, NULL on
> > error
> > > - */
> > > -static struct pkcs7_message *efi_variable_parse_signature(const void
> > *buf,
> > > -                                                       size_t buflen)
> > > -{
> > > -     u8 *ebuf;
> > > -     size_t ebuflen, len;
> > > -     struct pkcs7_message *msg;
> > > -
> > > -     /*
> > > -      * This is the best assumption to check if the binary is
> > > -      * already in a form of pkcs7's signedData.
> > > -      */
> > > -     if (buflen > sizeof(pkcs7_hdr) &&
> > > -         !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) {
> > > -             msg = pkcs7_parse_message(buf, buflen);
> > > -             goto out;
> > > -     }
> > > -
> > > -     /*
> > > -      * Otherwise, we should add a dummy prefix sequence for pkcs7
> > > -      * message parser to be able to process.
> > > -      * NOTE: EDK2 also uses similar hack in WrapPkcs7Data()
> > > -      * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c
> > > -      * TODO:
> > > -      * The header should be composed in a more refined manner.
> > > -      */
> > > -     debug("Makeshift prefix added to authentication data\n");
> > > -     ebuflen = sizeof(pkcs7_hdr) + buflen;
> > > -     if (ebuflen <= 0x7f) {
> > > -             debug("Data is too short\n");
> > > -             return NULL;
> > > -     }
> > > -
> > > -     ebuf = malloc(ebuflen);
> > > -     if (!ebuf) {
> > > -             debug("Out of memory\n");
> > > -             return NULL;
> > > -     }
> > > -
> > > -     memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr));
> > > -     memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen);
> > > -     len = ebuflen - 4;
> > > -     ebuf[2] = (len >> 8) & 0xff;
> > > -     ebuf[3] = len & 0xff;
> > > -     len = ebuflen - 0x13;
> > > -     ebuf[0x11] = (len >> 8) & 0xff;
> > > -     ebuf[0x12] = len & 0xff;
> > > -
> > > -     msg = pkcs7_parse_message(ebuf, ebuflen);
> > > -
> > > -     free(ebuf);
> > > -
> > > -out:
> > > -     if (IS_ERR(msg))
> > > -             return NULL;
> > > -
> > > -     return msg;
> > > -}
> > > +#ifdef CONFIG_SECURE_BOOT
> > >
> > >  /**
> > >   * efi_variable_authenticate - authenticate a variable
> > > @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16
> > *variable,
> > >       /* variable's signature list */
> > >       if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info))
> > >               goto err;
> > > -     var_sig = efi_variable_parse_signature(auth->auth_info.cert_data,
> > > +     var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data,
> > >                                              auth->auth_info.hdr.dwLength
> > >                                                  -
> > sizeof(auth->auth_info));
> > >       if (IS_ERR(var_sig)) {
> > > --
> > > 2.17.1
> > >
> >

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Heinrich Schuchardt]

On 5/6/20 5:04 PM, Grant Likely wrote:
>
>
> On 05/05/2020 18:57, Heinrich Schuchardt wrote:
>> On 05.05.20 19:23, Grant Likely wrote:
>>>
>>>
>>> On 05/05/2020 18:04, Heinrich Schuchardt wrote:
>>>> On 05.05.20 13:15, Grant Likely wrote:
>>>>>
>>>>>
>>>>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>>>>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>>>>>
>>>>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>>>>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>>>>>
>>>>>>> ?????? On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>>>> ?????? > Add support for the get_image_info and set_image routines,
>>>>>>> which are
>>>>>>> ?????? > part of the efi firmware management protocol.
>>>>>>> ?????? >
>>>>>>> ?????? > The current implementation uses the set_image routine for
>>>>>>> updating the
>>>>>>> ?????? > u-boot binary image for the qemu arm64 platform. This is
>>>>>>> supported
>>>>>>> ?????? > using the capsule-on-disk feature of the uefi
>>>>>>> specification,
>>>>>>> wherein
>>>>>>> ?????? > the firmware image to be updated is placed on the efi
>>>>>>> system
>>>>>>> partition
>>>>>>> ?????? > as a efi capsule under EFI/UpdateCapsule/ directory.
>>>>>>> Support
>>>>>>> has been
>>>>>>> ?????? > added for updating the u-boot image on platforms booting
>>>>>>> with arm
>>>>>>> ?????? > trusted firmware(tf-a), where the u-boot image gets
>>>>>>> booted as
>>>>>>> the BL33
>>>>>>> ?????? > payload(bl33.bin).
>>>>>>> ?????? >
>>>>>>> ?????? > The feature can be enabled by the following config options
>>>>>>> ?????? >
>>>>>>> ?????? > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>>>> ?????? > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>>>>> ?????? >
>>>>>>> ?????? > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>>>>> ?????? <mailto:sughosh.ganu@linaro.org>>
>>>>>>>
>>>>>>> ?????? U-Boot's UEFI subsystem should work in the same way for x86,
>>>>>>> ARM, and
>>>>>>> ?????? RISC-V. Please, come up with an architecture independent
>>>>>>> solution.
>>>>>>>
>>>>>>>
>>>>>>> Please check the explanation that I gave in the other mail. If you
>>>>>>> check
>>>>>>> the patch series, the actual capsule authentication logic has been
>>>>>>> kept
>>>>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very
>>>>>>> much
>>>>>>> intended for allowing platforms to define their firmware update
>>>>>>> routines. Edk2 also has platform specific implementation of the fmp
>>>>>>> protocol under the edk2-platforms directory.
>>>>>>>
>>>>>>> -sughosh
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> My idea is that for most platforms it will be enough to have a common
>>>>>> FMP implementation that consumes a capsule
>>>>>>
>>>>>> * with one or more binaries
>>>>>> * a media device path, a start address, and a truncation flag
>>>>>> ???? for each of the binaries
>>>>>>
>>>>>> The protocol implementation then will write the binaries to the
>>>>>> device
>>>>>> paths:
>>>>>>
>>>>>> * to an SD-Card or eMMC exposing the Block IO protocol
>>>>>> ???? for most devices
>>>>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>>>>> ???? (and truncate it if the truncation flag is set)
>>>>>
>>>>> Does U-Boot have a common device path protocol that can be backed by
>>>>> either a block device or a file on a filesystem? I didn't think it
>>>>> did.
>>>>
>>>> A block device, a partition, and a file all can be described by an UEFI
>>>> media device path.
>>>
>>> Sure, from a UEFI media path, but does the underlying U-Boot
>>> implementation have that abstraction?
>>
>>
>> With a file media path we can find the partition device path on which
>> the Simple file protocol is already installed by U-Boot.
>>
>>>
>>>>> In the mean time, there are at least three backends that the FMP is
>>>>> going to have to deal with; the two you list above (block device &
>>>>> file)
>>>>> and SMC backed when updating firmware is managed by the secure world.
>>>>> This first implementation only handles the file-backed use case.
>>>>> Can we
>>>>> start with that limitation and refactor when the block-device and SMC
>>>>> use cases are added in? I would hate to see this functionality held up
>>>>> on having to refactor other functionality in U-Boot.
>>>>
>>>> I would prefer one single FMP driver for all SMC use cases. Everything
>>>> device specific should be handled in the secure world.
>>>
>>> Not all platforms will be able to put firmware update into the secure
>>> world. For instance, Not many Arm v7 platforms have trustzone accessible
>>> to open source developers. On non-secure platforms (e.g., anything that
>>> loads firmware from a regular filesystem on SD or eMMC) it doesn't make
>>> much sense to loop out to secure firmware when U-Boot owns the
>>> filesystem and the secure world would then need to coordinate with
>>> U-Boot to commit the writes.
>>>
>>> All of the code can certainly be in the same location, but I do think
>>> there are three distinct generic backends for firmware updates:
>>> - normal-world file-backed (using filesystem)
>>> - normal-world block-backed (offsets from start of device)
>>
>> These two could be in one instance of the FMP protocol.
>>
>>> - secure device backed (needs to go into secure world for unpacking and
>>> processing regardless)
>>>
>>> There doesn't need to be platform-specific code in any of those back
>>> ends, but they do have different behaviour.
>>>
>>>>
>>>> Is there already a protocol defined for the communication of capsule
>>>> updates between the firmware and the secure monitor, e.g. in EDK2?
>>>
>>> Nothing defined yet (see below)
>>>
>>>> Would we simply use the UpdateCapsule() call parameters and pass them
>>>> via an SMC call?
>>>
>>> If secure world is handling the update? Yes, I think a thin
>>> UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
>>> runtime becomes feasible on U-Boot. There are a couple of people inside
>>> Arm looking at possible interfaces. In this situation there is very
>>> little done in normal-world at all.
>>>
>>> [...]
>>>
>>>>
>>>> According to my call with Sughosh the whole semihosting thing is about
>>>> providing a testing possibility not about any real use case.
>>>
>>> Yes, it's for testing, but it is particularly valuable testing because
>>> it allows the host filesystem to be exposed into QEMU. Exposing
>>> semihosting as a generic fstype_info in U-Boot is generically useful
>>> apart from this entire discussion!? :-)
>>>
>>> If we rework the semihosting code to be just another FS driver, then I
>>> think it is just an implementation detail on the file-backed firmware
>>> update path.
>>
>> In this case it could be completely separate from this patch series.
>>
>> Where do you see the benefit of semi-hosting compared to mounting an
>> image?
>
> Both are important. We need test benches for both file and block device
> update scenarios.
>
> g.
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> On QEMU you can easily mount an image as block device using parameters
>>>> of the qemu-system-* command. On the mounted image/block-device you can
>>>> test both file and block device based firmware updates. On the sandbox
>>>> you could use the 'host bind' command for mounting an image.
>>>
>>> Similar to the above, this would be an implementation detail on the
>>> block-device backed firmware update path. We need both.
>>>
>>> g.
>>> IMPORTANT NOTICE: The contents of this email and any attachments are
>>> confidential and may also be privileged. If you are not the intended
>>> recipient, please notify the sender immediately and do not disclose the
>>> contents to any other person, use it for any purpose, or store or copy
>>> the information in any medium. Thank you.
>>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.

[08.05  23:22] <apalos> who will provide the file path protocol and
potential offsets?
[09.05  12:02] <xypron> The media device path and the offsets have to be
provided by the one who builds the capsule and the must be integral part
of the capsule.

Best regards

Heinrich

[PATCH 5/8] efi_loader: Make the pkcs7 header parsing function an extern

[Sughosh Ganu]

On Fri, 8 May 2020 at 06:21, Akashi Takahiro <takahiro.akashi@linaro.org>
wrote:

> On Thu, May 07, 2020 at 04:48:30PM +0530, Sughosh Ganu wrote:
> > On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi@linaro.org
> >
> > wrote:
> >
> > > On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote:
> > > > The pkcs7 header parsing functionality is pretty generic, and can be
> > > > used by other features like capsule authentication. Make the function
> > > > as an extern, also changing it's name to efi_parse_pkcs7_header.
> > >
> > > The patch itself is fine to me, but is "pkcs7 header" a common term?
> > >
> >
> > I haven't come across it in any other code base. I used it since in the
> > concept of a capsule, the signature is prepended to the capsule payload.
> If
> > you can think of a better name, please suggest so. I will change it in
> the
> > next version.
>
> Simply, efi_parse_pkcs7_signature()?
>
> In addition, please update the function description, which still
> mentions "variable."
>

Ok. Will change.

-sughosh

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Sughosh Ganu]

On Fri, 8 May 2020 at 06:12, Akashi Takahiro <takahiro.akashi@linaro.org>
wrote:

> On Thu, May 07, 2020 at 05:20:35PM +0530, Sughosh Ganu wrote:
> > On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.akashi@linaro.org
> >
> > wrote:
> >
> > > Sughosh,
> > >
> > > On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> > > > Add support for authenticating uefi capsules. Most of the signature
> > > > verification functionality is shared with the uefi secure boot
> > > > feature.
> > > >
> > > > The root certificate containing the public key used for the signature
> > > > verification is stored as an efi variable, similar to the variables
> > > > used for uefi secure boot. The root certificate is stored as an efi
> > > > signature list(esl) file -- this file contains the x509 certificate
> > > > which is the root certificate.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > >  include/efi_api.h              |  17 +++++
> > > >  include/efi_loader.h           |   8 ++-
> > > >  lib/efi_loader/Kconfig         |  16 +++++
> > > >  lib/efi_loader/efi_capsule.c   | 126
> +++++++++++++++++++++++++++++++++
> > > >  lib/efi_loader/efi_signature.c |   4 +-
> > > >  5 files changed, 167 insertions(+), 4 deletions(-)
> > > >
> > >
> >
> > <snip>
> >
> > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > index ec2976ceba..245deabbed 100644
> > > > --- a/lib/efi_loader/Kconfig
> > > > +++ b/lib/efi_loader/Kconfig
> > > > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
> > > >       help
> > > >         Define storage device, say 1:1, for storing FIT image
> > > >
> > > > +config EFI_CAPSULE_AUTHENTICATE
> > > > +     bool "Update Capsule authentication"
> > > > +     depends on EFI_HAVE_CAPSULE_SUPPORT
> > > > +     depends on EFI_CAPSULE_ON_DISK
> > > > +     depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > >
> > > Do we need those two configurations?
> > >
> >
> > Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change.
>
> Actually I don't think we need EFI_CAPSULE_ON_DISK neither.
>

We at least need EFI_HAVE_CAPSULE_SUPPORT, isn't it.


>
> >
> > >
> > > > +     select SHA256
> > > > +     select RSA
> > > > +     select RSA_VERIFY
> > > > +     select RSA_VERIFY_WITH_PKEY
> > > > +     select X509_CERTIFICATE_PARSER
> > > > +     select PKCS7_MESSAGE_PARSER
> > > > +     default n
> > > > +     help
> > > > +       Select this option if you want to enable capsule
> > > > +       authentication
> > > > +
> > > >  config EFI_DEVICE_PATH_TO_TEXT
> > > >       bool "Device path to text protocol"
> > > >       default y
> > > > diff --git a/lib/efi_loader/efi_capsule.c
> b/lib/efi_loader/efi_capsule.c
> > > > index 931d363edc..a265d36ff0 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -12,6 +12,10 @@
> > > >  #include <malloc.h>
> > > >  #include <mapmem.h>
> > > >  #include <sort.h>
> > > > +#include "../lib/crypto/pkcs7_parser.h"
> > > > +
> > >
> > > As you might notice, the location was changed by
> > > my recent patch.
> > >
> >
> > Will check and update.
> >
> >
> > >
> > > > +#include <crypto/pkcs7.h>
> > > > +#include <linux/err.h>
> > > >
> > > >  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> > > >  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> > > > @@ -245,6 +249,128 @@ out:
> > > >
> > > >       return ret;
> > > >  }
> > > > +
> > > > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > > > +
> > > > +const efi_guid_t efi_guid_capsule_root_cert_guid =
> > > > +     EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > > > +
> > > > +__weak u16 *efi_get_truststore_name(void)
> > > > +{
> > > > +     return L"CRT";
> > >
> > > This variable is not defined by UEFI specification, isn't it?
> > > How can this variable be protected?
> > >
> >
> > This is not part of the uefi specification. The uefi specification does
> not
> > mandate any particular method for storing the root certificate to be used
> > for capsule authentication. The edk2 code gets the root certificate
> through
> > a pcd. I had tried using KEK variable for storing the root certificate,
> and
> > had also come up with an implementation. However, the addition/deletion
> and
> > update of the secure variables is very closely tied with the secure boot
> > feature and the secure boot state of the system, which is the reason why
> i
> > dropped that solution and used a separate variable, which can be defined
> by
> > the vendor to store the root certificate.
>
> My concern is that, without any protection, the value of this variable
> can be modified by a mal-user.
> (One simple solution would be to set this variable read-only.)
>

Yes, that is one solution. This will also be taken care of in a scenario
where the platform is booted in a trusted chain, and the env is also part
of the image which gets verified by the previous stage(e.g BL2) before
jumping to the u-boot image. If there is any change in the u-boot image,
that would result in a boot failure -- but that would need the env to be
part of the u-boot image which gets verified. I will explore making this
variable read-only.


> >
> > >
> > > > +}
> > > > +
> > > > +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> > > > +{
> > > > +
> > > > +     return &efi_guid_capsule_root_cert_guid;
> > > > +}
> > > > +
> > > > +/**
> > > > + * efi_capsule_authenticate() - Authenticate a uefi capsule
> > > > + *
> > > > + * @capsule:         Capsule file with the authentication
> > > > + *                   header
> > > > + * @capsule_size:    Size of the entire capsule
> > > > + * @image:           pointer to the image payload minus the
> > > > + *                   authentication header
> > > > + * @image_size:              size of the image payload
> > > > + *
> > > > + * Authenticate the capsule signature with the public key contained
> > > > + * in the root certificate stored as an efi environment variable
> > > > + *
> > > > + * Return: EFI_SUCCESS on successfull authentication or
> > > > + *      EFI_SECURITY_VIOLATION on authentication failure
> > > > + */
> > > > +efi_status_t efi_capsule_authenticate(const void *capsule,
> > > > +                                   efi_uintn_t capsule_size,
> > > > +                                   void **image, efi_uintn_t
> > > *image_size)
> > > > +{
> > > > +     uint64_t monotonic_count;
> > > > +     struct efi_signature_store *truststore;
> > > > +     struct pkcs7_message *capsule_sig;
> > > > +     struct efi_image_regions *regs;
> > > > +     struct efi_firmware_image_authentication *auth_hdr;
> > > > +     efi_status_t status;
> > > > +
> > > > +     status = EFI_SECURITY_VIOLATION;
> > > > +     capsule_sig = NULL;
> > > > +     truststore = NULL;
> > > > +     regs = NULL;
> > > > +
> > > > +     /* Sanity checks */
> > > > +     if (capsule == NULL || capsule_size == 0)
> > > > +             goto out;
> > > > +
> > > > +     auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > > > +     if (capsule_size < sizeof(*auth_hdr))
> > > > +             goto out;
> > > > +
> > > > +     if (auth_hdr->auth_info.hdr.dwLength <=
> > > > +         offsetof(struct win_certificate_uefi_guid, cert_data))
> > > > +             goto out;
> > > > +
> > > > +     if (guidcmp(&auth_hdr->auth_info.cert_type,
> > > &efi_guid_cert_type_pkcs7))
> > > > +             goto out;
> > > > +
> > > > +     *image = (uint8_t *)capsule +
> sizeof(auth_hdr->monotonic_count) +
> > > > +             auth_hdr->auth_info.hdr.dwLength;
> > > > +     *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > > > +
>  sizeof(auth_hdr->monotonic_count);
> > > > +     memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> > > > +            sizeof(monotonic_count));
> > > > +
> > > > +     /* data to be digested */
> > > > +     regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2,
> 1);
> > > > +     if (!regs)
> > > > +             goto out;
> > > > +
> > > > +     regs->max = 2;
> > > > +     efi_image_region_add(regs, (uint8_t *)*image,
> > > > +                          (uint8_t *)*image + *image_size, 1);
> > > > +
> > > > +     efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> > > > +                          (uint8_t *)&monotonic_count +
> > > sizeof(monotonic_count),
> > > > +                          1);
> > >
> > > Is the order of regions to be calculated correct?
> > > It seems that 'monotonic_count' precedes 'image' itself
> > > in a capsule header.
> > >
> >
> > Does that have any impact on the hash value computed.
>
> Not 100% sure, but if it doesn't, it cannot guarantee uniqueness
> of digest values.
>

Will check this.

-sughosh


>
> Thanks,
> -Takahiro Akashi
>
> > I did not see any
> > difference in the hash value computed due to the order in which the
> regions
> > are added. But that could be because of the way the hash value gets
> > computed at the time of building the capsule. I will check this out.
> >
> >
> > >
> > > > +
> > > > +     capsule_sig =
> efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> > > > +
> > > auth_hdr->auth_info.hdr.dwLength
> > > > +                                          -
> > > sizeof(auth_hdr->auth_info));
> > > > +     if (IS_ERR(capsule_sig)) {
> > >
> > > As Patrick reported, ex-efi_variable_parse_signature()
> > > returns NULL on error cases, this check should be "!capsule_sig."
> > >
> >
> > Will change.
> >
> > -sughosh
>

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Sughosh Ganu]

On Sat, 9 May 2020 at 15:34, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> On 5/6/20 5:04 PM, Grant Likely wrote:
> >
> >
> > On 05/05/2020 18:57, Heinrich Schuchardt wrote:
> >> On 05.05.20 19:23, Grant Likely wrote:
> >>>
> >>>
> >>> On 05/05/2020 18:04, Heinrich Schuchardt wrote:
> >>>> On 05.05.20 13:15, Grant Likely wrote:
> >>>>>
> >>>>>
> >>>>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
> >>>>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
> >>>>>>>
> >>>>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <
> xypron.glpk at gmx.de
> >>>>>>> <mailto:xypron.glpk@gmx.de>> wrote:
> >>>>>>>
> >>>>>>>        On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> >>>>>>>        > Add support for the get_image_info and set_image routines,
> >>>>>>> which are
> >>>>>>>        > part of the efi firmware management protocol.
> >>>>>>>        >
> >>>>>>>        > The current implementation uses the set_image routine for
> >>>>>>> updating the
> >>>>>>>        > u-boot binary image for the qemu arm64 platform. This is
> >>>>>>> supported
> >>>>>>>        > using the capsule-on-disk feature of the uefi
> >>>>>>> specification,
> >>>>>>> wherein
> >>>>>>>        > the firmware image to be updated is placed on the efi
> >>>>>>> system
> >>>>>>> partition
> >>>>>>>        > as a efi capsule under EFI/UpdateCapsule/ directory.
> >>>>>>> Support
> >>>>>>> has been
> >>>>>>>        > added for updating the u-boot image on platforms booting
> >>>>>>> with arm
> >>>>>>>        > trusted firmware(tf-a), where the u-boot image gets
> >>>>>>> booted as
> >>>>>>> the BL33
> >>>>>>>        > payload(bl33.bin).
> >>>>>>>        >
> >>>>>>>        > The feature can be enabled by the following config options
> >>>>>>>        >
> >>>>>>>        > CONFIG_EFI_CAPSULE_ON_DISK=y
> >>>>>>>        > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
> >>>>>>>        >
> >>>>>>>        > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
> >>>>>>>        <mailto:sughosh.ganu@linaro.org>>
> >>>>>>>
> >>>>>>>        U-Boot's UEFI subsystem should work in the same way for x86,
> >>>>>>> ARM, and
> >>>>>>>        RISC-V. Please, come up with an architecture independent
> >>>>>>> solution.
> >>>>>>>
> >>>>>>>
> >>>>>>> Please check the explanation that I gave in the other mail. If you
> >>>>>>> check
> >>>>>>> the patch series, the actual capsule authentication logic has been
> >>>>>>> kept
> >>>>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very
> >>>>>>> much
> >>>>>>> intended for allowing platforms to define their firmware update
> >>>>>>> routines. Edk2 also has platform specific implementation of the fmp
> >>>>>>> protocol under the edk2-platforms directory.
> >>>>>>>
> >>>>>>> -sughosh
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> My idea is that for most platforms it will be enough to have a
> common
> >>>>>> FMP implementation that consumes a capsule
> >>>>>>
> >>>>>> * with one or more binaries
> >>>>>> * a media device path, a start address, and a truncation flag
> >>>>>>      for each of the binaries
> >>>>>>
> >>>>>> The protocol implementation then will write the binaries to the
> >>>>>> device
> >>>>>> paths:
> >>>>>>
> >>>>>> * to an SD-Card or eMMC exposing the Block IO protocol
> >>>>>>      for most devices
> >>>>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
> >>>>>>      (and truncate it if the truncation flag is set)
> >>>>>
> >>>>> Does U-Boot have a common device path protocol that can be backed by
> >>>>> either a block device or a file on a filesystem? I didn't think it
> >>>>> did.
> >>>>
> >>>> A block device, a partition, and a file all can be described by an
> UEFI
> >>>> media device path.
> >>>
> >>> Sure, from a UEFI media path, but does the underlying U-Boot
> >>> implementation have that abstraction?
> >>
> >>
> >> With a file media path we can find the partition device path on which
> >> the Simple file protocol is already installed by U-Boot.
> >>
> >>>
> >>>>> In the mean time, there are at least three backends that the FMP is
> >>>>> going to have to deal with; the two you list above (block device &
> >>>>> file)
> >>>>> and SMC backed when updating firmware is managed by the secure world.
> >>>>> This first implementation only handles the file-backed use case.
> >>>>> Can we
> >>>>> start with that limitation and refactor when the block-device and SMC
> >>>>> use cases are added in? I would hate to see this functionality held
> up
> >>>>> on having to refactor other functionality in U-Boot.
> >>>>
> >>>> I would prefer one single FMP driver for all SMC use cases. Everything
> >>>> device specific should be handled in the secure world.
> >>>
> >>> Not all platforms will be able to put firmware update into the secure
> >>> world. For instance, Not many Arm v7 platforms have trustzone
> accessible
> >>> to open source developers. On non-secure platforms (e.g., anything that
> >>> loads firmware from a regular filesystem on SD or eMMC) it doesn't make
> >>> much sense to loop out to secure firmware when U-Boot owns the
> >>> filesystem and the secure world would then need to coordinate with
> >>> U-Boot to commit the writes.
> >>>
> >>> All of the code can certainly be in the same location, but I do think
> >>> there are three distinct generic backends for firmware updates:
> >>> - normal-world file-backed (using filesystem)
> >>> - normal-world block-backed (offsets from start of device)
> >>
> >> These two could be in one instance of the FMP protocol.
> >>
> >>> - secure device backed (needs to go into secure world for unpacking and
> >>> processing regardless)
> >>>
> >>> There doesn't need to be platform-specific code in any of those back
> >>> ends, but they do have different behaviour.
> >>>
> >>>>
> >>>> Is there already a protocol defined for the communication of capsule
> >>>> updates between the firmware and the secure monitor, e.g. in EDK2?
> >>>
> >>> Nothing defined yet (see below)
> >>>
> >>>> Would we simply use the UpdateCapsule() call parameters and pass them
> >>>> via an SMC call?
> >>>
> >>> If secure world is handling the update? Yes, I think a thin
> >>> UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
> >>> runtime becomes feasible on U-Boot. There are a couple of people inside
> >>> Arm looking at possible interfaces. In this situation there is very
> >>> little done in normal-world at all.
> >>>
> >>> [...]
> >>>
> >>>>
> >>>> According to my call with Sughosh the whole semihosting thing is about
> >>>> providing a testing possibility not about any real use case.
> >>>
> >>> Yes, it's for testing, but it is particularly valuable testing because
> >>> it allows the host filesystem to be exposed into QEMU. Exposing
> >>> semihosting as a generic fstype_info in U-Boot is generically useful
> >>> apart from this entire discussion!  :-)
> >>>
> >>> If we rework the semihosting code to be just another FS driver, then I
> >>> think it is just an implementation detail on the file-backed firmware
> >>> update path.
> >>
> >> In this case it could be completely separate from this patch series.
> >>
> >> Where do you see the benefit of semi-hosting compared to mounting an
> >> image?
> >
> > Both are important. We need test benches for both file and block device
> > update scenarios.
> >
> > g.
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>> On QEMU you can easily mount an image as block device using parameters
> >>>> of the qemu-system-* command. On the mounted image/block-device you
> can
> >>>> test both file and block device based firmware updates. On the sandbox
> >>>> you could use the 'host bind' command for mounting an image.
> >>>
> >>> Similar to the above, this would be an implementation detail on the
> >>> block-device backed firmware update path. We need both.
> >>>
> >>> g.
> >>> IMPORTANT NOTICE: The contents of this email and any attachments are
> >>> confidential and may also be privileged. If you are not the intended
> >>> recipient, please notify the sender immediately and do not disclose the
> >>> contents to any other person, use it for any purpose, or store or copy
> >>> the information in any medium. Thank you.
> >>
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> > confidential and may also be privileged. If you are not the intended
> > recipient, please notify the sender immediately and do not disclose the
> > contents to any other person, use it for any purpose, or store or copy
> > the information in any medium. Thank you.
>
> [08.05  23:22] <apalos> who will provide the file path protocol and
> potential offsets?
> [09.05  12:02] <xypron> The media device path and the offsets have to be
> provided by the one who builds the capsule and the must be integral part
> of the capsule.
>

Which part of the capsule do you propose to have this information. The
Firmware management protocol capsule structure does have vendor code,
which, as per the spec is vendor specific binary code which can be used to
"implement vendor-specific firmware image update policy". There is no
provision for passing such information as part of the capsule. Why can we
not get this information from the platform code. Is there any issue with
getting the media device path and offset information from the platform.

-sughosh

[PATCH 6/8] efi: capsule: Add support for uefi capsule authentication

[Akashi Takahiro]

On Sun, May 10, 2020 at 04:56:21PM +0530, Sughosh Ganu wrote:
> On Fri, 8 May 2020 at 06:12, Akashi Takahiro <takahiro.akashi@linaro.org>
> wrote:
> 
> > On Thu, May 07, 2020 at 05:20:35PM +0530, Sughosh Ganu wrote:
> > > On Thu, 7 May 2020 at 13:49, Akashi Takahiro <takahiro.akashi@linaro.org
> > >
> > > wrote:
> > >
> > > > Sughosh,
> > > >
> > > > On Thu, Apr 30, 2020 at 11:06:28PM +0530, Sughosh Ganu wrote:
> > > > > Add support for authenticating uefi capsules. Most of the signature
> > > > > verification functionality is shared with the uefi secure boot
> > > > > feature.
> > > > >
> > > > > The root certificate containing the public key used for the signature
> > > > > verification is stored as an efi variable, similar to the variables
> > > > > used for uefi secure boot. The root certificate is stored as an efi
> > > > > signature list(esl) file -- this file contains the x509 certificate
> > > > > which is the root certificate.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > >  include/efi_api.h              |  17 +++++
> > > > >  include/efi_loader.h           |   8 ++-
> > > > >  lib/efi_loader/Kconfig         |  16 +++++
> > > > >  lib/efi_loader/efi_capsule.c   | 126
> > +++++++++++++++++++++++++++++++++
> > > > >  lib/efi_loader/efi_signature.c |   4 +-
> > > > >  5 files changed, 167 insertions(+), 4 deletions(-)
> > > > >
> > > >
> > >
> > > <snip>
> > >
> > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > > > > index ec2976ceba..245deabbed 100644
> > > > > --- a/lib/efi_loader/Kconfig
> > > > > +++ b/lib/efi_loader/Kconfig
> > > > > @@ -110,6 +110,22 @@ config EFI_CAPSULE_FIT_DEVICE
> > > > >       help
> > > > >         Define storage device, say 1:1, for storing FIT image
> > > > >
> > > > > +config EFI_CAPSULE_AUTHENTICATE
> > > > > +     bool "Update Capsule authentication"
> > > > > +     depends on EFI_HAVE_CAPSULE_SUPPORT
> > > > > +     depends on EFI_CAPSULE_ON_DISK
> > > > > +     depends on EFI_FIRMWARE_MANAGEMENT_PROTOCOL
> > > >
> > > > Do we need those two configurations?
> > > >
> > >
> > > Right, I think we can just do with the EFI_CAPSULE_ON_DISK. Will change.
> >
> > Actually I don't think we need EFI_CAPSULE_ON_DISK neither.
> >
> 
> We at least need EFI_HAVE_CAPSULE_SUPPORT, isn't it.

Ah, yes of course.

-Takahiro Akashi
> 
> >
> > >
> > > >
> > > > > +     select SHA256
> > > > > +     select RSA
> > > > > +     select RSA_VERIFY
> > > > > +     select RSA_VERIFY_WITH_PKEY
> > > > > +     select X509_CERTIFICATE_PARSER
> > > > > +     select PKCS7_MESSAGE_PARSER
> > > > > +     default n
> > > > > +     help
> > > > > +       Select this option if you want to enable capsule
> > > > > +       authentication
> > > > > +
> > > > >  config EFI_DEVICE_PATH_TO_TEXT
> > > > >       bool "Device path to text protocol"
> > > > >       default y
> > > > > diff --git a/lib/efi_loader/efi_capsule.c
> > b/lib/efi_loader/efi_capsule.c
> > > > > index 931d363edc..a265d36ff0 100644
> > > > > --- a/lib/efi_loader/efi_capsule.c
> > > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > > @@ -12,6 +12,10 @@
> > > > >  #include <malloc.h>
> > > > >  #include <mapmem.h>
> > > > >  #include <sort.h>
> > > > > +#include "../lib/crypto/pkcs7_parser.h"
> > > > > +
> > > >
> > > > As you might notice, the location was changed by
> > > > my recent patch.
> > > >
> > >
> > > Will check and update.
> > >
> > >
> > > >
> > > > > +#include <crypto/pkcs7.h>
> > > > > +#include <linux/err.h>
> > > > >
> > > > >  const efi_guid_t efi_guid_capsule_report = EFI_CAPSULE_REPORT_GUID;
> > > > >  static const efi_guid_t efi_guid_firmware_management_capsule_id =
> > > > > @@ -245,6 +249,128 @@ out:
> > > > >
> > > > >       return ret;
> > > > >  }
> > > > > +
> > > > > +#if defined(CONFIG_EFI_CAPSULE_AUTHENTICATE)
> > > > > +
> > > > > +const efi_guid_t efi_guid_capsule_root_cert_guid =
> > > > > +     EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > > > > +
> > > > > +__weak u16 *efi_get_truststore_name(void)
> > > > > +{
> > > > > +     return L"CRT";
> > > >
> > > > This variable is not defined by UEFI specification, isn't it?
> > > > How can this variable be protected?
> > > >
> > >
> > > This is not part of the uefi specification. The uefi specification does
> > not
> > > mandate any particular method for storing the root certificate to be used
> > > for capsule authentication. The edk2 code gets the root certificate
> > through
> > > a pcd. I had tried using KEK variable for storing the root certificate,
> > and
> > > had also come up with an implementation. However, the addition/deletion
> > and
> > > update of the secure variables is very closely tied with the secure boot
> > > feature and the secure boot state of the system, which is the reason why
> > i
> > > dropped that solution and used a separate variable, which can be defined
> > by
> > > the vendor to store the root certificate.
> >
> > My concern is that, without any protection, the value of this variable
> > can be modified by a mal-user.
> > (One simple solution would be to set this variable read-only.)
> >
> 
> Yes, that is one solution. This will also be taken care of in a scenario
> where the platform is booted in a trusted chain, and the env is also part
> of the image which gets verified by the previous stage(e.g BL2) before
> jumping to the u-boot image. If there is any change in the u-boot image,
> that would result in a boot failure -- but that would need the env to be
> part of the u-boot image which gets verified. I will explore making this
> variable read-only.
> 
> 
> > >
> > > >
> > > > > +}
> > > > > +
> > > > > +__weak const efi_guid_t *efi_get_truststore_vendor(void)
> > > > > +{
> > > > > +
> > > > > +     return &efi_guid_capsule_root_cert_guid;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * efi_capsule_authenticate() - Authenticate a uefi capsule
> > > > > + *
> > > > > + * @capsule:         Capsule file with the authentication
> > > > > + *                   header
> > > > > + * @capsule_size:    Size of the entire capsule
> > > > > + * @image:           pointer to the image payload minus the
> > > > > + *                   authentication header
> > > > > + * @image_size:              size of the image payload
> > > > > + *
> > > > > + * Authenticate the capsule signature with the public key contained
> > > > > + * in the root certificate stored as an efi environment variable
> > > > > + *
> > > > > + * Return: EFI_SUCCESS on successfull authentication or
> > > > > + *      EFI_SECURITY_VIOLATION on authentication failure
> > > > > + */
> > > > > +efi_status_t efi_capsule_authenticate(const void *capsule,
> > > > > +                                   efi_uintn_t capsule_size,
> > > > > +                                   void **image, efi_uintn_t
> > > > *image_size)
> > > > > +{
> > > > > +     uint64_t monotonic_count;
> > > > > +     struct efi_signature_store *truststore;
> > > > > +     struct pkcs7_message *capsule_sig;
> > > > > +     struct efi_image_regions *regs;
> > > > > +     struct efi_firmware_image_authentication *auth_hdr;
> > > > > +     efi_status_t status;
> > > > > +
> > > > > +     status = EFI_SECURITY_VIOLATION;
> > > > > +     capsule_sig = NULL;
> > > > > +     truststore = NULL;
> > > > > +     regs = NULL;
> > > > > +
> > > > > +     /* Sanity checks */
> > > > > +     if (capsule == NULL || capsule_size == 0)
> > > > > +             goto out;
> > > > > +
> > > > > +     auth_hdr = (struct efi_firmware_image_authentication *)capsule;
> > > > > +     if (capsule_size < sizeof(*auth_hdr))
> > > > > +             goto out;
> > > > > +
> > > > > +     if (auth_hdr->auth_info.hdr.dwLength <=
> > > > > +         offsetof(struct win_certificate_uefi_guid, cert_data))
> > > > > +             goto out;
> > > > > +
> > > > > +     if (guidcmp(&auth_hdr->auth_info.cert_type,
> > > > &efi_guid_cert_type_pkcs7))
> > > > > +             goto out;
> > > > > +
> > > > > +     *image = (uint8_t *)capsule +
> > sizeof(auth_hdr->monotonic_count) +
> > > > > +             auth_hdr->auth_info.hdr.dwLength;
> > > > > +     *image_size = capsule_size - auth_hdr->auth_info.hdr.dwLength -
> > > > > +
> >  sizeof(auth_hdr->monotonic_count);
> > > > > +     memcpy(&monotonic_count, &auth_hdr->monotonic_count,
> > > > > +            sizeof(monotonic_count));
> > > > > +
> > > > > +     /* data to be digested */
> > > > > +     regs = calloc(sizeof(*regs) + sizeof(struct image_region) * 2,
> > 1);
> > > > > +     if (!regs)
> > > > > +             goto out;
> > > > > +
> > > > > +     regs->max = 2;
> > > > > +     efi_image_region_add(regs, (uint8_t *)*image,
> > > > > +                          (uint8_t *)*image + *image_size, 1);
> > > > > +
> > > > > +     efi_image_region_add(regs, (uint8_t *)&monotonic_count,
> > > > > +                          (uint8_t *)&monotonic_count +
> > > > sizeof(monotonic_count),
> > > > > +                          1);
> > > >
> > > > Is the order of regions to be calculated correct?
> > > > It seems that 'monotonic_count' precedes 'image' itself
> > > > in a capsule header.
> > > >
> > >
> > > Does that have any impact on the hash value computed.
> >
> > Not 100% sure, but if it doesn't, it cannot guarantee uniqueness
> > of digest values.
> >
> 
> Will check this.
> 
> -sughosh
> 
> 
> >
> > Thanks,
> > -Takahiro Akashi
> >
> > > I did not see any
> > > difference in the hash value computed due to the order in which the
> > regions
> > > are added. But that could be because of the way the hash value gets
> > > computed at the time of building the capsule. I will check this out.
> > >
> > >
> > > >
> > > > > +
> > > > > +     capsule_sig =
> > efi_parse_pkcs7_header(auth_hdr->auth_info.cert_data,
> > > > > +
> > > > auth_hdr->auth_info.hdr.dwLength
> > > > > +                                          -
> > > > sizeof(auth_hdr->auth_info));
> > > > > +     if (IS_ERR(capsule_sig)) {
> > > >
> > > > As Patrick reported, ex-efi_variable_parse_signature()
> > > > returns NULL on error cases, this check should be "!capsule_sig."
> > > >
> > >
> > > Will change.
> > >
> > > -sughosh
> >

[PATCH 1/8] semihosting: Change semihosting file operation functions into global symbols

[Akashi Takahiro]

On Thu, Apr 30, 2020 at 11:06:23PM +0530, Sughosh Ganu wrote:
> Change the semihosting file operation functions into external symbols
> so that they can be called from outside the file. These functions
> would be required to be called for implementing firmware update
> functionality for the qemu arm64 platform.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  arch/arm/lib/semihosting.c |  7 ++++---
>  include/semihosting.h      | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>  create mode 100644 include/semihosting.h
> 
> diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
> index 2658026cf4..3aeda1303a 100644
> --- a/arch/arm/lib/semihosting.c
> +++ b/arch/arm/lib/semihosting.c
> @@ -13,6 +13,7 @@
>   */
>  #include <common.h>
>  #include <command.h>
> +#include <semihosting.h>

Do we need this file here?

-Takahiro Akashi

>  #define SYSOPEN		0x01
>  #define SYSCLOSE	0x02
> @@ -43,7 +44,7 @@ static noinline long smh_trap(unsigned int sysnum, void *addr)
>   * Open a file on the host. Mode is "r" or "rb" currently. Returns a file
>   * descriptor or -1 on error.
>   */
> -static long smh_open(const char *fname, char *modestr)
> +long smh_open(const char *fname, char *modestr)
>  {
>  	long fd;
>  	unsigned long mode;
> @@ -82,7 +83,7 @@ static long smh_open(const char *fname, char *modestr)
>  /*
>   * Read 'len' bytes of file into 'memp'. Returns 0 on success, else failure
>   */
> -static long smh_read(long fd, void *memp, size_t len)
> +long smh_read(long fd, void *memp, size_t len)
>  {
>  	long ret;
>  	struct smh_read_s {
> @@ -116,7 +117,7 @@ static long smh_read(long fd, void *memp, size_t len)
>  /*
>   * Close the file using the file descriptor
>   */
> -static long smh_close(long fd)
> +long smh_close(long fd)
>  {
>  	long ret;
>  
> diff --git a/include/semihosting.h b/include/semihosting.h
> new file mode 100644
> index 0000000000..f1bf419275
> --- /dev/null
> +++ b/include/semihosting.h
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#if !defined _SEMIHOSTING_H_
> +#define _SEMIHOSTING_H_
> +
> +long smh_open(const char *fname, char *modestr);
> +long smh_read(long fd, void *memp, size_t len);
> +long smh_close(long fd);
> +
> +#endif /* _SEMIHOSTING_H_ */
> -- 
> 2.17.1
> 

[PATCH 1/8] semihosting: Change semihosting file operation functions into global symbols

[Heinrich Schuchardt]

On 5/11/20 5:05 AM, Akashi Takahiro wrote:
> On Thu, Apr 30, 2020 at 11:06:23PM +0530, Sughosh Ganu wrote:
>> Change the semihosting file operation functions into external symbols
>> so that they can be called from outside the file. These functions
>> would be required to be called for implementing firmware update
>> functionality for the qemu arm64 platform.
>>
>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>> ---
>>  arch/arm/lib/semihosting.c |  7 ++++---
>>  include/semihosting.h      | 13 +++++++++++++
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>  create mode 100644 include/semihosting.h
>>
>> diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
>> index 2658026cf4..3aeda1303a 100644
>> --- a/arch/arm/lib/semihosting.c
>> +++ b/arch/arm/lib/semihosting.c

Semihosting in QEMU is not ARM specific but exists also for the M68K,
Xtensa, MIPS, and NIOS2 architecture. arch/arm/lib/ is the wrong place
for this file. This file should be moved to lib/.

The smhload command should be moved to cmd/smhload.c

>> @@ -13,6 +13,7 @@
>>   */
>>  #include <common.h>
>>  #include <command.h>
>> +#include <semihosting.h>
>
> Do we need this file here?

This patch is about turning the functions defined in semihosting.h into
global functions. In patch 3/8 the functions are called from other parts
of the U-Boot code. We need the include here to ensure that the
definitions match.

>
> -Takahiro Akashi
>
>>  #define SYSOPEN		0x01
>>  #define SYSCLOSE	0x02
>> @@ -43,7 +44,7 @@ static noinline long smh_trap(unsigned int sysnum, void *addr)
>>   * Open a file on the host. Mode is "r" or "rb" currently. Returns a file
>>   * descriptor or -1 on error.
>>   */
>> -static long smh_open(const char *fname, char *modestr)
>> +long smh_open(const char *fname, char *modestr)
>>  {
>>  	long fd;
>>  	unsigned long mode;
>> @@ -82,7 +83,7 @@ static long smh_open(const char *fname, char *modestr)
>>  /*
>>   * Read 'len' bytes of file into 'memp'. Returns 0 on success, else failure
>>   */
>> -static long smh_read(long fd, void *memp, size_t len)
>> +long smh_read(long fd, void *memp, size_t len)
>>  {
>>  	long ret;
>>  	struct smh_read_s {
>> @@ -116,7 +117,7 @@ static long smh_read(long fd, void *memp, size_t len)
>>  /*
>>   * Close the file using the file descriptor
>>   */
>> -static long smh_close(long fd)
>> +long smh_close(long fd)
>>  {
>>  	long ret;
>>
>> diff --git a/include/semihosting.h b/include/semihosting.h
>> new file mode 100644
>> index 0000000000..f1bf419275
>> --- /dev/null
>> +++ b/include/semihosting.h
>> @@ -0,0 +1,13 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2020, Linaro Limited
>> + */
>> +
>> +#if !defined _SEMIHOSTING_H_
>> +#define _SEMIHOSTING_H_
>> +

If these functions become global symbols, we should provide a function
description according to

https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

and merge this file into doc/api/index.rst to produce HTML documentation
with 'make htmldoc'.

Cf. https://u-boot.readthedocs.io/en/latest/api/index.html

Best regards

Heinrich

>> +long smh_open(const char *fname, char *modestr);
>> +long smh_read(long fd, void *memp, size_t len);
>> +long smh_close(long fd);
>> +
>> +#endif /* _SEMIHOSTING_H_ */
>> --
>> 2.17.1
>>

[PATCH 2/8] semihosting: Add support for writing to a file

[Heinrich Schuchardt]

On 4/30/20 7:36 PM, Sughosh Ganu wrote:
> Add a function to enable writing to a file. Currently, support is
> added for writing to a binary file. This would be used for
> implementing the firmware update functionality for the qemu arm64
> platform.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  arch/arm/lib/semihosting.c | 41 ++++++++++++++++++++++++++++++++++++--
>  include/semihosting.h      |  1 +
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/lib/semihosting.c b/arch/arm/lib/semihosting.c
> index 3aeda1303a..08181132d1 100644
> --- a/arch/arm/lib/semihosting.c
> +++ b/arch/arm/lib/semihosting.c
> @@ -17,11 +17,18 @@
>
>  #define SYSOPEN		0x01
>  #define SYSCLOSE	0x02
> +#define SYSWRITE	0x5
>  #define SYSREAD		0x06
>  #define SYSFLEN		0x0C
>
> -#define MODE_READ	0x0
> -#define MODE_READBIN	0x1
> +#define MODE_READ		0x0
> +#define MODE_READBIN		0x1
> +#define MODE_READPLUS		0x2
> +#define MODE_READPLUSBIN	0x3
> +#define MODE_WRITE		0x4
> +#define MODE_WRITEBIN		0x5
> +#define MODE_WRITEPLUS		0x6
> +#define MODE_WRITEPLUSBIN	0x7
>
>  /*
>   * Call the handler
> @@ -61,6 +68,8 @@ long smh_open(const char *fname, char *modestr)
>  		mode = MODE_READ;
>  	} else if (!(strcmp(modestr, "rb"))) {
>  		mode = MODE_READBIN;
> +	} else if (!strcmp(modestr, "w+b")) {
> +		mode = MODE_WRITEPLUSBIN;

We could have a string matching each of the 8 constants above:

static char modes[] = "r\0\0\0rb\0\0r+\0\0r+b\0w\0\0\0wb\0\0w+\0\0w+b";
// This should use less bytes than 8 separate strings.

        for (mode = 0; mode < 8; ++mode) {
                if (!strcmp(&modes[4 * mode], modestr))
                        break;
        }
        if (mode & 8)
		printf("%s: ERROR mode \'%s\' not supported\n" ...

But why are we passing the mode as string anyway?
We should use an enum parameter instead.

Best regards

Heinrich

>  	} else {
>  		printf("%s: ERROR mode \'%s\' not supported\n", __func__,
>  		       modestr);
> @@ -114,6 +123,34 @@ long smh_read(long fd, void *memp, size_t len)
>  	return 0;
>  }
>
> +/*
> + * Write 'len' bytes into the file referenced by the fd. Returns 0 on success, else
> + * a negavite value for failure
> + */
> +long smh_write(long fd, void *memp, size_t len)
> +{
> +	long ret;
> +	struct smh_write_s {
> +		long fd;
> +		void *memp;
> +		size_t len;
> +	} write;
> +
> +	debug("%s: fd %ld, memp %p, len %zu\n", __func__, fd, memp, len);
> +
> +	write.fd = fd;
> +	write.memp = memp;
> +	write.len = len;
> +
> +	ret = smh_trap(SYSWRITE, &write);
> +
> +	if (ret > 0)
> +		printf("%s: ERROR ret %ld, fd %ld, len %zu, memp %p\n",
> +		       __func__, ret, fd, len, memp);
> +
> +	return ret == 0 ? 0 : -1;
> +}
> +
>  /*
>   * Close the file using the file descriptor
>   */
> diff --git a/include/semihosting.h b/include/semihosting.h
> index f1bf419275..fa5cecddf2 100644
> --- a/include/semihosting.h
> +++ b/include/semihosting.h
> @@ -8,6 +8,7 @@
>
>  long smh_open(const char *fname, char *modestr);
>  long smh_read(long fd, void *memp, size_t len);
> +long smh_write(long fd, void *memp, size_t len);
>  long smh_close(long fd);
>
>  #endif /* _SEMIHOSTING_H_ */
>

[PATCH 3/8] qemu: arm64: Add support for efi firmware management protocol routines

[Grant Likely]

On 09/05/2020 11:04, Heinrich Schuchardt wrote:
> On 5/6/20 5:04 PM, Grant Likely wrote:
>>
>>
>> On 05/05/2020 18:57, Heinrich Schuchardt wrote:
>>> On 05.05.20 19:23, Grant Likely wrote:
>>>>
>>>>
>>>> On 05/05/2020 18:04, Heinrich Schuchardt wrote:
>>>>> On 05.05.20 13:15, Grant Likely wrote:
>>>>>>
>>>>>>
>>>>>> On 01/05/2020 10:33, Heinrich Schuchardt wrote:
>>>>>>> On 4/30/20 9:13 PM, Sughosh Ganu wrote:
>>>>>>>>
>>>>>>>> On Fri, 1 May 2020 at 00:09, Heinrich Schuchardt <xypron.glpk@gmx.de
>>>>>>>> <mailto:xypron.glpk@gmx.de>> wrote:
>>>>>>>>
>>>>>>>>         On 4/30/20 7:36 PM, Sughosh Ganu wrote:
>>>>>>>>         > Add support for the get_image_info and set_image routines,
>>>>>>>> which are
>>>>>>>>         > part of the efi firmware management protocol.
>>>>>>>>         >
>>>>>>>>         > The current implementation uses the set_image routine for
>>>>>>>> updating the
>>>>>>>>         > u-boot binary image for the qemu arm64 platform. This is
>>>>>>>> supported
>>>>>>>>         > using the capsule-on-disk feature of the uefi
>>>>>>>> specification,
>>>>>>>> wherein
>>>>>>>>         > the firmware image to be updated is placed on the efi
>>>>>>>> system
>>>>>>>> partition
>>>>>>>>         > as a efi capsule under EFI/UpdateCapsule/ directory.
>>>>>>>> Support
>>>>>>>> has been
>>>>>>>>         > added for updating the u-boot image on platforms booting
>>>>>>>> with arm
>>>>>>>>         > trusted firmware(tf-a), where the u-boot image gets
>>>>>>>> booted as
>>>>>>>> the BL33
>>>>>>>>         > payload(bl33.bin).
>>>>>>>>         >
>>>>>>>>         > The feature can be enabled by the following config options
>>>>>>>>         >
>>>>>>>>         > CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>>>>>         > CONFIG_EFI_FIRMWARE_MANAGEMENT_PROTOCOL=y
>>>>>>>>         >
>>>>>>>>         > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>>>>>>>>         <mailto:sughosh.ganu@linaro.org>>
>>>>>>>>
>>>>>>>>         U-Boot's UEFI subsystem should work in the same way for x86,
>>>>>>>> ARM, and
>>>>>>>>         RISC-V. Please, come up with an architecture independent
>>>>>>>> solution.
>>>>>>>>
>>>>>>>>
>>>>>>>> Please check the explanation that I gave in the other mail. If you
>>>>>>>> check
>>>>>>>> the patch series, the actual capsule authentication logic has been
>>>>>>>> kept
>>>>>>>> architecture agnostic, in efi_capsule.c. The fmp protocol is very
>>>>>>>> much
>>>>>>>> intended for allowing platforms to define their firmware update
>>>>>>>> routines. Edk2 also has platform specific implementation of the fmp
>>>>>>>> protocol under the edk2-platforms directory.
>>>>>>>>
>>>>>>>> -sughosh
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> My idea is that for most platforms it will be enough to have a common
>>>>>>> FMP implementation that consumes a capsule
>>>>>>>
>>>>>>> * with one or more binaries
>>>>>>> * a media device path, a start address, and a truncation flag
>>>>>>>       for each of the binaries
>>>>>>>
>>>>>>> The protocol implementation then will write the binaries to the
>>>>>>> device
>>>>>>> paths:
>>>>>>>
>>>>>>> * to an SD-Card or eMMC exposing the Block IO protocol
>>>>>>>       for most devices
>>>>>>> * to a file in case of the Raspberry Pi or the Sandbox or QEMU
>>>>>>>       (and truncate it if the truncation flag is set)
>>>>>>
>>>>>> Does U-Boot have a common device path protocol that can be backed by
>>>>>> either a block device or a file on a filesystem? I didn't think it
>>>>>> did.
>>>>>
>>>>> A block device, a partition, and a file all can be described by an UEFI
>>>>> media device path.
>>>>
>>>> Sure, from a UEFI media path, but does the underlying U-Boot
>>>> implementation have that abstraction?
>>>
>>>
>>> With a file media path we can find the partition device path on which
>>> the Simple file protocol is already installed by U-Boot.
>>>
>>>>
>>>>>> In the mean time, there are at least three backends that the FMP is
>>>>>> going to have to deal with; the two you list above (block device &
>>>>>> file)
>>>>>> and SMC backed when updating firmware is managed by the secure world.
>>>>>> This first implementation only handles the file-backed use case.
>>>>>> Can we
>>>>>> start with that limitation and refactor when the block-device and SMC
>>>>>> use cases are added in? I would hate to see this functionality held up
>>>>>> on having to refactor other functionality in U-Boot.
>>>>>
>>>>> I would prefer one single FMP driver for all SMC use cases. Everything
>>>>> device specific should be handled in the secure world.
>>>>
>>>> Not all platforms will be able to put firmware update into the secure
>>>> world. For instance, Not many Arm v7 platforms have trustzone accessible
>>>> to open source developers. On non-secure platforms (e.g., anything that
>>>> loads firmware from a regular filesystem on SD or eMMC) it doesn't make
>>>> much sense to loop out to secure firmware when U-Boot owns the
>>>> filesystem and the secure world would then need to coordinate with
>>>> U-Boot to commit the writes.
>>>>
>>>> All of the code can certainly be in the same location, but I do think
>>>> there are three distinct generic backends for firmware updates:
>>>> - normal-world file-backed (using filesystem)
>>>> - normal-world block-backed (offsets from start of device)
>>>
>>> These two could be in one instance of the FMP protocol.
>>>
>>>> - secure device backed (needs to go into secure world for unpacking and
>>>> processing regardless)
>>>>
>>>> There doesn't need to be platform-specific code in any of those back
>>>> ends, but they do have different behaviour.
>>>>
>>>>>
>>>>> Is there already a protocol defined for the communication of capsule
>>>>> updates between the firmware and the secure monitor, e.g. in EDK2?
>>>>
>>>> Nothing defined yet (see below)
>>>>
>>>>> Would we simply use the UpdateCapsule() call parameters and pass them
>>>>> via an SMC call?
>>>>
>>>> If secure world is handling the update? Yes, I think a thin
>>>> UpdateCapsule() SMC makes sense, with the bonus that UpdateCapsule() at
>>>> runtime becomes feasible on U-Boot. There are a couple of people inside
>>>> Arm looking at possible interfaces. In this situation there is very
>>>> little done in normal-world at all.
>>>>
>>>> [...]
>>>>
>>>>>
>>>>> According to my call with Sughosh the whole semihosting thing is about
>>>>> providing a testing possibility not about any real use case.
>>>>
>>>> Yes, it's for testing, but it is particularly valuable testing because
>>>> it allows the host filesystem to be exposed into QEMU. Exposing
>>>> semihosting as a generic fstype_info in U-Boot is generically useful
>>>> apart from this entire discussion!  :-)
>>>>
>>>> If we rework the semihosting code to be just another FS driver, then I
>>>> think it is just an implementation detail on the file-backed firmware
>>>> update path.
>>>
>>> In this case it could be completely separate from this patch series.
>>>
>>> Where do you see the benefit of semi-hosting compared to mounting an
>>> image?
>>
>> Both are important. We need test benches for both file and block device
>> update scenarios.
>>
>> g.
>>
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>> On QEMU you can easily mount an image as block device using parameters
>>>>> of the qemu-system-* command. On the mounted image/block-device you can
>>>>> test both file and block device based firmware updates. On the sandbox
>>>>> you could use the 'host bind' command for mounting an image.
>>>>
>>>> Similar to the above, this would be an implementation detail on the
>>>> block-device backed firmware update path. We need both.
>>>>
>>>> g.
>>>> IMPORTANT NOTICE: The contents of this email and any attachments are
>>>> confidential and may also be privileged. If you are not the intended
>>>> recipient, please notify the sender immediately and do not disclose the
>>>> contents to any other person, use it for any purpose, or store or copy
>>>> the information in any medium. Thank you.
>>>
>> IMPORTANT NOTICE: The contents of this email and any attachments are
>> confidential and may also be privileged. If you are not the intended
>> recipient, please notify the sender immediately and do not disclose the
>> contents to any other person, use it for any purpose, or store or copy
>> the information in any medium. Thank you.
>
> [08.05  23:22] <apalos> who will provide the file path protocol and
> potential offsets?
> [09.05  12:02] <xypron> The media device path and the offsets have to be
> provided by the one who builds the capsule and the must be integral part
> of the capsule.
>
> Best regards

I'm not convinced that is true. The capsule may not be able to have that
information. In the case of A/B updates, the firmware needs to track
between the active and inactive copies, and always write the firmware to
the appropriate location. I think it makes more sense for each firmware
blob to be identified for what it is, and the firmware know the location
where each image should be written (or at least have a default write
location built in; there will probably be exceptions once in a while)

g.
>
> Heinrich
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.