On Tue, Jul 02, 2024 at 03:30:49PM +0200, Caleb Connolly wrote:
Hi Caleb,
Thanks for re-spinning this series; it looks very good.
My comments below.
Best regards,
Vincent.
> Add a tool that can generate GUIDs that match those generated internally
> by U-Boot for capsule update fw_images.
Nit picking: I think you are not "adding a tool" anymore but rather, modifying
an existing one.
>
> Dynamic UUIDs in U-Boot work by taking a namespace UUID and hashing it
> with the board compatible and fw_image name.
>
> This tool accepts the same inputs and will produce the same GUID as
> U-Boot would at runtime.
Same here for "this tool".
>
> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> ---
> doc/mkeficapsule.1 | 23 ++++++++
> tools/Makefile | 3 +
> tools/mkeficapsule.c | 157
> +++++++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 178 insertions(+), 5 deletions(-)
>
> diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
> index c4c2057d5c7a..bf735295effa 100644
> --- a/doc/mkeficapsule.1
> +++ b/doc/mkeficapsule.1
> @@ -9,8 +9,11 @@ mkeficapsule \- Generate EFI capsule file for U-Boot
> .SH SYNOPSIS
> .B mkeficapsule
> .RI [ options ] " " [ image-blob ] " " capsule-file
>
> +.B mkeficapsule
> +.RI guidgen " " [ GUID ] " " DTB " " IMAGE_NAME...
> +
> .SH "DESCRIPTION"
> The
> .B mkeficapsule
> command is used to create an EFI capsule file to be used by U-Boot for
> firmware
> @@ -41,8 +44,12 @@ format is the same as used in the new uImage format and
> allows for
> multiple binary blobs in a single capsule file.
> This type of image file can be generated by
> .BR mkimage .
>
> +mkeficapsule can also be used to simulate the dynamic GUID generation used to
> +identify firmware images in capsule updates by providing the namespace guid,
> dtb
> +for the board, and a list of firmware images.
> +
> .SH "OPTIONS"
>
> .TP
> .BI "-g\fR,\fB --guid " guid-string
> @@ -112,8 +119,24 @@ at every firmware update.
> .TP
> .B "-d\fR,\fB --dump_sig"
> Dump signature data into *.p7 file
>
> +.SH "GUIDGEN OPTIONS"
> +
> +.TP
> +.B "[GUID]"
> +The namespace/salt GUID, by default this is EFI_CAPSULE_NAMESPACE_GUID.
> +The format is:
> + xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> +
> +.TP
> +.B DTB
> +The device tree blob file for the board.
> +
> +.TP
> +.B IMAGE_NAME...
> +The names of the firmware images to generate GUIDs for.
> +
> .PP
> .SH FILES
> .TP
> .I /EFI/UpdateCapsule
> diff --git a/tools/Makefile b/tools/Makefile
> index ee08a9675df8..7d1b29943471 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -253,8 +253,11 @@ mkeficapsule-objs := generated/lib/uuid.o \
> $(LIBFDT_OBJS) \
> mkeficapsule.o
> hostprogs-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
>
> +genguid-objs := generated/lib/uuid.o generated/lib/sha1.o genguid.o
> +hostprogs-$(CONFIG_TOOLS_GENGUID) += genguid
> +
As far as I can tell those lines above should be removed, now that you have
integrated genguid into mkeficapsule.
> mkfwumdata-objs := mkfwumdata.o generated/lib/crc32.o
> HOSTLDLIBS_mkfwumdata += -luuid
> hostprogs-$(CONFIG_TOOLS_MKFWUMDATA) += mkfwumdata
>
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index 54fb4dee3ee5..593380e4236a 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -19,12 +19,16 @@
> #include <gnutls/gnutls.h>
> #include <gnutls/pkcs7.h>
> #include <gnutls/abstract.h>
>
> +#include <libfdt.h>
> #include <u-boot/uuid.h>
>
> #include "eficapsule.h"
>
> +// Matches CONFIG_EFI_CAPSULE_NAMESPACE_GUID
> +#define DEFAULT_NAMESPACE_GUID "8c9f137e-91dc-427b-b2d6-b420faebaf2a"
> +
> 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;
> @@ -38,8 +42,9 @@ enum {
> } capsule_type;
>
> static struct option options[] = {
> {"guid", required_argument, NULL, 'g'},
> + {"dtb", required_argument, NULL, 'd'},
I think this option entry should be removed. This seems unused and the existing
`-d' option of mkeficapsule means something completely unrelated ("dump
signature").
> {"index", required_argument, NULL, 'i'},
> {"instance", required_argument, NULL, 'I'},
> {"fw-version", required_argument, NULL, 'v'},
> {"private-key", required_argument, NULL, 'p'},
> @@ -53,11 +58,23 @@ static struct option options[] = {
> {"help", no_argument, NULL, 'h'},
> {NULL, 0, NULL, 0},
> };
>
> -static void print_usage(void)
> +
> +static void print_usage_guidgen(void)
> {
> - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n"
> + fprintf(stderr, "%s guidgen [GUID] DTB IMAGE_NAME...\n"
> + "Options:\n"
> +
> + "\tGUID Namespace GUID (default: %s)\n"
> + "\tDTB Device Tree Blob\n"
> + "\tIMAGE_NAME... One or more names of fw_images
> to generate GUIDs for\n",
> + tool_name, DEFAULT_NAMESPACE_GUID);
> +}
> +
> +static void print_usage_mkeficapsule(void)
> +{
> + fprintf(stderr, "Usage: \n\n%s [options] <image blob> <output file>\n"
> "Options:\n"
>
> "\t-g, --guid <guid string> guid for image blob type\n"
> "\t-i, --index <index> update image index\n"
> @@ -70,10 +87,11 @@ static void print_usage(void)
> "\t-A, --fw-accept firmware accept capsule, requires GUID, no
> image blob\n"
> "\t-R, --fw-revert firmware revert capsule, takes no GUID, no
> image blob\n"
> "\t-o, --capoemflag Capsule OEM Flag, an integer between 0x0000
> and 0xffff\n"
> "\t-D, --dump-capsule dump the contents of the capsule
> headers\n"
> - "\t-h, --help print a help message\n",
> + "\t-h, --help print a help message\n\n",
> tool_name);
> + print_usage_guidgen();
> }
>
> /**
> * auth_context - authentication context
> @@ -816,8 +834,130 @@ static void dump_capsule_contents(char *capsule_file)
> exit(EXIT_FAILURE);
> }
> }
>
> +static struct fdt_header *load_dtb(const char *path)
> +{
> + struct fdt_header *dtb;
> + ssize_t dtb_size;
> + FILE *f;
> +
> + /* Open and parse DTB */
> + f = fopen(path, "r");
> + if (!f) {
> + fprintf(stderr, "Cannot open %s\n", path);
> + return NULL;
> + }
> +
> + if (fseek(f, 0, SEEK_END)) {
> + fprintf(stderr, "Cannot seek to the end of %s: %s\n",
> + path, strerror(errno));
> + return NULL;
> + }
> +
> + dtb_size = ftell(f);
> + if (dtb_size < 0) {
> + fprintf(stderr, "Cannot ftell %s: %s\n",
> + path, strerror(errno));
> + return NULL;
> + }
> +
> + fseek(f, 0, SEEK_SET);
> +
> + dtb = malloc(dtb_size);
> + if (!dtb) {
> + fprintf(stderr, "Can't allocated %ld\n", dtb_size);
> + return NULL;
> + }
> +
> + if (fread(dtb, dtb_size, 1, f) != 1) {
> + fprintf(stderr, "Can't read %ld bytes from %s\n",
> + dtb_size, path);
> + free(dtb);
> + return NULL;
> + }
> +
> + fclose(f);
> +
> + return dtb;
> +}
> +
> +#define MAX_IMAGE_NAME_LEN 128
> +static int genguid(int argc, char **argv)
> +{
> + int idx = 2, ret;
> + unsigned char namespace[16];
> + struct efi_guid image_type_id;
> + const char *dtb_path;
> + struct fdt_header *dtb;
> + const char *compatible;
> + int compatlen, namelen;
> + uint16_t fw_image[MAX_IMAGE_NAME_LEN];
> +
> + if (argc < 2) {
> + fprintf(stderr, "Usage: ");
> + print_usage_guidgen();
> + return -1;
> + }
> +
> + if (uuid_str_to_bin(argv[1], namespace, UUID_STR_FORMAT_GUID)) {
> + uuid_str_to_bin(DEFAULT_NAMESPACE_GUID, namespace,
> UUID_STR_FORMAT_GUID);
> + dtb_path = argv[1];
> + } else {
> + dtb_path = argv[2];
> + idx = 3;
> + }
> +
> + if (idx == argc) {
> + fprintf(stderr, "Usage: ");
> + print_usage_guidgen();
> + return -1;
> + }
> +
> + dtb = load_dtb(dtb_path);
> + if (!dtb)
> + return -1;
> +
> + if ((ret = fdt_check_header(dtb))) {
> + fprintf(stderr, "Invalid DTB header: %d\n", ret);
> + return -1;
> + }
> +
> + compatible = fdt_getprop(dtb, 0, "compatible", &compatlen);
> + if (!compatible) {
> + fprintf(stderr, "No compatible string found in DTB\n");
> + return -1;
> + }
> + if (strnlen(compatible, compatlen) >= compatlen) {
> + fprintf(stderr, "Compatible string not null-terminated\n");
> + return -1;
> + }
> +
> + printf("Generating GUIDs for %s with namespace %s:\n",
> + compatible, DEFAULT_NAMESPACE_GUID);
> + for (; idx < argc; idx++) {
> + memset(fw_image, 0, sizeof(fw_image));
> + namelen = strlen(argv[idx]);
> + if (namelen > MAX_IMAGE_NAME_LEN) {
> + fprintf(stderr, "Image name too long: %s\n", argv[idx]);
> + return -1;
> + }
> +
> + for (int i = 0; i < namelen; i++)
> + fw_image[i] = (uint16_t)argv[idx][i];
> +
> + gen_v5_guid((struct uuid *)&namespace, &image_type_id,
> + compatible, strlen(compatible),
> + fw_image, namelen * sizeof(uint16_t),
> + NULL);
> +
> + printf("%s: ", argv[idx]);
> + print_guid(&image_type_id);
> + }
> +
> + return 0;
> +}
> +
> /**
> * main - main entry function of mkeficapsule
> * @argc: Number of arguments
> * @argv: Array of pointers to arguments
> @@ -840,8 +980,15 @@ int main(int argc, char **argv)
> char *privkey_file, *cert_file;
> int c, idx;
> struct fmp_payload_header_params fmp_ph_params = { 0 };
>
> + /* Generate dynamic GUIDs */
> + if (argc > 1 && !strcmp(argv[1], "guidgen")) {
> + if (genguid(argc - 1, argv + 1))
> + exit(EXIT_FAILURE);
> + exit(EXIT_SUCCESS);
> + }
> +
> guid = NULL;
> index = 0;
> instance = 0;
> mcount = 0;
> @@ -928,9 +1075,9 @@ int main(int argc, char **argv)
> case 'D':
> capsule_dump = true;
> break;
> default:
> - print_usage();
> + print_usage_mkeficapsule();
> exit(EXIT_SUCCESS);
> }
> }
>
> @@ -951,9 +1098,9 @@ int main(int argc, char **argv)
> (capsule_type != CAPSULE_NORMAL_BLOB &&
> ((argc != optind + 1) ||
> ((capsule_type == CAPSULE_ACCEPT) && !guid) ||
> ((capsule_type == CAPSULE_REVERT) && guid)))) {
> - print_usage();
> + print_usage_mkeficapsule();
> exit(EXIT_FAILURE);
> }
>
> if (capsule_type != CAPSULE_NORMAL_BLOB) {
>
> --
> 2.45.2
>
