| Message ID | 20240529-b4-dynamic-uuid-v2-2-c26f31057bbe@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: CapsuleUpdate: support for dynamic UUIDs | expand |
Hi Caleb, [...] > > #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))) Why not just go a guidcmp()? memchr_inv() will return the first invalid match, but we don't need that > + 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; > + } > + > + compatible = ofnode_read_string(ofnode_root(), "compatible"); > + > + if (!compatible) { > + log_debug("%s: model or compatible not defined\n", __func__); > + return EFI_UNSUPPORTED; > + } > + > + if (!update_info.num_images) { > + log_debug("%s: no fw_images, make sure update_info.num_images is set\n", __func__); > + return -ENODATA; > + } [...] Cheers /Ilias
On 30/05/2024 16:56, Ilias Apalodimas wrote: > Hi Caleb, Hi Ilias, > > [...] > >> >> #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))) > > Why not just go a guidcmp()? memchr_inv() will return the first > invalid match, but we don't need that guidcmp() would require allocating a zero guid to compare to, this is just a check for any non-zero byte in the GUID so memchr_inv() seemed more fitting. I can switch to guidcmp() for readability. > >> + 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; >> + } >> + >> + compatible = ofnode_read_string(ofnode_root(), "compatible"); >> + >> + if (!compatible) { >> + log_debug("%s: model or compatible not defined\n", __func__); >> + return EFI_UNSUPPORTED; >> + } >> + >> + if (!update_info.num_images) { >> + log_debug("%s: no fw_images, make sure update_info.num_images is set\n", __func__); >> + return -ENODATA; >> + } > > [...] > > Cheers > /Ilias
On Fri, 31 May 2024 at 14:26, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 30/05/2024 16:56, Ilias Apalodimas wrote: > > Hi Caleb, > > Hi Ilias, > > > > [...] > > > >> > >> #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))) > > > > Why not just go a guidcmp()? memchr_inv() will return the first > > invalid match, but we don't need that > > guidcmp() would require allocating a zero guid to compare to, this is > just a check for any non-zero byte in the GUID so memchr_inv() seemed > more fitting. > > I can switch to guidcmp() for readability. Right, I misread what memchr_inv() does, keep it as-is it's fine Thanks /Ilias > > > >> + 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; > >> + } > >> + > >> + compatible = ofnode_read_string(ofnode_root(), "compatible"); > >> + > >> + if (!compatible) { > >> + log_debug("%s: model or compatible not defined\n", __func__); > >> + return EFI_UNSUPPORTED; > >> + } > >> + > >> + if (!update_info.num_images) { > >> + log_debug("%s: no fw_images, make sure update_info.num_images is set\n", __func__); > >> + return -ENODATA; > >> + } > > > > [...] > > > > Cheers > > /Ilias > > -- > // Caleb (they/them)
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 + 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. + + 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; + } + + compatible = ofnode_read_string(ofnode_root(), "compatible"); + + if (!compatible) { + log_debug("%s: model or compatible not defined\n", __func__); + return EFI_UNSUPPORTED; + } + + if (!update_info.num_images) { + log_debug("%s: no fw_images, make sure update_info.num_images is set\n", __func__); + 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; +} +#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);
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(+)