Re: [PATCH v4 09/10] tools: mkeficapsule: support generating dynamic GUIDs

["Vincent Stehlé" <vincent.stehle@arm.com>]

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
>