[PATCH v2 0/4] FMP versioning support

[PATCH v2 0/4] FMP versioning support

[Masahisa Kojima]

Firmware version management is not implemented in the current
FMP implementation. This series aims to add the versioning support
in FMP.

EDK2 reference implementation utilizes the FMP Payload Header
inserted right before the capsule payload. With this series,
U-Boot also follows the EDK2 implementation.

Currently, there is no way to know the current running firmware
version through the EFI interface. FMP->GetImageInfo() returns
always 0 for the version number. So a user can not know that
expected firmware is running after the capsule update.

With this series applied, version number can be specified
in the capsule file generation with mkeficapsule tool, then
user can know the running firmware version through
FMP->GetImageInfo() and ESRT.

Note that this series does not mandate the FMP Payload Header,
compatible with boards that are already using the existing
U-Boot FMP implementation.
If no FMP Payload Header is found in the capsule file, fw_version,
lowest supported version, last attempt version and last attempt
status is set to 0 and this is the same behavior as existing FMP
implementation.

Changes in v2:
- add FMP Payload Header generation in mkeficapsule tool

Masahisa Kojima (4):
  efi_loader: store firmware version into FmpState variable
  efi_loader: versioning support in GetImageInfo
  efi_loader: check lowest supported version in capsule update
  mkeficapsule: add FMP Payload Header

 lib/efi_loader/efi_firmware.c | 271 ++++++++++++++++++++++++++++++----
 tools/mkeficapsule.c          |  81 +++++++++-
 2 files changed, 319 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] efi_loader: store firmware version into FmpState variable

[Masahisa Kojima]

Firmware version management is not implemented in the current
FMP protocol.
EDK2 reference implementation capsule generation script inserts
the FMP Payload Header right before the payload, it contains the
firmware version and lowest supported version.

This commit utilizes the FMP Payload Header, read the header and
stores the firmware version, lowest supported version,
last attempt version and last attempt status into "FmpStateXXXX"
EFI non-volatile variable. XXXX indicates the image index,
since FMP protocol handles multiple image indexes.

This change is compatible with the existing FMP implementation.
This change does not mandate the FMP Payload Header.
If no FMP Payload Header is found in the capsule file, fw_version,
lowest supported version, last attempt version and last attempt
status is 0 and this is the same behavior as existing FMP
implementation.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- modify indent

 lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
 1 file changed, 175 insertions(+), 23 deletions(-)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 93e2b01c07..d1afafb052 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -10,6 +10,7 @@
 #include <charset.h>
 #include <dfu.h>
 #include <efi_loader.h>
+#include <efi_variable.h>
 #include <fwu.h>
 #include <image.h>
 #include <signatures.h>
@@ -18,6 +19,12 @@
 
 #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
 
+#define EFI_FMP_STATE_GUID \
+	EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
+		 0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
+
+static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
+
 /**
  * struct fmp_payload_header - EDK2 header for the FMP payload
  *
@@ -36,6 +43,24 @@ struct fmp_payload_header {
 	u32 lowest_supported_version;
 };
 
+/**
+ * struct fmp_state - fmp firmware update state
+ *
+ * This structure describes the state of the firmware update
+ * through FMP protocol.
+ *
+ * @fw_version:			Firmware versions used
+ * @lowest_supported_version:	Lowest supported version
+ * @last_attempt_version:	Last attempt version
+ * @last_attempt_status:	Last attempt status
+ */
+struct fmp_state {
+	u32 fw_version;
+	u32 lowest_supported_version;
+	u32 last_attempt_version;
+	u32 last_attempt_status;
+};
+
 __weak void set_dfu_alt_info(char *interface, char *devstr)
 {
 	env_set("dfu_alt_info", update_info.dfu_string);
@@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
  * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
  * @p_image:		Pointer to new image
  * @p_image_size:	Pointer to size of new image
+ * @state		Pointer to fmp state
  *
  * Authenticate the capsule if authentication is enabled.
  * The image pointer and the image size are updated in case of success.
@@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
  */
 static
 efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
-					       efi_uintn_t *p_image_size)
+					       efi_uintn_t *p_image_size,
+					       struct fmp_state *state)
 {
 	const void *image = *p_image;
 	efi_uintn_t image_size = *p_image_size;
-	u32 fmp_hdr_signature;
-	struct fmp_payload_header *header;
 	void *capsule_payload;
 	efi_status_t status;
 	efi_uintn_t capsule_payload_size;
@@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
 
 		if (status == EFI_SECURITY_VIOLATION) {
 			printf("Capsule authentication check failed. Aborting update\n");
+			state->last_attempt_status =
+				LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
 			return status;
 		} else if (status != EFI_SUCCESS) {
+			state->last_attempt_status =
+				LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
 			return status;
 		}
 
@@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
 		debug("Updating capsule without authenticating.\n");
 	}
 
-	fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
-	header = (void *)image;
+	*p_image = image;
+	*p_image_size = image_size;
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
+ * @state:		Pointer to fmp state
+ * @image_index:	image index
+ * @updated:		flag to indicate firmware update is successful
+ *
+ * Update the FmpStateXXXX variable with the firmware update state.
+ *
+ * Return:		status code
+ */
+static
+efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
+					    bool updated)
+{
+	u16 varname[13]; /* u"FmpStateXXXX" */
+	efi_status_t ret;
+	efi_uintn_t size;
+	struct fmp_state var_state = { 0 };
+
+	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
+				image_index);
+	size = sizeof(var_state);
+	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
+				   &var_state, NULL);
+	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
+		return ret;
+
+	/*
+	 * When the capsule update is successful, FmpStateXXXX variable is set
+	 * according to the fmp payload header information. If there is no fmp payload
+	 * header in the capsule file, all values are set to 0.
+	 * When the capsule update fails, only last attempt information of FmpStateXXXX
+	 * variable is updated, fw_version and lowest_supported_version keep original
+	 * value or 0(in case no FmpStateXXXX variable found).
+	 */
+	if (updated) {
+		var_state.fw_version = state->fw_version;
+		var_state.lowest_supported_version = state->lowest_supported_version;
+		var_state.last_attempt_version = state->last_attempt_version;
+		var_state.last_attempt_status = state->last_attempt_status;
+	} else {
+		var_state.last_attempt_version = state->last_attempt_version;
+		var_state.last_attempt_status = state->last_attempt_status;
+	}
+
+	ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
+				   EFI_VARIABLE_NON_VOLATILE |
+				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				   EFI_VARIABLE_RUNTIME_ACCESS,
+				   sizeof(var_state), &var_state, false);
+
+	return ret;
+}
 
+/**
+ * efi_firmware_parse_payload_header - parse FMP payload header
+ * @p_image:		Pointer to new image
+ * @p_image_size:	Pointer to size of new image
+ * @state		Pointer to fmp state
+ *
+ * Parse the FMP payload header and fill the fmp_state structure.
+ * If no FMP payload header is found, fmp_state structure is not updated.
+ *
+ */
+static
+void efi_firmware_parse_payload_header(const void **p_image,
+				       efi_uintn_t *p_image_size,
+				       struct fmp_state *state)
+{
+	const void *image = *p_image;
+	efi_uintn_t image_size = *p_image_size;
+	const struct fmp_payload_header *header;
+	u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
+
+	header = image;
 	if (!memcmp(&header->signature, &fmp_hdr_signature,
 		    sizeof(fmp_hdr_signature))) {
-		/*
-		 * When building the capsule with the scripts in
-		 * edk2, a FMP header is inserted above the capsule
-		 * payload. Compensate for this header to get the
-		 * actual payload that is to be updated.
-		 */
+		/* FMP header is inserted above the capsule payload */
+		state->fw_version = header->fw_version;
+		state->lowest_supported_version = header->lowest_supported_version;
+		state->last_attempt_version = header->fw_version;
 		image += header->header_size;
 		image_size -= header->header_size;
 	}
 
 	*p_image = image;
 	*p_image_size = image_size;
-	return EFI_SUCCESS;
+}
+
+/**
+ * efi_firmware_verify_image - verify image
+ * @p_image:		Pointer to new image
+ * @p_image_size:	Pointer to size of new image
+ * @image_index		Image index
+ * @state		Pointer to fmp state
+ *
+ * Verify the capsule file
+ *
+ * Return:		status code
+ */
+static
+efi_status_t efi_firmware_verify_image(const void **p_image,
+				       efi_uintn_t *p_image_size,
+				       u8 image_index,
+				       struct fmp_state *state)
+{
+	efi_status_t ret;
+
+	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
+	efi_firmware_parse_payload_header(p_image, p_image_size, state);
+
+	return ret;
 }
 
 /**
@@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
 	efi_status_t (*progress)(efi_uintn_t completion),
 	u16 **abort_reason)
 {
+	bool updated;
 	efi_status_t status;
+	struct fmp_state state = { 0 };
 
 	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
 		  image_size, vendor_code, progress, abort_reason);
@@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
 	if (!image || image_index != 1)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	status = efi_firmware_capsule_authenticate(&image, &image_size);
+	status = efi_firmware_verify_image(&image, &image_size, image_index,
+					   &state);
 	if (status != EFI_SUCCESS)
-		return EFI_EXIT(status);
+		goto err;
 
-	if (fit_update(image))
-		return EFI_EXIT(EFI_DEVICE_ERROR);
+	if (fit_update(image)) {
+		status = EFI_DEVICE_ERROR;
+		goto err;
+	}
 
-	return EFI_EXIT(EFI_SUCCESS);
+	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
+err:
+	updated = (status == EFI_SUCCESS) ? true : false;
+	efi_firmware_set_fmp_state_var(&state, image_index, updated);
+
+	return EFI_EXIT(status);
 }
 
 const struct efi_firmware_management_protocol efi_fmp_fit = {
@@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 	u16 **abort_reason)
 {
 	int ret;
+	bool updated;
 	efi_status_t status;
+	struct fmp_state state = { 0 };
 
 	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
 		  image_size, vendor_code, progress, abort_reason);
@@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 	if (!image)
 		return EFI_EXIT(EFI_INVALID_PARAMETER);
 
-	status = efi_firmware_capsule_authenticate(&image, &image_size);
+	status = efi_firmware_verify_image(&image, &image_size, image_index,
+					   &state);
 	if (status != EFI_SUCCESS)
-		return EFI_EXIT(status);
+		goto err;
 
 	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
 		/*
@@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
 		 */
 		ret = fwu_get_image_index(&image_index);
 		if (ret) {
+			state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
 			log_debug("Unable to get FWU image_index\n");
-			return EFI_EXIT(EFI_DEVICE_ERROR);
+			status = EFI_DEVICE_ERROR;
+			goto err;
 		}
 	}
 
 	if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
-			     NULL, NULL))
-		return EFI_EXIT(EFI_DEVICE_ERROR);
+			     NULL, NULL)) {
+		status = EFI_DEVICE_ERROR;
+		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
+		goto err;
+	}
+
+	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
+err:
+	updated = (status == EFI_SUCCESS) ? true : false;
+	efi_firmware_set_fmp_state_var(&state, image_index, updated);
 
-	return EFI_EXIT(EFI_SUCCESS);
+	return EFI_EXIT(status);
 }
 
 const struct efi_firmware_management_protocol efi_fmp_raw = {
-- 
2.17.1

[PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Masahisa Kojima]

Current FMP->GetImageInfo() always return 0 for the firmware
version, user can not identify which firmware version is currently
running through the EFI interface.

This commit reads the "FmpStateXXXX" EFI variable, then fills the
firmware version, lowest supported version, last attempt version
and last attempt status in FMP->GetImageInfo().

Now FMP->GetImageInfo() and ESRT have the meaningful version number.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
No update since v1

 lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index d1afafb052..ead20fa914 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
 	*package_version_name = NULL; /* not supported */
 
 	for (i = 0; i < num_image_type_guids; i++) {
+		u16 varname[13]; /* u"FmpStateXXXX" */
+		efi_status_t ret;
+		efi_uintn_t size;
+		struct fmp_state var_state = { 0 };
+
 		image_info[i].image_index = fw_array[i].image_index;
 		image_info[i].image_type_id = fw_array[i].image_type_id;
 		image_info[i].image_id = fw_array[i].image_index;
 
 		image_info[i].image_id_name = fw_array[i].fw_name;
 
-		image_info[i].version = 0; /* not supported */
+		efi_create_indexed_name(varname, sizeof(varname), "FmpState",
+					fw_array[i].image_index);
+		size = sizeof(var_state);
+		ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
+					   &size, &var_state, NULL);
+		if (ret == EFI_SUCCESS) {
+			image_info[i].version = var_state.fw_version;
+			image_info[i].lowest_supported_image_version =
+				var_state.lowest_supported_version;
+			image_info[i].last_attempt_version =
+				var_state.last_attempt_version;
+			image_info[i].last_attempt_status =
+				var_state.last_attempt_status;
+		} else {
+			image_info[i].version = 0;
+			image_info[i].lowest_supported_image_version = 0;
+			image_info[i].last_attempt_version = 0;
+			image_info[i].last_attempt_status =
+				LAST_ATTEMPT_STATUS_SUCCESS;
+		}
+
 		image_info[i].version_name = NULL; /* not supported */
 		image_info[i].size = 0;
 		image_info[i].attributes_supported =
@@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
 			image_info[0].attributes_setting |=
 				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
 
-		image_info[i].lowest_supported_image_version = 0;
-		image_info[i].last_attempt_version = 0;
-		image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
 		image_info[i].hardware_instance = 1;
 		image_info[i].dependencies = NULL;
 	}
-- 
2.17.1

[PATCH v2 3/4] efi_loader: check lowest supported version in capsule update

[Masahisa Kojima]

The FMP Payload Header which EDK2 capsule generation scripts
insert contains lowest supported version.
This commit reads the lowest supported version stored in the
"FmpStateXXXX" EFI non-volatile variable, then check if the
firmware version of ongoing capsule is equal or greater than
the lowest supported version.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes in v2:
- add error message when the firmware version is lower than
  lowest supported version

 lib/efi_loader/efi_firmware.c | 45 ++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index ead20fa914..fc6babfc34 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -368,6 +368,34 @@ void efi_firmware_parse_payload_header(const void **p_image,
 	*p_image_size = image_size;
 }
 
+/**
+ * efi_firmware_get_lowest_supported_version - get the lowest supported version
+ * @image_index:	image_index
+ *
+ * Get the lowest supported version from FmpStateXXXX variable.
+ *
+ * Return:		lowest supported version, return 0 if reading FmpStateXXXX
+ *			variable failed
+ */
+static
+u32 efi_firmware_get_lowest_supported_version(u8 image_index)
+{
+	u16 varname[13]; /* u"FmpStateXXXX" */
+	efi_status_t ret;
+	efi_uintn_t size;
+	struct fmp_state var_state = { 0 };
+
+	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
+				image_index);
+	size = sizeof(var_state);
+	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
+				   &var_state, NULL);
+	if (ret != EFI_SUCCESS)
+		return 0;
+
+	return var_state.lowest_supported_version;
+}
+
 /**
  * efi_firmware_verify_image - verify image
  * @p_image:		Pointer to new image
@@ -375,7 +403,8 @@ void efi_firmware_parse_payload_header(const void **p_image,
  * @image_index		Image index
  * @state		Pointer to fmp state
  *
- * Verify the capsule file
+ * Verify the capsule authentication and check if the fw_version
+ * is equal or greater than the lowest supported version.
  *
  * Return:		status code
  */
@@ -386,10 +415,24 @@ efi_status_t efi_firmware_verify_image(const void **p_image,
 				       struct fmp_state *state)
 {
 	efi_status_t ret;
+	u32 lowest_supported_version;
 
 	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
 	efi_firmware_parse_payload_header(p_image, p_image_size, state);
 
+	/* check lowest_supported_version if capsule authentication passes */
+	if (ret == EFI_SUCCESS) {
+		lowest_supported_version =
+			efi_firmware_get_lowest_supported_version(image_index);
+		if (lowest_supported_version > state->fw_version) {
+			printf("fw_version(%u) is too low(expected >%u). Aborting update\n",
+			       state->fw_version, lowest_supported_version);
+			state->last_attempt_status =
+				LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION;
+			ret = EFI_INVALID_PARAMETER;
+		}
+	}
+
 	return ret;
 }
 
-- 
2.17.1

[PATCH v2 4/4] mkeficapsule: add FMP Payload Header

[Masahisa Kojima]

Current mkeficapsule tool does not provide firmware
version management. EDK2 reference implementation inserts
the FMP Payload Header right before the payload.
It coutains the fw_version and lowest supported version.

This commit adds three new parameters required to generate
the FMP Payload Header for mkeficapsule tool.
 '-f' indicates whether FMP Payload Header is inserted.
 '-v' indicates the firmware version.
 '-l' indicates the lowest supported version.

When mkeficapsule tool is invoked without '-f' option,
FMP Payload Header is not inserted, the behavior is same as
current implementation.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Newly created in v2

 tools/mkeficapsule.c | 81 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
index b71537beee..e0a6948df8 100644
--- a/tools/mkeficapsule.c
+++ b/tools/mkeficapsule.c
@@ -29,7 +29,7 @@ static const char *tool_name = "mkeficapsule";
 efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
 efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 
-static const char *opts_short = "g:i:I:v:p:c:m:o:dhAR";
+static const char *opts_short = "g:i:I:v:l:p:c:m:o:dfhAR";
 
 enum {
 	CAPSULE_NORMAL_BLOB = 0,
@@ -41,6 +41,9 @@ static struct option options[] = {
 	{"guid", required_argument, NULL, 'g'},
 	{"index", required_argument, NULL, 'i'},
 	{"instance", required_argument, NULL, 'I'},
+	{"fmp-payload-header", no_argument, NULL, 'f'},
+	{"fw-version", required_argument, NULL, 'v'},
+	{"lsv", required_argument, NULL, 'l'},
 	{"private-key", required_argument, NULL, 'p'},
 	{"certificate", required_argument, NULL, 'c'},
 	{"monotonic-count", required_argument, NULL, 'm'},
@@ -60,6 +63,9 @@ static void print_usage(void)
 		"\t-g, --guid <guid string>    guid for image blob type\n"
 		"\t-i, --index <index>         update image index\n"
 		"\t-I, --instance <instance>   update hardware instance\n"
+		"\t-f, --fmp-payload-header    insert fmp payload header\n"
+		"\t-v, --fw-version <version>  firmware version\n"
+		"\t-l, --lsv <version>         lowest supported version\n"
 		"\t-p, --private-key <privkey file>  private key file\n"
 		"\t-c, --certificate <cert file>     signer's certificate file\n"
 		"\t-m, --monotonic-count <count>     monotonic count\n"
@@ -71,6 +77,30 @@ static void print_usage(void)
 		tool_name);
 }
 
+#define SIGNATURE_16(A, B)	((A) | ((B) << 8))
+#define SIGNATURE_32(A, B, C, D)	\
+	(SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16))
+
+#define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
+
+/**
+ * struct fmp_payload_header - EDK2 header for the FMP payload
+ *
+ * This structure describes the header which is preprended to the
+ * FMP payload by the edk2 capsule generation scripts.
+ *
+ * @signature:			Header signature used to identify the header
+ * @header_size:		Size of the structure
+ * @fw_version:			Firmware versions used
+ * @lowest_supported_version:	Lowest supported version
+ */
+struct fmp_payload_header {
+	uint32_t signature;
+	uint32_t header_size;
+	uint32_t fw_version;
+	uint32_t lowest_supported_version;
+};
+
 /**
  * auth_context - authentication context
  * @key_file:	Path to a private key file
@@ -95,6 +125,12 @@ struct auth_context {
 	size_t sig_size;
 };
 
+struct fmp_payload_header_params {
+	bool have_header;
+	uint32_t fw_version;
+	uint32_t lowest_supported_version;
+};
+
 static int dump_sig;
 
 /**
@@ -402,6 +438,7 @@ static void free_sig_data(struct auth_context *ctx)
  */
 static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 			unsigned long index, unsigned long instance,
+			struct fmp_payload_header_params *fmp_ph_params,
 			uint64_t mcount, char *privkey_file, char *cert_file,
 			uint16_t oemflags)
 {
@@ -410,10 +447,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	struct efi_firmware_management_capsule_image_header image;
 	struct auth_context auth_context;
 	FILE *f;
-	uint8_t *data;
+	uint8_t *data, *new_data, *buf;
 	off_t bin_size;
 	uint64_t offset;
 	int ret;
+	struct fmp_payload_header payload_header;
 
 #ifdef DEBUG
 	fprintf(stderr, "For output: %s\n", path);
@@ -423,6 +461,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	auth_context.sig_size = 0;
 	f = NULL;
 	data = NULL;
+	new_data = NULL;
 	ret = -1;
 
 	/*
@@ -431,12 +470,31 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	if (read_bin_file(bin, &data, &bin_size))
 		goto err;
 
+	buf = data;
+
+	/* insert fmp payload header right before the payload */
+	if (fmp_ph_params->have_header) {
+		new_data = malloc(bin_size + sizeof(payload_header));
+		if (!new_data)
+			goto err;
+
+		payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE;
+		payload_header.header_size = sizeof(payload_header);
+		payload_header.fw_version = fmp_ph_params->fw_version;
+		payload_header.lowest_supported_version =
+			fmp_ph_params->lowest_supported_version;
+		memcpy(new_data, &payload_header, sizeof(payload_header));
+		memcpy(new_data + sizeof(payload_header), data, bin_size);
+		buf = new_data;
+		bin_size += sizeof(payload_header);
+	}
+
 	/* first, calculate signature to determine its size */
 	if (privkey_file && cert_file) {
 		auth_context.key_file = privkey_file;
 		auth_context.cert_file = cert_file;
 		auth_context.auth.monotonic_count = mcount;
-		auth_context.image_data = data;
+		auth_context.image_data = buf;
 		auth_context.image_size = bin_size;
 
 		if (create_auth_data(&auth_context)) {
@@ -536,7 +594,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
 	/*
 	 * firmware binary
 	 */
-	if (write_capsule_file(f, data, bin_size, "Firmware binary"))
+	if (write_capsule_file(f, buf, bin_size, "Firmware binary"))
 		goto err;
 
 	ret = 0;
@@ -545,6 +603,7 @@ err:
 		fclose(f);
 	free_sig_data(&auth_context);
 	free(data);
+	free(new_data);
 
 	return ret;
 }
@@ -644,6 +703,7 @@ int main(int argc, char **argv)
 	unsigned long oemflags;
 	char *privkey_file, *cert_file;
 	int c, idx;
+	struct fmp_payload_header_params fmp_ph_params = { 0 };
 
 	guid = NULL;
 	index = 0;
@@ -679,6 +739,15 @@ int main(int argc, char **argv)
 		case 'I':
 			instance = strtoul(optarg, NULL, 0);
 			break;
+		case 'f':
+			fmp_ph_params.have_header = true;
+			break;
+		case 'v':
+			fmp_ph_params.fw_version = strtoul(optarg, NULL, 0);
+			break;
+		case 'l':
+			fmp_ph_params.lowest_supported_version = strtoul(optarg, NULL, 0);
+			break;
 		case 'p':
 			if (privkey_file) {
 				fprintf(stderr,
@@ -747,11 +816,11 @@ int main(int argc, char **argv)
 	if (capsule_type != CAPSULE_NORMAL_BLOB) {
 		if (create_empty_capsule(argv[argc - 1], guid,
 					 capsule_type == CAPSULE_ACCEPT) < 0) {
-			fprintf(stderr, "Creating empty capsule failed\n");
+			fprintf(stderr, "Creating empty capsulefailed\n");
 			exit(EXIT_FAILURE);
 		}
 	} else 	if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
-				 index, instance, mcount, privkey_file,
+				 index, instance, &fmp_ph_params, mcount, privkey_file,
 				 cert_file, (uint16_t)oemflags) < 0) {
 		fprintf(stderr, "Creating firmware capsule failed\n");
 		exit(EXIT_FAILURE);
-- 
2.17.1

Re: [PATCH v2 1/4] efi_loader: store firmware version into FmpState variable

[Takahiro Akashi]

On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP protocol.
> EDK2 reference implementation capsule generation script inserts
> the FMP Payload Header right before the payload, it contains the
> firmware version and lowest supported version.
> 
> This commit utilizes the FMP Payload Header, read the header and
> stores the firmware version, lowest supported version,
> last attempt version and last attempt status into "FmpStateXXXX"
> EFI non-volatile variable. XXXX indicates the image index,
> since FMP protocol handles multiple image indexes.
> 
> This change is compatible with the existing FMP implementation.
> This change does not mandate the FMP Payload Header.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is 0 and this is the same behavior as existing FMP
> implementation.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - modify indent
> 
>  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
>  1 file changed, 175 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 93e2b01c07..d1afafb052 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -10,6 +10,7 @@
>  #include <charset.h>
>  #include <dfu.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <fwu.h>
>  #include <image.h>
>  #include <signatures.h>
> @@ -18,6 +19,12 @@
>  
>  #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
>  
> +#define EFI_FMP_STATE_GUID \
> +	EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> +		 0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> +
> +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> +
>  /**
>   * struct fmp_payload_header - EDK2 header for the FMP payload
>   *
> @@ -36,6 +43,24 @@ struct fmp_payload_header {
>  	u32 lowest_supported_version;
>  };
>  
> +/**
> + * struct fmp_state - fmp firmware update state
> + *
> + * This structure describes the state of the firmware update
> + * through FMP protocol.
> + *
> + * @fw_version:			Firmware versions used
> + * @lowest_supported_version:	Lowest supported version
> + * @last_attempt_version:	Last attempt version
> + * @last_attempt_status:	Last attempt status
> + */
> +struct fmp_state {
> +	u32 fw_version;
> +	u32 lowest_supported_version;
> +	u32 last_attempt_version;
> +	u32 last_attempt_status;
> +};
> +
>  __weak void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	env_set("dfu_alt_info", update_info.dfu_string);
> @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
>   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
>   * @p_image:		Pointer to new image
>   * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
>   *
>   * Authenticate the capsule if authentication is enabled.
>   * The image pointer and the image size are updated in case of success.
> @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
>   */
>  static
>  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> -					       efi_uintn_t *p_image_size)
> +					       efi_uintn_t *p_image_size,
> +					       struct fmp_state *state)
>  {
>  	const void *image = *p_image;
>  	efi_uintn_t image_size = *p_image_size;
> -	u32 fmp_hdr_signature;
> -	struct fmp_payload_header *header;
>  	void *capsule_payload;
>  	efi_status_t status;
>  	efi_uintn_t capsule_payload_size;
> @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  
>  		if (status == EFI_SECURITY_VIOLATION) {
>  			printf("Capsule authentication check failed. Aborting update\n");
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
>  			return status;
>  		} else if (status != EFI_SUCCESS) {
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			return status;
>  		}
>  
> @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  		debug("Updating capsule without authenticating.\n");
>  	}
>  
> -	fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> -	header = (void *)image;
> +	*p_image = image;
> +	*p_image_size = image_size;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> + * @state:		Pointer to fmp state
> + * @image_index:	image index
> + * @updated:		flag to indicate firmware update is successful
> + *
> + * Update the FmpStateXXXX variable with the firmware update state.
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> +					    bool updated)
> +{
> +	u16 varname[13]; /* u"FmpStateXXXX" */
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	struct fmp_state var_state = { 0 };
> +
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				image_index);
> +	size = sizeof(var_state);
> +	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> +				   &var_state, NULL);
> +	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> +		return ret;
> +
> +	/*
> +	 * When the capsule update is successful, FmpStateXXXX variable is set
> +	 * according to the fmp payload header information. If there is no fmp payload
> +	 * header in the capsule file, all values are set to 0.
> +	 * When the capsule update fails, only last attempt information of FmpStateXXXX
> +	 * variable is updated, fw_version and lowest_supported_version keep original
> +	 * value or 0(in case no FmpStateXXXX variable found).
> +	 */
> +	if (updated) {
> +		var_state.fw_version = state->fw_version;
> +		var_state.lowest_supported_version = state->lowest_supported_version;
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	} else {
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	}
> +
> +	ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> +				   EFI_VARIABLE_NON_VOLATILE |
> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> +				   sizeof(var_state), &var_state, false);
> +
> +	return ret;
> +}
>  
> +/**
> + * efi_firmware_parse_payload_header - parse FMP payload header
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
> + *
> + * Parse the FMP payload header and fill the fmp_state structure.
> + * If no FMP payload header is found, fmp_state structure is not updated.
> + *
> + */
> +static
> +void efi_firmware_parse_payload_header(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       struct fmp_state *state)
> +{
> +	const void *image = *p_image;
> +	efi_uintn_t image_size = *p_image_size;
> +	const struct fmp_payload_header *header;
> +	u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> +
> +	header = image;
>  	if (!memcmp(&header->signature, &fmp_hdr_signature,
>  		    sizeof(fmp_hdr_signature))) {
> -		/*
> -		 * When building the capsule with the scripts in
> -		 * edk2, a FMP header is inserted above the capsule
> -		 * payload. Compensate for this header to get the
> -		 * actual payload that is to be updated.
> -		 */
> +		/* FMP header is inserted above the capsule payload */
> +		state->fw_version = header->fw_version;
> +		state->lowest_supported_version = header->lowest_supported_version;
> +		state->last_attempt_version = header->fw_version;
>  		image += header->header_size;
>  		image_size -= header->header_size;
>  	}
>  
>  	*p_image = image;
>  	*p_image_size = image_size;
> -	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_verify_image - verify image
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @image_index		Image index
> + * @state		Pointer to fmp state
> + *
> + * Verify the capsule file
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_verify_image(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       u8 image_index,
> +				       struct fmp_state *state)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> +	efi_firmware_parse_payload_header(p_image, p_image_size, state);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	efi_status_t (*progress)(efi_uintn_t completion),
>  	u16 **abort_reason)
>  {
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	if (!image || image_index != 1)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
> -	if (fit_update(image))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +	if (fit_update(image)) {
> +		status = EFI_DEVICE_ERROR;

I think we need to add here:
		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;

-Takahiro Akashi

> +		goto err;
> +	}
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
> +
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_fit = {
> @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	u16 **abort_reason)
>  {
>  	int ret;
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	if (!image)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
>  	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>  		/*
> @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		 */
>  		ret = fwu_get_image_index(&image_index);
>  		if (ret) {
> +			state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			log_debug("Unable to get FWU image_index\n");
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			status = EFI_DEVICE_ERROR;
> +			goto err;
>  		}
>  	}
>  
>  	if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> -			     NULL, NULL))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +			     NULL, NULL)) {
> +		status = EFI_DEVICE_ERROR;
> +		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +		goto err;
> +	}
> +
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_raw = {
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Takahiro Akashi]

On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote:
> Current FMP->GetImageInfo() always return 0 for the firmware
> version, user can not identify which firmware version is currently
> running through the EFI interface.
> 
> This commit reads the "FmpStateXXXX" EFI variable, then fills the
> firmware version, lowest supported version, last attempt version
> and last attempt status in FMP->GetImageInfo().
> 
> Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No update since v1
> 
>  lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index d1afafb052..ead20fa914 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
>  	*package_version_name = NULL; /* not supported */
>  
>  	for (i = 0; i < num_image_type_guids; i++) {
> +		u16 varname[13]; /* u"FmpStateXXXX" */
> +		efi_status_t ret;
> +		efi_uintn_t size;
> +		struct fmp_state var_state = { 0 };
> +
>  		image_info[i].image_index = fw_array[i].image_index;
>  		image_info[i].image_type_id = fw_array[i].image_type_id;
>  		image_info[i].image_id = fw_array[i].image_index;
>  
>  		image_info[i].image_id_name = fw_array[i].fw_name;
>  
> -		image_info[i].version = 0; /* not supported */
> +		efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +					fw_array[i].image_index);

Don't we have to think of the systems where multiple FMP drivers are
used? In those cases, 'image_index' doesn't work as an unique ID.
It is unlikely under the current code, but we should consider
any future extension.

-Takahiro Akashi


> +		size = sizeof(var_state);
> +		ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
> +					   &size, &var_state, NULL);
> +		if (ret == EFI_SUCCESS) {
> +			image_info[i].version = var_state.fw_version;
> +			image_info[i].lowest_supported_image_version =
> +				var_state.lowest_supported_version;
> +			image_info[i].last_attempt_version =
> +				var_state.last_attempt_version;
> +			image_info[i].last_attempt_status =
> +				var_state.last_attempt_status;
> +		} else {
> +			image_info[i].version = 0;
> +			image_info[i].lowest_supported_image_version = 0;
> +			image_info[i].last_attempt_version = 0;
> +			image_info[i].last_attempt_status =
> +				LAST_ATTEMPT_STATUS_SUCCESS;
> +		}
> +
>  		image_info[i].version_name = NULL; /* not supported */
>  		image_info[i].size = 0;
>  		image_info[i].attributes_supported =
> @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
>  			image_info[0].attributes_setting |=
>  				IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
>  
> -		image_info[i].lowest_supported_image_version = 0;
> -		image_info[i].last_attempt_version = 0;
> -		image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
>  		image_info[i].hardware_instance = 1;
>  		image_info[i].dependencies = NULL;
>  	}
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/4] mkeficapsule: add FMP Payload Header

[Takahiro Akashi]

On Wed, Mar 01, 2023 at 06:15:22PM +0900, Masahisa Kojima wrote:
> Current mkeficapsule tool does not provide firmware
> version management. EDK2 reference implementation inserts
> the FMP Payload Header right before the payload.
> It coutains the fw_version and lowest supported version.
> 
> This commit adds three new parameters required to generate
> the FMP Payload Header for mkeficapsule tool.
>  '-f' indicates whether FMP Payload Header is inserted.
>  '-v' indicates the firmware version.
>  '-l' indicates the lowest supported version.

from user's point of view, '-f' looks redundant since '-v' or '-l'
implicitly means '-f'.

> When mkeficapsule tool is invoked without '-f' option,
> FMP Payload Header is not inserted, the behavior is same as
> current implementation.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Newly created in v2
> 
>  tools/mkeficapsule.c | 81 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 75 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index b71537beee..e0a6948df8 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -29,7 +29,7 @@ static const char *tool_name = "mkeficapsule";
>  efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>  efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>  
> -static const char *opts_short = "g:i:I:v:p:c:m:o:dhAR";
> +static const char *opts_short = "g:i:I:v:l:p:c:m:o:dfhAR";
>  
>  enum {
>  	CAPSULE_NORMAL_BLOB = 0,
> @@ -41,6 +41,9 @@ static struct option options[] = {
>  	{"guid", required_argument, NULL, 'g'},
>  	{"index", required_argument, NULL, 'i'},
>  	{"instance", required_argument, NULL, 'I'},
> +	{"fmp-payload-header", no_argument, NULL, 'f'},
> +	{"fw-version", required_argument, NULL, 'v'},
> +	{"lsv", required_argument, NULL, 'l'},
>  	{"private-key", required_argument, NULL, 'p'},
>  	{"certificate", required_argument, NULL, 'c'},
>  	{"monotonic-count", required_argument, NULL, 'm'},
> @@ -60,6 +63,9 @@ static void print_usage(void)
>  		"\t-g, --guid <guid string>    guid for image blob type\n"
>  		"\t-i, --index <index>         update image index\n"
>  		"\t-I, --instance <instance>   update hardware instance\n"
> +		"\t-f, --fmp-payload-header    insert fmp payload header\n"
> +		"\t-v, --fw-version <version>  firmware version\n"
> +		"\t-l, --lsv <version>         lowest supported version\n"
>  		"\t-p, --private-key <privkey file>  private key file\n"
>  		"\t-c, --certificate <cert file>     signer's certificate file\n"
>  		"\t-m, --monotonic-count <count>     monotonic count\n"
> @@ -71,6 +77,30 @@ static void print_usage(void)
>  		tool_name);
>  }
>  
> +#define SIGNATURE_16(A, B)	((A) | ((B) << 8))
> +#define SIGNATURE_32(A, B, C, D)	\
> +	(SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16))
> +
> +#define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
> +
> +/**
> + * struct fmp_payload_header - EDK2 header for the FMP payload
> + *
> + * This structure describes the header which is preprended to the
> + * FMP payload by the edk2 capsule generation scripts.
> + *
> + * @signature:			Header signature used to identify the header
> + * @header_size:		Size of the structure
> + * @fw_version:			Firmware versions used
> + * @lowest_supported_version:	Lowest supported version
> + */
> +struct fmp_payload_header {
> +	uint32_t signature;
> +	uint32_t header_size;
> +	uint32_t fw_version;
> +	uint32_t lowest_supported_version;
> +};
> +
>  /**
>   * auth_context - authentication context
>   * @key_file:	Path to a private key file
> @@ -95,6 +125,12 @@ struct auth_context {
>  	size_t sig_size;
>  };
>  
> +struct fmp_payload_header_params {
> +	bool have_header;
> +	uint32_t fw_version;
> +	uint32_t lowest_supported_version;
> +};
> +

You may put those definitions in tools/eficapsule.h.

>  static int dump_sig;
>  
>  /**
> @@ -402,6 +438,7 @@ static void free_sig_data(struct auth_context *ctx)
>   */
>  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>  			unsigned long index, unsigned long instance,
> +			struct fmp_payload_header_params *fmp_ph_params,
>  			uint64_t mcount, char *privkey_file, char *cert_file,
>  			uint16_t oemflags)
>  {
> @@ -410,10 +447,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>  	struct efi_firmware_management_capsule_image_header image;
>  	struct auth_context auth_context;
>  	FILE *f;
> -	uint8_t *data;
> +	uint8_t *data, *new_data, *buf;
>  	off_t bin_size;
>  	uint64_t offset;
>  	int ret;
> +	struct fmp_payload_header payload_header;
>  
>  #ifdef DEBUG
>  	fprintf(stderr, "For output: %s\n", path);
> @@ -423,6 +461,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>  	auth_context.sig_size = 0;
>  	f = NULL;
>  	data = NULL;
> +	new_data = NULL;
>  	ret = -1;
>  
>  	/*
> @@ -431,12 +470,31 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>  	if (read_bin_file(bin, &data, &bin_size))
>  		goto err;
>  
> +	buf = data;
> +
> +	/* insert fmp payload header right before the payload */
> +	if (fmp_ph_params->have_header) {
> +		new_data = malloc(bin_size + sizeof(payload_header));
> +		if (!new_data)
> +			goto err;
> +
> +		payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE;
> +		payload_header.header_size = sizeof(payload_header);
> +		payload_header.fw_version = fmp_ph_params->fw_version;
> +		payload_header.lowest_supported_version =
> +			fmp_ph_params->lowest_supported_version;
> +		memcpy(new_data, &payload_header, sizeof(payload_header));
> +		memcpy(new_data + sizeof(payload_header), data, bin_size);
> +		buf = new_data;
> +		bin_size += sizeof(payload_header);
> +	}
> +
>  	/* first, calculate signature to determine its size */
>  	if (privkey_file && cert_file) {
>  		auth_context.key_file = privkey_file;
>  		auth_context.cert_file = cert_file;
>  		auth_context.auth.monotonic_count = mcount;
> -		auth_context.image_data = data;
> +		auth_context.image_data = buf;
>  		auth_context.image_size = bin_size;
>  
>  		if (create_auth_data(&auth_context)) {
> @@ -536,7 +594,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>  	/*
>  	 * firmware binary
>  	 */
> -	if (write_capsule_file(f, data, bin_size, "Firmware binary"))
> +	if (write_capsule_file(f, buf, bin_size, "Firmware binary"))
>  		goto err;
>  
>  	ret = 0;
> @@ -545,6 +603,7 @@ err:
>  		fclose(f);
>  	free_sig_data(&auth_context);
>  	free(data);
> +	free(new_data);
>  
>  	return ret;
>  }
> @@ -644,6 +703,7 @@ int main(int argc, char **argv)
>  	unsigned long oemflags;
>  	char *privkey_file, *cert_file;
>  	int c, idx;
> +	struct fmp_payload_header_params fmp_ph_params = { 0 };
>  
>  	guid = NULL;
>  	index = 0;
> @@ -679,6 +739,15 @@ int main(int argc, char **argv)
>  		case 'I':
>  			instance = strtoul(optarg, NULL, 0);
>  			break;
> +		case 'f':
> +			fmp_ph_params.have_header = true;
> +			break;
> +		case 'v':
> +			fmp_ph_params.fw_version = strtoul(optarg, NULL, 0);
> +			break;
> +		case 'l':
> +			fmp_ph_params.lowest_supported_version = strtoul(optarg, NULL, 0);
> +			break;

Should we check dependencies between two options?
Let's say, '-v' is required if '-l' is provided, and
'fw_version' must be greater than 'lowest_supported_version'.

-Takahiro Akashi

>  		case 'p':
>  			if (privkey_file) {
>  				fprintf(stderr,
> @@ -747,11 +816,11 @@ int main(int argc, char **argv)
>  	if (capsule_type != CAPSULE_NORMAL_BLOB) {
>  		if (create_empty_capsule(argv[argc - 1], guid,
>  					 capsule_type == CAPSULE_ACCEPT) < 0) {
> -			fprintf(stderr, "Creating empty capsule failed\n");
> +			fprintf(stderr, "Creating empty capsulefailed\n");
>  			exit(EXIT_FAILURE);
>  		}
>  	} else 	if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
> -				 index, instance, mcount, privkey_file,
> +				 index, instance, &fmp_ph_params, mcount, privkey_file,
>  				 cert_file, (uint16_t)oemflags) < 0) {
>  		fprintf(stderr, "Creating firmware capsule failed\n");
>  		exit(EXIT_FAILURE);
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/4] efi_loader: store firmware version into FmpState variable

[Masahisa Kojima]

Hi Akashi-san,

On Thu, 2 Mar 2023 at 14:09, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP protocol.
> > EDK2 reference implementation capsule generation script inserts
> > the FMP Payload Header right before the payload, it contains the
> > firmware version and lowest supported version.
> >
> > This commit utilizes the FMP Payload Header, read the header and
> > stores the firmware version, lowest supported version,
> > last attempt version and last attempt status into "FmpStateXXXX"
> > EFI non-volatile variable. XXXX indicates the image index,
> > since FMP protocol handles multiple image indexes.
> >
> > This change is compatible with the existing FMP implementation.
> > This change does not mandate the FMP Payload Header.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - modify indent
> >
> >  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
> >  1 file changed, 175 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 93e2b01c07..d1afafb052 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -10,6 +10,7 @@
> >  #include <charset.h>
> >  #include <dfu.h>
> >  #include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <fwu.h>
> >  #include <image.h>
> >  #include <signatures.h>
> > @@ -18,6 +19,12 @@
> >
> >  #define FMP_PAYLOAD_HDR_SIGNATURE    SIGNATURE_32('M', 'S', 'S', '1')
> >
> > +#define EFI_FMP_STATE_GUID \
> > +     EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> > +              0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> > +
> > +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> > +
> >  /**
> >   * struct fmp_payload_header - EDK2 header for the FMP payload
> >   *
> > @@ -36,6 +43,24 @@ struct fmp_payload_header {
> >       u32 lowest_supported_version;
> >  };
> >
> > +/**
> > + * struct fmp_state - fmp firmware update state
> > + *
> > + * This structure describes the state of the firmware update
> > + * through FMP protocol.
> > + *
> > + * @fw_version:                      Firmware versions used
> > + * @lowest_supported_version:        Lowest supported version
> > + * @last_attempt_version:    Last attempt version
> > + * @last_attempt_status:     Last attempt status
> > + */
> > +struct fmp_state {
> > +     u32 fw_version;
> > +     u32 lowest_supported_version;
> > +     u32 last_attempt_version;
> > +     u32 last_attempt_status;
> > +};
> > +
> >  __weak void set_dfu_alt_info(char *interface, char *devstr)
> >  {
> >       env_set("dfu_alt_info", update_info.dfu_string);
> > @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
> >   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
> >   * @p_image:         Pointer to new image
> >   * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> >   *
> >   * Authenticate the capsule if authentication is enabled.
> >   * The image pointer and the image size are updated in case of success.
> > @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
> >   */
> >  static
> >  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> > -                                            efi_uintn_t *p_image_size)
> > +                                            efi_uintn_t *p_image_size,
> > +                                            struct fmp_state *state)
> >  {
> >       const void *image = *p_image;
> >       efi_uintn_t image_size = *p_image_size;
> > -     u32 fmp_hdr_signature;
> > -     struct fmp_payload_header *header;
> >       void *capsule_payload;
> >       efi_status_t status;
> >       efi_uintn_t capsule_payload_size;
> > @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >
> >               if (status == EFI_SECURITY_VIOLATION) {
> >                       printf("Capsule authentication check failed. Aborting update\n");
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
> >                       return status;
> >               } else if (status != EFI_SUCCESS) {
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       return status;
> >               }
> >
> > @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >               debug("Updating capsule without authenticating.\n");
> >       }
> >
> > -     fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > -     header = (void *)image;
> > +     *p_image = image;
> > +     *p_image_size = image_size;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> > + * @state:           Pointer to fmp state
> > + * @image_index:     image index
> > + * @updated:         flag to indicate firmware update is successful
> > + *
> > + * Update the FmpStateXXXX variable with the firmware update state.
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> > +                                         bool updated)
> > +{
> > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     struct fmp_state var_state = { 0 };
> > +
> > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                             image_index);
> > +     size = sizeof(var_state);
> > +     ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> > +                                &var_state, NULL);
> > +     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +             return ret;
> > +
> > +     /*
> > +      * When the capsule update is successful, FmpStateXXXX variable is set
> > +      * according to the fmp payload header information. If there is no fmp payload
> > +      * header in the capsule file, all values are set to 0.
> > +      * When the capsule update fails, only last attempt information of FmpStateXXXX
> > +      * variable is updated, fw_version and lowest_supported_version keep original
> > +      * value or 0(in case no FmpStateXXXX variable found).
> > +      */
> > +     if (updated) {
> > +             var_state.fw_version = state->fw_version;
> > +             var_state.lowest_supported_version = state->lowest_supported_version;
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     } else {
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     }
> > +
> > +     ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> > +                                EFI_VARIABLE_NON_VOLATILE |
> > +                                EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                sizeof(var_state), &var_state, false);
> > +
> > +     return ret;
> > +}
> >
> > +/**
> > + * efi_firmware_parse_payload_header - parse FMP payload header
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> > + *
> > + * Parse the FMP payload header and fill the fmp_state structure.
> > + * If no FMP payload header is found, fmp_state structure is not updated.
> > + *
> > + */
> > +static
> > +void efi_firmware_parse_payload_header(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    struct fmp_state *state)
> > +{
> > +     const void *image = *p_image;
> > +     efi_uintn_t image_size = *p_image_size;
> > +     const struct fmp_payload_header *header;
> > +     u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > +
> > +     header = image;
> >       if (!memcmp(&header->signature, &fmp_hdr_signature,
> >                   sizeof(fmp_hdr_signature))) {
> > -             /*
> > -              * When building the capsule with the scripts in
> > -              * edk2, a FMP header is inserted above the capsule
> > -              * payload. Compensate for this header to get the
> > -              * actual payload that is to be updated.
> > -              */
> > +             /* FMP header is inserted above the capsule payload */
> > +             state->fw_version = header->fw_version;
> > +             state->lowest_supported_version = header->lowest_supported_version;
> > +             state->last_attempt_version = header->fw_version;
> >               image += header->header_size;
> >               image_size -= header->header_size;
> >       }
> >
> >       *p_image = image;
> >       *p_image_size = image_size;
> > -     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_verify_image - verify image
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @image_index              Image index
> > + * @state            Pointer to fmp state
> > + *
> > + * Verify the capsule file
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_verify_image(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    u8 image_index,
> > +                                    struct fmp_state *state)
> > +{
> > +     efi_status_t ret;
> > +
> > +     ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> > +     efi_firmware_parse_payload_header(p_image, p_image_size, state);
> > +
> > +     return ret;
> >  }
> >
> >  /**
> > @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       efi_status_t (*progress)(efi_uintn_t completion),
> >       u16 **abort_reason)
> >  {
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       if (!image || image_index != 1)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> > -     if (fit_update(image))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +     if (fit_update(image)) {
> > +             status = EFI_DEVICE_ERROR;
>
> I think we need to add here:
>                 state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;

Yes, I will fix it.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
> > +             goto err;
> > +     }
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> > +
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_fit = {
> > @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       u16 **abort_reason)
> >  {
> >       int ret;
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       if (!image)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> >       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> >               /*
> > @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >                */
> >               ret = fwu_get_image_index(&image_index);
> >               if (ret) {
> > +                     state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       log_debug("Unable to get FWU image_index\n");
> > -                     return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                     status = EFI_DEVICE_ERROR;
> > +                     goto err;
> >               }
> >       }
> >
> >       if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> > -                          NULL, NULL))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                          NULL, NULL)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> > +             goto err;
> > +     }
> > +
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_raw = {
> > --
> > 2.17.1
> >

Re: [PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Masahisa Kojima]

On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote:
> > Current FMP->GetImageInfo() always return 0 for the firmware
> > version, user can not identify which firmware version is currently
> > running through the EFI interface.
> >
> > This commit reads the "FmpStateXXXX" EFI variable, then fills the
> > firmware version, lowest supported version, last attempt version
> > and last attempt status in FMP->GetImageInfo().
> >
> > Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > No update since v1
> >
> >  lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
> >  1 file changed, 26 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index d1afafb052..ead20fa914 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
> >       *package_version_name = NULL; /* not supported */
> >
> >       for (i = 0; i < num_image_type_guids; i++) {
> > +             u16 varname[13]; /* u"FmpStateXXXX" */
> > +             efi_status_t ret;
> > +             efi_uintn_t size;
> > +             struct fmp_state var_state = { 0 };
> > +
> >               image_info[i].image_index = fw_array[i].image_index;
> >               image_info[i].image_type_id = fw_array[i].image_type_id;
> >               image_info[i].image_id = fw_array[i].image_index;
> >
> >               image_info[i].image_id_name = fw_array[i].fw_name;
> >
> > -             image_info[i].version = 0; /* not supported */
> > +             efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                                     fw_array[i].image_index);
>
> Don't we have to think of the systems where multiple FMP drivers are
> used? In those cases, 'image_index' doesn't work as an unique ID.
> It is unlikely under the current code, but we should consider
> any future extension.

I assume that other FMP drivers implement their own version management.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > +             size = sizeof(var_state);
> > +             ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
> > +                                        &size, &var_state, NULL);
> > +             if (ret == EFI_SUCCESS) {
> > +                     image_info[i].version = var_state.fw_version;
> > +                     image_info[i].lowest_supported_image_version =
> > +                             var_state.lowest_supported_version;
> > +                     image_info[i].last_attempt_version =
> > +                             var_state.last_attempt_version;
> > +                     image_info[i].last_attempt_status =
> > +                             var_state.last_attempt_status;
> > +             } else {
> > +                     image_info[i].version = 0;
> > +                     image_info[i].lowest_supported_image_version = 0;
> > +                     image_info[i].last_attempt_version = 0;
> > +                     image_info[i].last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_SUCCESS;
> > +             }
> > +
> >               image_info[i].version_name = NULL; /* not supported */
> >               image_info[i].size = 0;
> >               image_info[i].attributes_supported =
> > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
> >                       image_info[0].attributes_setting |=
> >                               IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> >
> > -             image_info[i].lowest_supported_image_version = 0;
> > -             image_info[i].last_attempt_version = 0;
> > -             image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> >               image_info[i].hardware_instance = 1;
> >               image_info[i].dependencies = NULL;
> >       }
> > --
> > 2.17.1
> >

Re: [PATCH v2 4/4] mkeficapsule: add FMP Payload Header

[Masahisa Kojima]

On Thu, 2 Mar 2023 at 14:29, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:22PM +0900, Masahisa Kojima wrote:
> > Current mkeficapsule tool does not provide firmware
> > version management. EDK2 reference implementation inserts
> > the FMP Payload Header right before the payload.
> > It coutains the fw_version and lowest supported version.
> >
> > This commit adds three new parameters required to generate
> > the FMP Payload Header for mkeficapsule tool.
> >  '-f' indicates whether FMP Payload Header is inserted.
> >  '-v' indicates the firmware version.
> >  '-l' indicates the lowest supported version.
>
> from user's point of view, '-f' looks redundant since '-v' or '-l'
> implicitly means '-f'.

I agree, thanks.

>
> > When mkeficapsule tool is invoked without '-f' option,
> > FMP Payload Header is not inserted, the behavior is same as
> > current implementation.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Newly created in v2
> >
> >  tools/mkeficapsule.c | 81 ++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 75 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> > index b71537beee..e0a6948df8 100644
> > --- a/tools/mkeficapsule.c
> > +++ b/tools/mkeficapsule.c
> > @@ -29,7 +29,7 @@ static const char *tool_name = "mkeficapsule";
> >  efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> >  efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >
> > -static const char *opts_short = "g:i:I:v:p:c:m:o:dhAR";
> > +static const char *opts_short = "g:i:I:v:l:p:c:m:o:dfhAR";
> >
> >  enum {
> >       CAPSULE_NORMAL_BLOB = 0,
> > @@ -41,6 +41,9 @@ static struct option options[] = {
> >       {"guid", required_argument, NULL, 'g'},
> >       {"index", required_argument, NULL, 'i'},
> >       {"instance", required_argument, NULL, 'I'},
> > +     {"fmp-payload-header", no_argument, NULL, 'f'},
> > +     {"fw-version", required_argument, NULL, 'v'},
> > +     {"lsv", required_argument, NULL, 'l'},
> >       {"private-key", required_argument, NULL, 'p'},
> >       {"certificate", required_argument, NULL, 'c'},
> >       {"monotonic-count", required_argument, NULL, 'm'},
> > @@ -60,6 +63,9 @@ static void print_usage(void)
> >               "\t-g, --guid <guid string>    guid for image blob type\n"
> >               "\t-i, --index <index>         update image index\n"
> >               "\t-I, --instance <instance>   update hardware instance\n"
> > +             "\t-f, --fmp-payload-header    insert fmp payload header\n"
> > +             "\t-v, --fw-version <version>  firmware version\n"
> > +             "\t-l, --lsv <version>         lowest supported version\n"
> >               "\t-p, --private-key <privkey file>  private key file\n"
> >               "\t-c, --certificate <cert file>     signer's certificate file\n"
> >               "\t-m, --monotonic-count <count>     monotonic count\n"
> > @@ -71,6 +77,30 @@ static void print_usage(void)
> >               tool_name);
> >  }
> >
> > +#define SIGNATURE_16(A, B)   ((A) | ((B) << 8))
> > +#define SIGNATURE_32(A, B, C, D)     \
> > +     (SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16))
> > +
> > +#define FMP_PAYLOAD_HDR_SIGNATURE    SIGNATURE_32('M', 'S', 'S', '1')
> > +
> > +/**
> > + * struct fmp_payload_header - EDK2 header for the FMP payload
> > + *
> > + * This structure describes the header which is preprended to the
> > + * FMP payload by the edk2 capsule generation scripts.
> > + *
> > + * @signature:                       Header signature used to identify the header
> > + * @header_size:             Size of the structure
> > + * @fw_version:                      Firmware versions used
> > + * @lowest_supported_version:        Lowest supported version
> > + */
> > +struct fmp_payload_header {
> > +     uint32_t signature;
> > +     uint32_t header_size;
> > +     uint32_t fw_version;
> > +     uint32_t lowest_supported_version;
> > +};
> > +
> >  /**
> >   * auth_context - authentication context
> >   * @key_file:        Path to a private key file
> > @@ -95,6 +125,12 @@ struct auth_context {
> >       size_t sig_size;
> >  };
> >
> > +struct fmp_payload_header_params {
> > +     bool have_header;
> > +     uint32_t fw_version;
> > +     uint32_t lowest_supported_version;
> > +};
> > +
>
> You may put those definitions in tools/eficapsule.h.

OK.

>
> >  static int dump_sig;
> >
> >  /**
> > @@ -402,6 +438,7 @@ static void free_sig_data(struct auth_context *ctx)
> >   */
> >  static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >                       unsigned long index, unsigned long instance,
> > +                     struct fmp_payload_header_params *fmp_ph_params,
> >                       uint64_t mcount, char *privkey_file, char *cert_file,
> >                       uint16_t oemflags)
> >  {
> > @@ -410,10 +447,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >       struct efi_firmware_management_capsule_image_header image;
> >       struct auth_context auth_context;
> >       FILE *f;
> > -     uint8_t *data;
> > +     uint8_t *data, *new_data, *buf;
> >       off_t bin_size;
> >       uint64_t offset;
> >       int ret;
> > +     struct fmp_payload_header payload_header;
> >
> >  #ifdef DEBUG
> >       fprintf(stderr, "For output: %s\n", path);
> > @@ -423,6 +461,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >       auth_context.sig_size = 0;
> >       f = NULL;
> >       data = NULL;
> > +     new_data = NULL;
> >       ret = -1;
> >
> >       /*
> > @@ -431,12 +470,31 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >       if (read_bin_file(bin, &data, &bin_size))
> >               goto err;
> >
> > +     buf = data;
> > +
> > +     /* insert fmp payload header right before the payload */
> > +     if (fmp_ph_params->have_header) {
> > +             new_data = malloc(bin_size + sizeof(payload_header));
> > +             if (!new_data)
> > +                     goto err;
> > +
> > +             payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > +             payload_header.header_size = sizeof(payload_header);
> > +             payload_header.fw_version = fmp_ph_params->fw_version;
> > +             payload_header.lowest_supported_version =
> > +                     fmp_ph_params->lowest_supported_version;
> > +             memcpy(new_data, &payload_header, sizeof(payload_header));
> > +             memcpy(new_data + sizeof(payload_header), data, bin_size);
> > +             buf = new_data;
> > +             bin_size += sizeof(payload_header);
> > +     }
> > +
> >       /* first, calculate signature to determine its size */
> >       if (privkey_file && cert_file) {
> >               auth_context.key_file = privkey_file;
> >               auth_context.cert_file = cert_file;
> >               auth_context.auth.monotonic_count = mcount;
> > -             auth_context.image_data = data;
> > +             auth_context.image_data = buf;
> >               auth_context.image_size = bin_size;
> >
> >               if (create_auth_data(&auth_context)) {
> > @@ -536,7 +594,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >       /*
> >        * firmware binary
> >        */
> > -     if (write_capsule_file(f, data, bin_size, "Firmware binary"))
> > +     if (write_capsule_file(f, buf, bin_size, "Firmware binary"))
> >               goto err;
> >
> >       ret = 0;
> > @@ -545,6 +603,7 @@ err:
> >               fclose(f);
> >       free_sig_data(&auth_context);
> >       free(data);
> > +     free(new_data);
> >
> >       return ret;
> >  }
> > @@ -644,6 +703,7 @@ int main(int argc, char **argv)
> >       unsigned long oemflags;
> >       char *privkey_file, *cert_file;
> >       int c, idx;
> > +     struct fmp_payload_header_params fmp_ph_params = { 0 };
> >
> >       guid = NULL;
> >       index = 0;
> > @@ -679,6 +739,15 @@ int main(int argc, char **argv)
> >               case 'I':
> >                       instance = strtoul(optarg, NULL, 0);
> >                       break;
> > +             case 'f':
> > +                     fmp_ph_params.have_header = true;
> > +                     break;
> > +             case 'v':
> > +                     fmp_ph_params.fw_version = strtoul(optarg, NULL, 0);
> > +                     break;
> > +             case 'l':
> > +                     fmp_ph_params.lowest_supported_version = strtoul(optarg, NULL, 0);
> > +                     break;
>
> Should we check dependencies between two options?
> Let's say, '-v' is required if '-l' is provided, and
> 'fw_version' must be greater than 'lowest_supported_version'.

Yes, I will add a dependency check in the next version.
Thank you for your review.

Regards,
Masahisa Kojima


>
> -Takahiro Akashi
>
> >               case 'p':
> >                       if (privkey_file) {
> >                               fprintf(stderr,
> > @@ -747,11 +816,11 @@ int main(int argc, char **argv)
> >       if (capsule_type != CAPSULE_NORMAL_BLOB) {
> >               if (create_empty_capsule(argv[argc - 1], guid,
> >                                        capsule_type == CAPSULE_ACCEPT) < 0) {
> > -                     fprintf(stderr, "Creating empty capsule failed\n");
> > +                     fprintf(stderr, "Creating empty capsulefailed\n");
> >                       exit(EXIT_FAILURE);
> >               }
> >       } else  if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
> > -                              index, instance, mcount, privkey_file,
> > +                              index, instance, &fmp_ph_params, mcount, privkey_file,
> >                                cert_file, (uint16_t)oemflags) < 0) {
> >               fprintf(stderr, "Creating firmware capsule failed\n");
> >               exit(EXIT_FAILURE);
> > --
> > 2.17.1
> >

Re: [PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Takahiro Akashi]

On Thu, Mar 02, 2023 at 07:05:50PM +0900, Masahisa Kojima wrote:
> On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> >
> > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote:
> > > Current FMP->GetImageInfo() always return 0 for the firmware
> > > version, user can not identify which firmware version is currently
> > > running through the EFI interface.
> > >
> > > This commit reads the "FmpStateXXXX" EFI variable, then fills the
> > > firmware version, lowest supported version, last attempt version
> > > and last attempt status in FMP->GetImageInfo().
> > >
> > > Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > > No update since v1
> > >
> > >  lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
> > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > index d1afafb052..ead20fa914 100644
> > > --- a/lib/efi_loader/efi_firmware.c
> > > +++ b/lib/efi_loader/efi_firmware.c
> > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
> > >       *package_version_name = NULL; /* not supported */
> > >
> > >       for (i = 0; i < num_image_type_guids; i++) {
> > > +             u16 varname[13]; /* u"FmpStateXXXX" */
> > > +             efi_status_t ret;
> > > +             efi_uintn_t size;
> > > +             struct fmp_state var_state = { 0 };
> > > +
> > >               image_info[i].image_index = fw_array[i].image_index;
> > >               image_info[i].image_type_id = fw_array[i].image_type_id;
> > >               image_info[i].image_id = fw_array[i].image_index;
> > >
> > >               image_info[i].image_id_name = fw_array[i].fw_name;
> > >
> > > -             image_info[i].version = 0; /* not supported */
> > > +             efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > +                                     fw_array[i].image_index);
> >
> > Don't we have to think of the systems where multiple FMP drivers are
> > used? In those cases, 'image_index' doesn't work as an unique ID.
> > It is unlikely under the current code, but we should consider
> > any future extension.
> 
> I assume that other FMP drivers implement their own version management.

I don't have a strong opinion, but my idea about a variable name is
to use 'image_type_id' as a 'vendor' field which allows for making the variable
globally unique.

-Takahiro Akashi


> Thanks,
> Masahisa Kojima
> 
> >
> > -Takahiro Akashi
> >
> >
> > > +             size = sizeof(var_state);
> > > +             ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
> > > +                                        &size, &var_state, NULL);
> > > +             if (ret == EFI_SUCCESS) {
> > > +                     image_info[i].version = var_state.fw_version;
> > > +                     image_info[i].lowest_supported_image_version =
> > > +                             var_state.lowest_supported_version;
> > > +                     image_info[i].last_attempt_version =
> > > +                             var_state.last_attempt_version;
> > > +                     image_info[i].last_attempt_status =
> > > +                             var_state.last_attempt_status;
> > > +             } else {
> > > +                     image_info[i].version = 0;
> > > +                     image_info[i].lowest_supported_image_version = 0;
> > > +                     image_info[i].last_attempt_version = 0;
> > > +                     image_info[i].last_attempt_status =
> > > +                             LAST_ATTEMPT_STATUS_SUCCESS;
> > > +             }
> > > +
> > >               image_info[i].version_name = NULL; /* not supported */
> > >               image_info[i].size = 0;
> > >               image_info[i].attributes_supported =
> > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
> > >                       image_info[0].attributes_setting |=
> > >                               IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> > >
> > > -             image_info[i].lowest_supported_image_version = 0;
> > > -             image_info[i].last_attempt_version = 0;
> > > -             image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > >               image_info[i].hardware_instance = 1;
> > >               image_info[i].dependencies = NULL;
> > >       }
> > > --
> > > 2.17.1
> > >

Re: [PATCH v2 1/4] efi_loader: store firmware version into FmpState variable

[Takahiro Akashi]

On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP protocol.
> EDK2 reference implementation capsule generation script inserts
> the FMP Payload Header right before the payload, it contains the
> firmware version and lowest supported version.
> 
> This commit utilizes the FMP Payload Header, read the header and
> stores the firmware version, lowest supported version,
> last attempt version and last attempt status into "FmpStateXXXX"
> EFI non-volatile variable. XXXX indicates the image index,
> since FMP protocol handles multiple image indexes.
> 
> This change is compatible with the existing FMP implementation.
> This change does not mandate the FMP Payload Header.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is 0 and this is the same behavior as existing FMP
> implementation.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v2:
> - modify indent
> 
>  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
>  1 file changed, 175 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 93e2b01c07..d1afafb052 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -10,6 +10,7 @@
>  #include <charset.h>
>  #include <dfu.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <fwu.h>
>  #include <image.h>
>  #include <signatures.h>
> @@ -18,6 +19,12 @@
>  
>  #define FMP_PAYLOAD_HDR_SIGNATURE	SIGNATURE_32('M', 'S', 'S', '1')
>  
> +#define EFI_FMP_STATE_GUID \
> +	EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> +		 0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> +
> +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> +
>  /**
>   * struct fmp_payload_header - EDK2 header for the FMP payload
>   *
> @@ -36,6 +43,24 @@ struct fmp_payload_header {
>  	u32 lowest_supported_version;
>  };
>  
> +/**
> + * struct fmp_state - fmp firmware update state
> + *
> + * This structure describes the state of the firmware update
> + * through FMP protocol.
> + *
> + * @fw_version:			Firmware versions used
> + * @lowest_supported_version:	Lowest supported version
> + * @last_attempt_version:	Last attempt version
> + * @last_attempt_status:	Last attempt status
> + */
> +struct fmp_state {
> +	u32 fw_version;
> +	u32 lowest_supported_version;
> +	u32 last_attempt_version;
> +	u32 last_attempt_status;
> +};
> +
>  __weak void set_dfu_alt_info(char *interface, char *devstr)
>  {
>  	env_set("dfu_alt_info", update_info.dfu_string);
> @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
>   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
>   * @p_image:		Pointer to new image
>   * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
>   *
>   * Authenticate the capsule if authentication is enabled.
>   * The image pointer and the image size are updated in case of success.
> @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
>   */
>  static
>  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> -					       efi_uintn_t *p_image_size)
> +					       efi_uintn_t *p_image_size,
> +					       struct fmp_state *state)
>  {
>  	const void *image = *p_image;
>  	efi_uintn_t image_size = *p_image_size;
> -	u32 fmp_hdr_signature;
> -	struct fmp_payload_header *header;
>  	void *capsule_payload;
>  	efi_status_t status;
>  	efi_uintn_t capsule_payload_size;
> @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  
>  		if (status == EFI_SECURITY_VIOLATION) {
>  			printf("Capsule authentication check failed. Aborting update\n");
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
>  			return status;
>  		} else if (status != EFI_SUCCESS) {
> +			state->last_attempt_status =
> +				LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			return status;
>  		}
>  
> @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
>  		debug("Updating capsule without authenticating.\n");
>  	}
>  
> -	fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> -	header = (void *)image;
> +	*p_image = image;
> +	*p_image_size = image_size;
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> + * @state:		Pointer to fmp state
> + * @image_index:	image index
> + * @updated:		flag to indicate firmware update is successful
> + *
> + * Update the FmpStateXXXX variable with the firmware update state.
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> +					    bool updated)
> +{
> +	u16 varname[13]; /* u"FmpStateXXXX" */
> +	efi_status_t ret;
> +	efi_uintn_t size;
> +	struct fmp_state var_state = { 0 };
> +
> +	efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> +				image_index);
> +	size = sizeof(var_state);
> +	ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> +				   &var_state, NULL);
> +	if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> +		return ret;
> +
> +	/*
> +	 * When the capsule update is successful, FmpStateXXXX variable is set
> +	 * according to the fmp payload header information. If there is no fmp payload
> +	 * header in the capsule file, all values are set to 0.
> +	 * When the capsule update fails, only last attempt information of FmpStateXXXX
> +	 * variable is updated, fw_version and lowest_supported_version keep original
> +	 * value or 0(in case no FmpStateXXXX variable found).
> +	 */
> +	if (updated) {
> +		var_state.fw_version = state->fw_version;
> +		var_state.lowest_supported_version = state->lowest_supported_version;
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	} else {
> +		var_state.last_attempt_version = state->last_attempt_version;
> +		var_state.last_attempt_status = state->last_attempt_status;
> +	}
> +
> +	ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> +				   EFI_VARIABLE_NON_VOLATILE |
> +				   EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				   EFI_VARIABLE_RUNTIME_ACCESS,
> +				   sizeof(var_state), &var_state, false);

One of my concerns about using UEFI variables as a storage for internal data
like firmwware status is that they are unnecessarily accessible to users.

In this sense, I think that those variable should be *read-only* and
maintained only by the system.

-Takahiro Akashi


> +	return ret;
> +}
>  
> +/**
> + * efi_firmware_parse_payload_header - parse FMP payload header
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @state		Pointer to fmp state
> + *
> + * Parse the FMP payload header and fill the fmp_state structure.
> + * If no FMP payload header is found, fmp_state structure is not updated.
> + *
> + */
> +static
> +void efi_firmware_parse_payload_header(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       struct fmp_state *state)
> +{
> +	const void *image = *p_image;
> +	efi_uintn_t image_size = *p_image_size;
> +	const struct fmp_payload_header *header;
> +	u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> +
> +	header = image;
>  	if (!memcmp(&header->signature, &fmp_hdr_signature,
>  		    sizeof(fmp_hdr_signature))) {
> -		/*
> -		 * When building the capsule with the scripts in
> -		 * edk2, a FMP header is inserted above the capsule
> -		 * payload. Compensate for this header to get the
> -		 * actual payload that is to be updated.
> -		 */
> +		/* FMP header is inserted above the capsule payload */
> +		state->fw_version = header->fw_version;
> +		state->lowest_supported_version = header->lowest_supported_version;
> +		state->last_attempt_version = header->fw_version;
>  		image += header->header_size;
>  		image_size -= header->header_size;
>  	}
>  
>  	*p_image = image;
>  	*p_image_size = image_size;
> -	return EFI_SUCCESS;
> +}
> +
> +/**
> + * efi_firmware_verify_image - verify image
> + * @p_image:		Pointer to new image
> + * @p_image_size:	Pointer to size of new image
> + * @image_index		Image index
> + * @state		Pointer to fmp state
> + *
> + * Verify the capsule file
> + *
> + * Return:		status code
> + */
> +static
> +efi_status_t efi_firmware_verify_image(const void **p_image,
> +				       efi_uintn_t *p_image_size,
> +				       u8 image_index,
> +				       struct fmp_state *state)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> +	efi_firmware_parse_payload_header(p_image, p_image_size, state);
> +
> +	return ret;
>  }
>  
>  /**
> @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	efi_status_t (*progress)(efi_uintn_t completion),
>  	u16 **abort_reason)
>  {
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
>  	if (!image || image_index != 1)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
> -	if (fit_update(image))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +	if (fit_update(image)) {
> +		status = EFI_DEVICE_ERROR;
> +		goto err;
> +	}
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
> +
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_fit = {
> @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	u16 **abort_reason)
>  {
>  	int ret;
> +	bool updated;
>  	efi_status_t status;
> +	struct fmp_state state = { 0 };
>  
>  	EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
>  		  image_size, vendor_code, progress, abort_reason);
> @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  	if (!image)
>  		return EFI_EXIT(EFI_INVALID_PARAMETER);
>  
> -	status = efi_firmware_capsule_authenticate(&image, &image_size);
> +	status = efi_firmware_verify_image(&image, &image_size, image_index,
> +					   &state);
>  	if (status != EFI_SUCCESS)
> -		return EFI_EXIT(status);
> +		goto err;
>  
>  	if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
>  		/*
> @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
>  		 */
>  		ret = fwu_get_image_index(&image_index);
>  		if (ret) {
> +			state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
>  			log_debug("Unable to get FWU image_index\n");
> -			return EFI_EXIT(EFI_DEVICE_ERROR);
> +			status = EFI_DEVICE_ERROR;
> +			goto err;
>  		}
>  	}
>  
>  	if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> -			     NULL, NULL))
> -		return EFI_EXIT(EFI_DEVICE_ERROR);
> +			     NULL, NULL)) {
> +		status = EFI_DEVICE_ERROR;
> +		state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> +		goto err;
> +	}
> +
> +	state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> +err:
> +	updated = (status == EFI_SUCCESS) ? true : false;
> +	efi_firmware_set_fmp_state_var(&state, image_index, updated);
>  
> -	return EFI_EXIT(EFI_SUCCESS);
> +	return EFI_EXIT(status);
>  }
>  
>  const struct efi_firmware_management_protocol efi_fmp_raw = {
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] efi_loader: versioning support in GetImageInfo

[Masahisa Kojima]

Hi Akashi-san,

On Fri, 3 Mar 2023 at 09:10, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Mar 02, 2023 at 07:05:50PM +0900, Masahisa Kojima wrote:
> > On Thu, 2 Mar 2023 at 14:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Wed, Mar 01, 2023 at 06:15:20PM +0900, Masahisa Kojima wrote:
> > > > Current FMP->GetImageInfo() always return 0 for the firmware
> > > > version, user can not identify which firmware version is currently
> > > > running through the EFI interface.
> > > >
> > > > This commit reads the "FmpStateXXXX" EFI variable, then fills the
> > > > firmware version, lowest supported version, last attempt version
> > > > and last attempt status in FMP->GetImageInfo().
> > > >
> > > > Now FMP->GetImageInfo() and ESRT have the meaningful version number.
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > > No update since v1
> > > >
> > > >  lib/efi_loader/efi_firmware.c | 30 ++++++++++++++++++++++++++----
> > > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > > > index d1afafb052..ead20fa914 100644
> > > > --- a/lib/efi_loader/efi_firmware.c
> > > > +++ b/lib/efi_loader/efi_firmware.c
> > > > @@ -173,13 +173,38 @@ static efi_status_t efi_fill_image_desc_array(
> > > >       *package_version_name = NULL; /* not supported */
> > > >
> > > >       for (i = 0; i < num_image_type_guids; i++) {
> > > > +             u16 varname[13]; /* u"FmpStateXXXX" */
> > > > +             efi_status_t ret;
> > > > +             efi_uintn_t size;
> > > > +             struct fmp_state var_state = { 0 };
> > > > +
> > > >               image_info[i].image_index = fw_array[i].image_index;
> > > >               image_info[i].image_type_id = fw_array[i].image_type_id;
> > > >               image_info[i].image_id = fw_array[i].image_index;
> > > >
> > > >               image_info[i].image_id_name = fw_array[i].fw_name;
> > > >
> > > > -             image_info[i].version = 0; /* not supported */
> > > > +             efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > > > +                                     fw_array[i].image_index);
> > >
> > > Don't we have to think of the systems where multiple FMP drivers are
> > > used? In those cases, 'image_index' doesn't work as an unique ID.
> > > It is unlikely under the current code, but we should consider
> > > any future extension.
> >
> > I assume that other FMP drivers implement their own version management.
>
> I don't have a strong opinion, but my idea about a variable name is
> to use 'image_type_id' as a 'vendor' field which allows for making the variable
> globally unique.

Thank you for the suggestion, I agree to use image_type_id as vendor field.

Regards,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > -Takahiro Akashi
> > >
> > >
> > > > +             size = sizeof(var_state);
> > > > +             ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL,
> > > > +                                        &size, &var_state, NULL);
> > > > +             if (ret == EFI_SUCCESS) {
> > > > +                     image_info[i].version = var_state.fw_version;
> > > > +                     image_info[i].lowest_supported_image_version =
> > > > +                             var_state.lowest_supported_version;
> > > > +                     image_info[i].last_attempt_version =
> > > > +                             var_state.last_attempt_version;
> > > > +                     image_info[i].last_attempt_status =
> > > > +                             var_state.last_attempt_status;
> > > > +             } else {
> > > > +                     image_info[i].version = 0;
> > > > +                     image_info[i].lowest_supported_image_version = 0;
> > > > +                     image_info[i].last_attempt_version = 0;
> > > > +                     image_info[i].last_attempt_status =
> > > > +                             LAST_ATTEMPT_STATUS_SUCCESS;
> > > > +             }
> > > > +
> > > >               image_info[i].version_name = NULL; /* not supported */
> > > >               image_info[i].size = 0;
> > > >               image_info[i].attributes_supported =
> > > > @@ -193,9 +218,6 @@ static efi_status_t efi_fill_image_desc_array(
> > > >                       image_info[0].attributes_setting |=
> > > >                               IMAGE_ATTRIBUTE_AUTHENTICATION_REQUIRED;
> > > >
> > > > -             image_info[i].lowest_supported_image_version = 0;
> > > > -             image_info[i].last_attempt_version = 0;
> > > > -             image_info[i].last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > > >               image_info[i].hardware_instance = 1;
> > > >               image_info[i].dependencies = NULL;
> > > >       }
> > > > --
> > > > 2.17.1
> > > >

Re: [PATCH v2 1/4] efi_loader: store firmware version into FmpState variable

[Masahisa Kojima]

On Fri, 3 Mar 2023 at 09:17, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Wed, Mar 01, 2023 at 06:15:19PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP protocol.
> > EDK2 reference implementation capsule generation script inserts
> > the FMP Payload Header right before the payload, it contains the
> > firmware version and lowest supported version.
> >
> > This commit utilizes the FMP Payload Header, read the header and
> > stores the firmware version, lowest supported version,
> > last attempt version and last attempt status into "FmpStateXXXX"
> > EFI non-volatile variable. XXXX indicates the image index,
> > since FMP protocol handles multiple image indexes.
> >
> > This change is compatible with the existing FMP implementation.
> > This change does not mandate the FMP Payload Header.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes in v2:
> > - modify indent
> >
> >  lib/efi_loader/efi_firmware.c | 198 ++++++++++++++++++++++++++++++----
> >  1 file changed, 175 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 93e2b01c07..d1afafb052 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -10,6 +10,7 @@
> >  #include <charset.h>
> >  #include <dfu.h>
> >  #include <efi_loader.h>
> > +#include <efi_variable.h>
> >  #include <fwu.h>
> >  #include <image.h>
> >  #include <signatures.h>
> > @@ -18,6 +19,12 @@
> >
> >  #define FMP_PAYLOAD_HDR_SIGNATURE    SIGNATURE_32('M', 'S', 'S', '1')
> >
> > +#define EFI_FMP_STATE_GUID \
> > +     EFI_GUID(0x84bed885, 0x193a, 0x403f, 0xa2, 0x78, \
> > +              0xe8, 0x9e, 0x23, 0x8a, 0xd6, 0xe1)
> > +
> > +static const efi_guid_t efi_guid_fmp_state = EFI_FMP_STATE_GUID;
> > +
> >  /**
> >   * struct fmp_payload_header - EDK2 header for the FMP payload
> >   *
> > @@ -36,6 +43,24 @@ struct fmp_payload_header {
> >       u32 lowest_supported_version;
> >  };
> >
> > +/**
> > + * struct fmp_state - fmp firmware update state
> > + *
> > + * This structure describes the state of the firmware update
> > + * through FMP protocol.
> > + *
> > + * @fw_version:                      Firmware versions used
> > + * @lowest_supported_version:        Lowest supported version
> > + * @last_attempt_version:    Last attempt version
> > + * @last_attempt_status:     Last attempt status
> > + */
> > +struct fmp_state {
> > +     u32 fw_version;
> > +     u32 lowest_supported_version;
> > +     u32 last_attempt_version;
> > +     u32 last_attempt_status;
> > +};
> > +
> >  __weak void set_dfu_alt_info(char *interface, char *devstr)
> >  {
> >       env_set("dfu_alt_info", update_info.dfu_string);
> > @@ -182,6 +207,7 @@ static efi_status_t efi_fill_image_desc_array(
> >   * efi_firmware_capsule_authenticate - authenticate the capsule if enabled
> >   * @p_image:         Pointer to new image
> >   * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> >   *
> >   * Authenticate the capsule if authentication is enabled.
> >   * The image pointer and the image size are updated in case of success.
> > @@ -190,12 +216,11 @@ static efi_status_t efi_fill_image_desc_array(
> >   */
> >  static
> >  efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> > -                                            efi_uintn_t *p_image_size)
> > +                                            efi_uintn_t *p_image_size,
> > +                                            struct fmp_state *state)
> >  {
> >       const void *image = *p_image;
> >       efi_uintn_t image_size = *p_image_size;
> > -     u32 fmp_hdr_signature;
> > -     struct fmp_payload_header *header;
> >       void *capsule_payload;
> >       efi_status_t status;
> >       efi_uintn_t capsule_payload_size;
> > @@ -209,8 +234,12 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >
> >               if (status == EFI_SECURITY_VIOLATION) {
> >                       printf("Capsule authentication check failed. Aborting update\n");
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
> >                       return status;
> >               } else if (status != EFI_SUCCESS) {
> > +                     state->last_attempt_status =
> > +                             LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       return status;
> >               }
> >
> > @@ -222,24 +251,124 @@ efi_status_t efi_firmware_capsule_authenticate(const void **p_image,
> >               debug("Updating capsule without authenticating.\n");
> >       }
> >
> > -     fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > -     header = (void *)image;
> > +     *p_image = image;
> > +     *p_image_size = image_size;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_set_fmp_state_var - set FmpStateXXXX variable
> > + * @state:           Pointer to fmp state
> > + * @image_index:     image index
> > + * @updated:         flag to indicate firmware update is successful
> > + *
> > + * Update the FmpStateXXXX variable with the firmware update state.
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_index,
> > +                                         bool updated)
> > +{
> > +     u16 varname[13]; /* u"FmpStateXXXX" */
> > +     efi_status_t ret;
> > +     efi_uintn_t size;
> > +     struct fmp_state var_state = { 0 };
> > +
> > +     efi_create_indexed_name(varname, sizeof(varname), "FmpState",
> > +                             image_index);
> > +     size = sizeof(var_state);
> > +     ret = efi_get_variable_int(varname, &efi_guid_fmp_state, NULL, &size,
> > +                                &var_state, NULL);
> > +     if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
> > +             return ret;
> > +
> > +     /*
> > +      * When the capsule update is successful, FmpStateXXXX variable is set
> > +      * according to the fmp payload header information. If there is no fmp payload
> > +      * header in the capsule file, all values are set to 0.
> > +      * When the capsule update fails, only last attempt information of FmpStateXXXX
> > +      * variable is updated, fw_version and lowest_supported_version keep original
> > +      * value or 0(in case no FmpStateXXXX variable found).
> > +      */
> > +     if (updated) {
> > +             var_state.fw_version = state->fw_version;
> > +             var_state.lowest_supported_version = state->lowest_supported_version;
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     } else {
> > +             var_state.last_attempt_version = state->last_attempt_version;
> > +             var_state.last_attempt_status = state->last_attempt_status;
> > +     }
> > +
> > +     ret = efi_set_variable_int(varname, &efi_guid_fmp_state,
> > +                                EFI_VARIABLE_NON_VOLATILE |
> > +                                EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                EFI_VARIABLE_RUNTIME_ACCESS,
> > +                                sizeof(var_state), &var_state, false);
>
> One of my concerns about using UEFI variables as a storage for internal data
> like firmwware status is that they are unnecessarily accessible to users.
>
> In this sense, I think that those variable should be *read-only* and
> maintained only by the system.

Yes, this FmpState variable should be read-only.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
>
> > +     return ret;
> > +}
> >
> > +/**
> > + * efi_firmware_parse_payload_header - parse FMP payload header
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @state            Pointer to fmp state
> > + *
> > + * Parse the FMP payload header and fill the fmp_state structure.
> > + * If no FMP payload header is found, fmp_state structure is not updated.
> > + *
> > + */
> > +static
> > +void efi_firmware_parse_payload_header(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    struct fmp_state *state)
> > +{
> > +     const void *image = *p_image;
> > +     efi_uintn_t image_size = *p_image_size;
> > +     const struct fmp_payload_header *header;
> > +     u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE;
> > +
> > +     header = image;
> >       if (!memcmp(&header->signature, &fmp_hdr_signature,
> >                   sizeof(fmp_hdr_signature))) {
> > -             /*
> > -              * When building the capsule with the scripts in
> > -              * edk2, a FMP header is inserted above the capsule
> > -              * payload. Compensate for this header to get the
> > -              * actual payload that is to be updated.
> > -              */
> > +             /* FMP header is inserted above the capsule payload */
> > +             state->fw_version = header->fw_version;
> > +             state->lowest_supported_version = header->lowest_supported_version;
> > +             state->last_attempt_version = header->fw_version;
> >               image += header->header_size;
> >               image_size -= header->header_size;
> >       }
> >
> >       *p_image = image;
> >       *p_image_size = image_size;
> > -     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * efi_firmware_verify_image - verify image
> > + * @p_image:         Pointer to new image
> > + * @p_image_size:    Pointer to size of new image
> > + * @image_index              Image index
> > + * @state            Pointer to fmp state
> > + *
> > + * Verify the capsule file
> > + *
> > + * Return:           status code
> > + */
> > +static
> > +efi_status_t efi_firmware_verify_image(const void **p_image,
> > +                                    efi_uintn_t *p_image_size,
> > +                                    u8 image_index,
> > +                                    struct fmp_state *state)
> > +{
> > +     efi_status_t ret;
> > +
> > +     ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state);
> > +     efi_firmware_parse_payload_header(p_image, p_image_size, state);
> > +
> > +     return ret;
> >  }
> >
> >  /**
> > @@ -330,7 +459,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       efi_status_t (*progress)(efi_uintn_t completion),
> >       u16 **abort_reason)
> >  {
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -338,14 +469,22 @@ efi_status_t EFIAPI efi_firmware_fit_set_image(
> >       if (!image || image_index != 1)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> > -     if (fit_update(image))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +     if (fit_update(image)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             goto err;
> > +     }
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> > +
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_fit = {
> > @@ -391,7 +530,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       u16 **abort_reason)
> >  {
> >       int ret;
> > +     bool updated;
> >       efi_status_t status;
> > +     struct fmp_state state = { 0 };
> >
> >       EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image,
> >                 image_size, vendor_code, progress, abort_reason);
> > @@ -399,9 +540,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >       if (!image)
> >               return EFI_EXIT(EFI_INVALID_PARAMETER);
> >
> > -     status = efi_firmware_capsule_authenticate(&image, &image_size);
> > +     status = efi_firmware_verify_image(&image, &image_size, image_index,
> > +                                        &state);
> >       if (status != EFI_SUCCESS)
> > -             return EFI_EXIT(status);
> > +             goto err;
> >
> >       if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {
> >               /*
> > @@ -410,16 +552,26 @@ efi_status_t EFIAPI efi_firmware_raw_set_image(
> >                */
> >               ret = fwu_get_image_index(&image_index);
> >               if (ret) {
> > +                     state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >                       log_debug("Unable to get FWU image_index\n");
> > -                     return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                     status = EFI_DEVICE_ERROR;
> > +                     goto err;
> >               }
> >       }
> >
> >       if (dfu_write_by_alt(image_index - 1, (void *)image, image_size,
> > -                          NULL, NULL))
> > -             return EFI_EXIT(EFI_DEVICE_ERROR);
> > +                          NULL, NULL)) {
> > +             status = EFI_DEVICE_ERROR;
> > +             state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> > +             goto err;
> > +     }
> > +
> > +     state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS;
> > +err:
> > +     updated = (status == EFI_SUCCESS) ? true : false;
> > +     efi_firmware_set_fmp_state_var(&state, image_index, updated);
> >
> > -     return EFI_EXIT(EFI_SUCCESS);
> > +     return EFI_EXIT(status);
> >  }
> >
> >  const struct efi_firmware_management_protocol efi_fmp_raw = {
> > --
> > 2.17.1
> >

Re: [PATCH v2 0/4] FMP versioning support

[Takahiro Akashi]

Kojima-san,

On Wed, Mar 01, 2023 at 06:15:18PM +0900, Masahisa Kojima wrote:
> Firmware version management is not implemented in the current
> FMP implementation. This series aims to add the versioning support
> in FMP.

I think that you need to think of A/B update case, especially
when a capsule update is *reverted* to an older version.

In addition, please don't forget in the next patch set:
- update the man page of mkeficapsule command
- add test cases (in pytest)

-Takahiro Akashi

> EDK2 reference implementation utilizes the FMP Payload Header
> inserted right before the capsule payload. With this series,
> U-Boot also follows the EDK2 implementation.
> 
> Currently, there is no way to know the current running firmware
> version through the EFI interface. FMP->GetImageInfo() returns
> always 0 for the version number. So a user can not know that
> expected firmware is running after the capsule update.
> 
> With this series applied, version number can be specified
> in the capsule file generation with mkeficapsule tool, then
> user can know the running firmware version through
> FMP->GetImageInfo() and ESRT.
> 
> Note that this series does not mandate the FMP Payload Header,
> compatible with boards that are already using the existing
> U-Boot FMP implementation.
> If no FMP Payload Header is found in the capsule file, fw_version,
> lowest supported version, last attempt version and last attempt
> status is set to 0 and this is the same behavior as existing FMP
> implementation.
> 
> Changes in v2:
> - add FMP Payload Header generation in mkeficapsule tool
> 
> Masahisa Kojima (4):
>   efi_loader: store firmware version into FmpState variable
>   efi_loader: versioning support in GetImageInfo
>   efi_loader: check lowest supported version in capsule update
>   mkeficapsule: add FMP Payload Header
> 
>  lib/efi_loader/efi_firmware.c | 271 ++++++++++++++++++++++++++++++----
>  tools/mkeficapsule.c          |  81 +++++++++-
>  2 files changed, 319 insertions(+), 33 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v2 0/4] FMP versioning support

[Masahisa Kojima]

Hi Akashi-san,

On Sat, 4 Mar 2023 at 10:28, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> Kojima-san,
>
> On Wed, Mar 01, 2023 at 06:15:18PM +0900, Masahisa Kojima wrote:
> > Firmware version management is not implemented in the current
> > FMP implementation. This series aims to add the versioning support
> > in FMP.
>
> I think that you need to think of A/B update case, especially
> when a capsule update is *reverted* to an older version.

A/B update(FWU) has their own version information in the Image Directory
defined in FWU spec[1], this is not implemented yet on U-Boot.
On second thought, I think A/B update(FWU) case should be excluded
from the version management introduced by this series.

>
> In addition, please don't forget in the next patch set:
> - update the man page of mkeficapsule command
> - add test cases (in pytest)

Yes, noted.

[1] https://developer.arm.com/documentation/den0118/a/

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
> > EDK2 reference implementation utilizes the FMP Payload Header
> > inserted right before the capsule payload. With this series,
> > U-Boot also follows the EDK2 implementation.
> >
> > Currently, there is no way to know the current running firmware
> > version through the EFI interface. FMP->GetImageInfo() returns
> > always 0 for the version number. So a user can not know that
> > expected firmware is running after the capsule update.
> >
> > With this series applied, version number can be specified
> > in the capsule file generation with mkeficapsule tool, then
> > user can know the running firmware version through
> > FMP->GetImageInfo() and ESRT.
> >
> > Note that this series does not mandate the FMP Payload Header,
> > compatible with boards that are already using the existing
> > U-Boot FMP implementation.
> > If no FMP Payload Header is found in the capsule file, fw_version,
> > lowest supported version, last attempt version and last attempt
> > status is set to 0 and this is the same behavior as existing FMP
> > implementation.
> >
> > Changes in v2:
> > - add FMP Payload Header generation in mkeficapsule tool
> >
> > Masahisa Kojima (4):
> >   efi_loader: store firmware version into FmpState variable
> >   efi_loader: versioning support in GetImageInfo
> >   efi_loader: check lowest supported version in capsule update
> >   mkeficapsule: add FMP Payload Header
> >
> >  lib/efi_loader/efi_firmware.c | 271 ++++++++++++++++++++++++++++++----
> >  tools/mkeficapsule.c          |  81 +++++++++-
> >  2 files changed, 319 insertions(+), 33 deletions(-)
> >
> > --
> > 2.17.1
> >

Re: [PATCH v2 0/4] FMP versioning support

[Takahiro Akashi]

On Mon, Mar 06, 2023 at 03:08:55PM +0900, Masahisa Kojima wrote:
> Hi Akashi-san,
> 
> On Sat, 4 Mar 2023 at 10:28, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> >
> > Kojima-san,
> >
> > On Wed, Mar 01, 2023 at 06:15:18PM +0900, Masahisa Kojima wrote:
> > > Firmware version management is not implemented in the current
> > > FMP implementation. This series aims to add the versioning support
> > > in FMP.
> >
> > I think that you need to think of A/B update case, especially
> > when a capsule update is *reverted* to an older version.
> 
> A/B update(FWU) has their own version information in the Image Directory
> defined in FWU spec[1], this is not implemented yet on U-Boot.
> On second thought, I think A/B update(FWU) case should be excluded
> from the version management introduced by this series.

I don't think it's a good idea to have two different implementations
for normal case and A/B update unless there is a good reason when
we use the same FMP driver.

-Takahiro Akashi

> >
> > In addition, please don't forget in the next patch set:
> > - update the man page of mkeficapsule command
> > - add test cases (in pytest)
> 
> Yes, noted.
> 
> [1] https://developer.arm.com/documentation/den0118/a/
> 
> Thanks,
> Masahisa Kojima
> 
> >
> > -Takahiro Akashi
> >
> > > EDK2 reference implementation utilizes the FMP Payload Header
> > > inserted right before the capsule payload. With this series,
> > > U-Boot also follows the EDK2 implementation.
> > >
> > > Currently, there is no way to know the current running firmware
> > > version through the EFI interface. FMP->GetImageInfo() returns
> > > always 0 for the version number. So a user can not know that
> > > expected firmware is running after the capsule update.
> > >
> > > With this series applied, version number can be specified
> > > in the capsule file generation with mkeficapsule tool, then
> > > user can know the running firmware version through
> > > FMP->GetImageInfo() and ESRT.
> > >
> > > Note that this series does not mandate the FMP Payload Header,
> > > compatible with boards that are already using the existing
> > > U-Boot FMP implementation.
> > > If no FMP Payload Header is found in the capsule file, fw_version,
> > > lowest supported version, last attempt version and last attempt
> > > status is set to 0 and this is the same behavior as existing FMP
> > > implementation.
> > >
> > > Changes in v2:
> > > - add FMP Payload Header generation in mkeficapsule tool
> > >
> > > Masahisa Kojima (4):
> > >   efi_loader: store firmware version into FmpState variable
> > >   efi_loader: versioning support in GetImageInfo
> > >   efi_loader: check lowest supported version in capsule update
> > >   mkeficapsule: add FMP Payload Header
> > >
> > >  lib/efi_loader/efi_firmware.c | 271 ++++++++++++++++++++++++++++++----
> > >  tools/mkeficapsule.c          |  81 +++++++++-
> > >  2 files changed, 319 insertions(+), 33 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >

Re: [PATCH v2 0/4] FMP versioning support

[Masahisa Kojima]

On Mon, 6 Mar 2023 at 15:32, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> On Mon, Mar 06, 2023 at 03:08:55PM +0900, Masahisa Kojima wrote:
> > Hi Akashi-san,
> >
> > On Sat, 4 Mar 2023 at 10:28, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
> > >
> > > Kojima-san,
> > >
> > > On Wed, Mar 01, 2023 at 06:15:18PM +0900, Masahisa Kojima wrote:
> > > > Firmware version management is not implemented in the current
> > > > FMP implementation. This series aims to add the versioning support
> > > > in FMP.
> > >
> > > I think that you need to think of A/B update case, especially
> > > when a capsule update is *reverted* to an older version.
> >
> > A/B update(FWU) has their own version information in the Image Directory
> > defined in FWU spec[1], this is not implemented yet on U-Boot.
> > On second thought, I think A/B update(FWU) case should be excluded
> > from the version management introduced by this series.
>
> I don't think it's a good idea to have two different implementations
> for normal case and A/B update unless there is a good reason when
> we use the same FMP driver.

Yes, I think we need different FMP drivers for the normal case
and A/B update case if we implement the different version management.
It's just a thought that we can transfer the FWU specific code from
lib/efi_loader/efi_capsule.c to FMP driver for FWU.

Another idea is that we implement version management defined in FWU
spec and it is also used for a normal case, but I'm not sure it is feasible.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
> > >
> > > In addition, please don't forget in the next patch set:
> > > - update the man page of mkeficapsule command
> > > - add test cases (in pytest)
> >
> > Yes, noted.
> >
> > [1] https://developer.arm.com/documentation/den0118/a/
> >
> > Thanks,
> > Masahisa Kojima
> >
> > >
> > > -Takahiro Akashi
> > >
> > > > EDK2 reference implementation utilizes the FMP Payload Header
> > > > inserted right before the capsule payload. With this series,
> > > > U-Boot also follows the EDK2 implementation.
> > > >
> > > > Currently, there is no way to know the current running firmware
> > > > version through the EFI interface. FMP->GetImageInfo() returns
> > > > always 0 for the version number. So a user can not know that
> > > > expected firmware is running after the capsule update.
> > > >
> > > > With this series applied, version number can be specified
> > > > in the capsule file generation with mkeficapsule tool, then
> > > > user can know the running firmware version through
> > > > FMP->GetImageInfo() and ESRT.
> > > >
> > > > Note that this series does not mandate the FMP Payload Header,
> > > > compatible with boards that are already using the existing
> > > > U-Boot FMP implementation.
> > > > If no FMP Payload Header is found in the capsule file, fw_version,
> > > > lowest supported version, last attempt version and last attempt
> > > > status is set to 0 and this is the same behavior as existing FMP
> > > > implementation.
> > > >
> > > > Changes in v2:
> > > > - add FMP Payload Header generation in mkeficapsule tool
> > > >
> > > > Masahisa Kojima (4):
> > > >   efi_loader: store firmware version into FmpState variable
> > > >   efi_loader: versioning support in GetImageInfo
> > > >   efi_loader: check lowest supported version in capsule update
> > > >   mkeficapsule: add FMP Payload Header
> > > >
> > > >  lib/efi_loader/efi_firmware.c | 271 ++++++++++++++++++++++++++++++----
> > > >  tools/mkeficapsule.c          |  81 +++++++++-
> > > >  2 files changed, 319 insertions(+), 33 deletions(-)
> > > >
> > > > --
> > > > 2.17.1
> > > >