On 05/06/2024 07:52, Heinrich Schuchardt wrote:
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.conno...@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.
Ack
+ 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.
Arguably it doesn't matter, it's just salt which is roughly UUID shaped :D
Why do we need a vendor specific name-space if we are using compatible
strings which are vendor specific themselves?
If vendor A and vendor B both build embedded products based on a
Qualcomm RB2 (for example), they might both use the same DTB but
customise other parts of U-Boot. We want them both to be able to use
LVFS to provide updates, so they need a way to avoid conflicting UUIDs.
I think requiring them to use a custom compatible might be a bit
arbitrary, especially if they aren't using any custom hardware.
+
+ 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.
Simply boot testing a production build should hit this error path... But
sure a built time check would be better. Could you suggest how I might
do this? Can we have host tools that are used during build? Where abouts
should I add this?
+
+ 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.
Ah yeah.
+ 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?
I'm not sure I understand. This is just a sanity check, though it's
actually duplicated above and this one should be dropped (it's a
holdover from a previous version of this patch).
+ 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?
Many boards have hardcoded GUIDs, which may or may not be in used. So we
can't just break them all by migrating them over. This will have to be
done incrementally by device maintainers.
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);
Kind regards,
--
// Caleb (they/them)
