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 at 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);
>