Re: [PATCH v3 2/7] efi: add a helper to generate dynamic UUIDs

On 5/31/24 15:50, Caleb Connolly wrote:

Introduce a new helper efi_capsule_update_info_gen_ids() which populates the capsule update fw images image_type_id field. This allows for determinstic UUIDs to be used that can scale to a large number of different boards and board variants without the need to maintain a big list.

We call this from efi_fill_image_desc_array() to populate the UUIDs lazily on-demand.

This is behind an additional config option as it depends on V5 UUIDs and the SHA1 implementation.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

lib/efi_loader/Kconfig | 23 +++++++++++++++ lib/efi_loader/efi_capsule.c | 1 + lib/efi_loader/efi_firmware.c | 66 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index 430bb7f0f7dc..e90caf4f8e14 100644 --- a/lib/efi_loader/Kconfig +++ b/lib/efi_loader/Kconfig @@ -235,8 +235,31 @@ config EFI_CAPSULE_ON_DISK_EARLY If this option is enabled, capsules will be enforced to be executed as part of U-Boot initialisation so that they will surely take place whatever is set to distro_bootcmd.

+config EFI_CAPSULE_DYNAMIC_UUIDS

• bool "Dynamic UUIDs for capsules"
• depends on EFI_HAVE_CAPSULE_SUPPORT
• select UUID_GEN_V5

This select will create a build error if CONFIG_SHA1=n as CONFIG_UUID_GEN_V5 depends on it.

• help

•  Select this option if you want to use dynamically generated v5
  

•  UUIDs for your board. To make use of this feature, your board
  

•  code should call efi_capsule_update_info_gen_ids() with a seed
  

•  UUID to generate the image_type_id field for each fw_image.
  

•  The CapsuleUpdate payloads are expected to generate matching UUIDs
  

•  using the same scheme.
  

+config EFI_CAPSULE_NAMESPACE_UUID

• string "Namespace UUID for dynamic UUIDs"
• depends on EFI_CAPSULE_DYNAMIC_UUIDS
• help

•  Define the namespace or "salt" UUID used to generate the per-image
  

•  UUIDs. This should be a UUID in the standard 8-4-4-4-12 format.
  

As UUIDs can be formatted low-endian or big-endian I would not know how the value will be interpreted.

Why do we need a vendor specific name-space if we are using compatible strings which are vendor specific themselves?

•  Device vendors are expected to generate their own namespace UUID
  

•  to avoid conflicts with existing products.
  

• config EFI_CAPSULE_FIRMWARE bool

  config EFI_CAPSULE_FIRMWARE_MANAGEMENT

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 0937800e588f..ac02e79ae7d8 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -19,8 +19,9 @@ #include <mapmem.h> #include <sort.h> #include <sysreset.h> #include <asm/global_data.h> +#include <uuid.h>

#include <crypto/pkcs7.h> #include <crypto/pkcs7_parser.h> #include <linux/err.h> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c index ba5aba098c0f..a8dafe4f01a5 100644 --- a/lib/efi_loader/efi_firmware.c +++ b/lib/efi_loader/efi_firmware.c @@ -244,8 +244,71 @@ void efi_firmware_fill_version_info(struct efi_firmware_image_descriptor *image_

free(var_state); }

+#if CONFIG_IS_ENABLED(EFI_CAPSULE_DYNAMIC_UUIDS) +/**

• • efi_capsule_update_info_gen_ids - generate GUIDs for the images
• • Generate the image_type_id for each image in the update_info.images array
• • using the first compatible from the device tree and a salt
• • UUID defined at build time.
• • Returns: status code
• */

+static efi_status_t efi_capsule_update_info_gen_ids(void) +{

• int ret, i;
• struct uuid namespace;
• const char *compatible; /* Full array including null bytes */
• struct efi_fw_image *fw_array;
• fw_array = update_info.images;
• /* Check if we need to run (there are images and we didn't already generate their IDs) */
• if (!update_info.num_images ||

•    memchr_inv(&fw_array[0].image_type_id, 0, sizeof(fw_array[0].image_type_id)))
  

• return EFI_SUCCESS;
  

• ret = uuid_str_to_bin(CONFIG_EFI_CAPSULE_NAMESPACE_UUID,

• 	(unsigned char *)&namespace, UUID_STR_FORMAT_GUID);
  

• if (ret) {

• log_debug("%s: CONFIG_EFI_CAPSULE_NAMESPACE_UUID is invalid: %d\n", __func__, ret);
  

• return EFI_UNSUPPORTED;
  

• }

You don't want end-users to be the first to find this issue. This must be a build time check.

• compatible = ofnode_read_string(ofnode_root(), "compatible");
• if (!compatible) {

• log_debug("%s: model or compatible not defined\n", __func__);
  

You are not reading model here.

• return EFI_UNSUPPORTED;
  

• }
• if (!update_info.num_images) {

• log_debug("%s: no fw_images, make sure update_info.num_images is set\n", __func__);
  

I thought we were using capsules without images in A/B updates and need to process them?

• return -ENODATA;
  

• }
• for (i = 0; i < update_info.num_images; i++) {

• gen_uuid_v5(&namespace,
  

• 	    (struct uuid *)&fw_array[i].image_type_id,
  

• 	    compatible, strlen(compatible),
  

• 	    fw_array[i].fw_name, u16_strsize(fw_array[i].fw_name)
  

• 		- sizeof(uint16_t),
  

• 	    NULL);
  

• log_debug("Image %ls UUID %pUs\n", fw_array[i].fw_name,
  

• 	  &fw_array[i].image_type_id);
  

• }
• return EFI_SUCCESS;

+} +#else +static efi_status_t efi_capsule_update_info_gen_ids(void) +{

• return EFI_SUCCESS;

Why should we have a case were we don't generate the image UUIDs? Don't we get rid of capsule GUIDs in the device-trees?

Best regards

Heinrich

+} +#endif

• /**
  • efi_fill_image_desc_array - populate image descriptor array
  • @image_info_size: Size of @image_info
  • @image_info: Image information

@@ -282,8 +345,11 @@ static efi_status_t efi_fill_image_desc_array( return EFI_BUFFER_TOO_SMALL; } *image_info_size = total_size;

• if (efi_capsule_update_info_gen_ids() != EFI_SUCCESS)

• return EFI_UNSUPPORTED;
  

• fw_array = update_info.images; *descriptor_count = update_info.num_images; *descriptor_version = EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION; *descriptor_size = sizeof(*image_info);