Re: [RFC PATCH v3 9/9] mkeficapsule: Add support for generating empty capsules

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Sughosh, 

On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote:
> The Dependable Boot specification describes the structure of the
> firmware accept and revert capsules. These are empty capsules which
> are used for signalling the acceptance or rejection of the updated
> firmware by the OS. Add support for generating these empty capsules.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Changes since V2:
> * New patch for generating empty capsules
> 
>  tools/eficapsule.h   |   8 ++++
>  tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 100 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/eficapsule.h b/tools/eficapsule.h
> index 8c1560bb06..6001952bdc 100644
> --- a/tools/eficapsule.h
> +++ b/tools/eficapsule.h
> @@ -50,6 +50,14 @@ typedef struct {
>  	EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
>  		 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
>  
> +#define FW_ACCEPT_OS_GUID \
> +	EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \
> +		 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8)
> +
> +#define FW_REVERT_OS_GUID \
> +	EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \
> +		 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0)
> +
>  /* flags */
>  #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET      0x00010000
>  
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index 161affdd15..643da3849d 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -29,6 +29,7 @@
>  #include "eficapsule.h"
>  
>  static const char *tool_name = "mkeficapsule";
> +static unsigned char empty_capsule;
>  
>  efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
>  efi_guid_t efi_guid_image_type_uboot_fit =
> @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw =
>  efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>  
>  #ifdef CONFIG_TOOLS_LIBCRYPTO
> -static const char *opts_short = "frg:i:I:v:p:c:m:dh";
> +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR";
>  #else
> -static const char *opts_short = "frg:i:I:v:h";
> +static const char *opts_short = "frg:i:I:v:hAR";
>  #endif
>  
>  static struct option options[] = {
> @@ -55,15 +56,23 @@ static struct option options[] = {
>  	{"monotonic-count", required_argument, NULL, 'm'},
>  	{"dump-sig", no_argument, NULL, 'd'},
>  #endif
> +	{"fw-accept", no_argument, NULL, 'A'},
> +	{"fw-revert", no_argument, NULL, 'R'},
>  	{"help", no_argument, NULL, 'h'},
>  	{NULL, 0, NULL, 0},
>  };
>  
>  static void print_usage(void)
>  {
> -	fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n"
> -		"Options:\n"
> +	if (empty_capsule) {
> +		fprintf(stderr, "Usage: %s [options]  <output file>\n",
> +			tool_name);
> +	} else {
> +		fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n",
> +			tool_name);
> +	}
>  
> +	fprintf(stderr, "Options:\n"
>  		"\t-f, --fit                   FIT image type\n"
>  		"\t-r, --raw                   raw image type\n"
>  		"\t-g, --guid <guid string>    guid for image blob type\n"
> @@ -75,8 +84,9 @@ static void print_usage(void)
>  		"\t-m, --monotonic-count <count>     monotonic count\n"
>  		"\t-d, --dump_sig              dump signature (*.p7)\n"
>  #endif
> -		"\t-h, --help                  print a help message\n",
> -		tool_name);
> +	       "\t-A, --fw-accept          firmware accept capsule\n"
> +	       "\t-R, --fw-revert          firmware revert capsule\n"
> +	       "\t-h, --help                  print a help message\n");
>  }
>  
>  /**
> @@ -598,6 +608,59 @@ void convert_uuid_to_guid(unsigned char *buf)
>  	buf[7] = c;
>  }
>  
> +static int create_empty_capsule(char *path, efi_guid_t *guid, bool fw_accept)
> +{
> +	struct efi_capsule_header header;
> +	FILE *f;
> +	int ret;
> +	efi_guid_t fw_accept_guid = FW_ACCEPT_OS_GUID;
> +	efi_guid_t fw_revert_guid = FW_REVERT_OS_GUID;
> +	efi_guid_t payload, capsule_guid;
> +
> +	f = NULL;
> +	ret = -1;

Can we init those at their declaration?

> +
> +	f = fopen(path, "w");
> +	if (!f) {
> +		printf("cannot open %s\n", path);
> +		goto err;
> +	}
> +
> +	if (fw_accept)
> +		capsule_guid = fw_accept_guid;
> +	else
> +		capsule_guid = fw_revert_guid;

ternary operator would look better here.

> +
> +	memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t));
> +	header.header_size = sizeof(header);
> +	header.flags = 0;

Is it the flags only you need?  Or maybe memset the entire headeri to 0?
> +
> +	if (fw_accept) {
> +		header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t);
> +	} else {
> +		header.capsule_image_size = sizeof(header);
> +	}

ternary again?

> +
> +	if (write_capsule_file(f, &header, sizeof(header),
> +			       "Capsule header"))
> +		goto err;
> +
> +	if (fw_accept) {
> +		memcpy(&payload, guid, sizeof(efi_guid_t));
> +		if (write_capsule_file(f, &payload, sizeof(payload),
> +				       "FW Accept Capsule Payload"))
> +			goto err;
> +	}
> +
> +	ret = 0;
> +
> +err:
> +	if (f)
> +		fclose(f);
> +
> +	return ret;
> +}
> +
>  /**
>   * main - main entry function of mkeficapsule
>   * @argc:	Number of arguments
> @@ -616,6 +679,7 @@ int main(int argc, char **argv)
>  	unsigned char uuid_buf[16];
>  	unsigned long index, instance;
>  	uint64_t mcount;
> +	unsigned char accept_fw_capsule, revert_fw_capsule;
>  	char *privkey_file, *cert_file;
>  	int c, idx;
>  
> @@ -625,6 +689,8 @@ int main(int argc, char **argv)
>  	mcount = 0;
>  	privkey_file = NULL;
>  	cert_file = NULL;
> +	accept_fw_capsule = 0;
> +	revert_fw_capsule = 0;
>  	dump_sig = 0;
>  	for (;;) {
>  		c = getopt_long(argc, argv, opts_short, options, &idx);
> @@ -691,22 +757,38 @@ int main(int argc, char **argv)
>  			dump_sig = 1;
>  			break;
>  #endif /* CONFIG_TOOLS_LIBCRYPTO */
> +		case 'A':
> +			accept_fw_capsule = 1;
> +			break;
> +		case 'R':
> +			revert_fw_capsule = 1;
> +			break;
>  		case 'h':
>  			print_usage();
>  			exit(EXIT_SUCCESS);
>  		}
>  	}
>  
> +	empty_capsule = (accept_fw_capsule || revert_fw_capsule);

Why do we need 3 variables here?
Would it be better to have an enum and just use a single variable like "is_accept_capsule"?

> +
>  	/* check necessary parameters */
> -	if ((argc != optind + 2) || !guid ||
> -	    ((privkey_file && !cert_file) ||
> +	if ((!empty_capsule && argc != optind + 2) ||
> +	    (empty_capsule && argc != optind + 1) ||
> +	    (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) ||
>  	     (!privkey_file && cert_file))) {
>  		print_usage();
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance,
> -			 mcount, privkey_file, cert_file) < 0) {
> +	if (empty_capsule) {
> +		if (create_empty_capsule(argv[argc - 1], guid,
> +					 accept_fw_capsule ? 1 : 0) < 0) {
> +			printf("Creating empty capsule failed\n");
> +			exit(EXIT_FAILURE);
> +		}
> +	} else 	if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
> +				 index, instance, mcount, privkey_file,
> +				 cert_file) < 0) {
>  		fprintf(stderr, "Creating firmware capsule failed\n");
>  		exit(EXIT_FAILURE);
>  	}
> -- 
> 2.17.1
> 


Thanks
/Ilias