Re: [PATCH v4 10/11] mkeficapsule: Add support for generating empty capsules

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, Feb 09, 2022 at 12:05:06PM +0900, AKASHI Takahiro wrote:
> Hi Sughosh,
> 
> On Mon, Feb 07, 2022 at 11:50:00PM +0530, Sughosh Ganu wrote:
> > The Dependable Boot specification describes the structure of the
> 
> What is this specification? Please specify the link to the doc.
> 
> > 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 V3:
> > * Add related documentation for empty capsules in the mkeficapsule man
> >   page.
> > * Add separate usage for empty capsules, with corresponding valid
> >   options.
> > * Use ternary operators where possible.
> > * Put a exclusivity check for the empty capsule options.
> > 
> >  doc/mkeficapsule.1   |  23 +++++++-
> >  tools/eficapsule.h   |   8 +++
> >  tools/mkeficapsule.c | 131 ++++++++++++++++++++++++++++++++++++-------
> >  3 files changed, 139 insertions(+), 23 deletions(-)
> > 
> > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
> > index 8babb27ee8..75fc15906a 100644
> > --- a/doc/mkeficapsule.1
> > +++ b/doc/mkeficapsule.1
> > @@ -8,7 +8,7 @@ mkeficapsule \- Generate EFI capsule file for U-Boot
> >  
> >  .SH SYNOPSIS
> >  .B mkeficapsule
> > -.RI [ options "] " image-blob " " capsule-file
> > +.RI [ options ] " " [ image-blob ] " " capsule-file
> 
> With this formatting, "capsule-file" will get italic.

oops, I meant to say "roman."

> => .RI [ options "] [" image-blob "] " capsule-file
> 
> Right?
> 
> Furthermore, I think we can describe the command syntax of the two
> different cases (normal or empty capsule) more specifically.
> 
> >  
> >  .SH "DESCRIPTION"
> >  .B mkeficapsule
> > @@ -23,8 +23,13 @@ Optionally, a capsule file can be signed with a given private key.
> >  In this case, the update will be authenticated by verifying the signature
> >  before applying.
> >  
> > +Additionally, an empty capsule file can be generated for acceptance or
> > +rejection of firmware images by a governing component like an Operating
> > +System. The empty capsules do not require an image-blob input file.
> > +
> > +
> >  .B mkeficapsule
> > -takes any type of image files, including:
> > +takes any type of image files when generating non empty capsules, including:
> >  .TP
> >  .I raw image
> >  format is a single binary blob of any type of firmware.
> > @@ -43,7 +48,7 @@ specify a guid for the FMP driver.
> >  .SH "OPTIONS"
> >  One of
> >  .BR --fit ", " --raw " or " --guid
> > -option must be specified.
> > +option must be specified for non empty capsules.
> >  
> >  .TP
> >  .BR -f ", " --fit
> > @@ -69,6 +74,18 @@ Specify an image index
> >  .BI "-I\fR,\fB --instance " instance
> >  Specify a hardware instance
> >  
> > +.PP
> > +For generation of firmware accept empty capsule
> > +.BR --guid
> > +is mandatory
> 
> I don't still understand why we need GUID for accept empty capsule.
> We should have only one choice, whether all the new firmware be
> permanently applied or completely reverted.
> 
> That's A/B update, isn't it?
> 
> > +.TP
> > +.BI "-A\fR,\fB --fw-accept "
> > +Generate a firmware acceptance empty capsule
> > +
> > +.TP
> > +.BI "-R\fR,\fB --fw-revert "
> > +Generate a firmware revert empty capsule
> > +
> >  .TP
> >  .BR -h ", " --help
> >  Print a help message
> > 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..e5dbec3a92 100644
> > --- a/tools/mkeficapsule.c
> > +++ b/tools/mkeficapsule.c
> > @@ -29,6 +29,7 @@
> >  #include "eficapsule.h"
> >  
> >  static const char *tool_name = "mkeficapsule";
> > +unsigned char accept_fw_capsule, revert_fw_capsule, empty_capsule;
> 
> Bool? but those variables are redundant.
> 
> As Ilias suggested, introducing a new enum type here can
> simplify the code in the following code.
> enum {
>         CAPSULE_NORMAL_BLOB = 0,
>         CAPSULE_ACCEPT,
>         CAPSULE_REVERT,
> } capsule_type;
> 
> 
> >  
> >  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
> 
> Please rebase your patch to my v10 or later.
> I have already removed the dependency on openssl library.
> 
> > -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,28 +56,50 @@ 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"
> > -
> > -		"\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"
> > -		"\t-i, --index <index>         update image index\n"
> > -		"\t-I, --instance <instance>   update hardware instance\n"
> > +	if (empty_capsule) {
> > +		if (accept_fw_capsule) {
> > +			fprintf(stderr, "Usage: %s [options] <output file>\n",
> > +				tool_name);
> > +			fprintf(stderr, "Options:\n"
> > +				"\t-A, --fw-accept          firmware accept capsule\n"
> > +				"\t-g, --guid <guid string>    guid for image blob type\n"
> 
> While I doubt the necessity of "--guid,"
> why not accept "-f" or "-r" as a guid of image blob type?
> (It seems that your actual code does.)
> 
> > +				"\t-h, --help                  print a help message\n"
> > +				);
> > +		} else {
> > +			fprintf(stderr, "Usage: %s [options] <output file>\n",
> > +				tool_name);
> > +			fprintf(stderr, "Options:\n"
> > +				"\t-R, --fw-revert          firmware revert capsule\n"
> > +				"\t-h, --help                  print a help message\n"
> > +				);
> > +		}
> > +	} 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"
> > +			"\t-i, --index <index>         update image index\n"
> > +			"\t-I, --instance <instance>   update hardware instance\n"
> >  #ifdef CONFIG_TOOLS_LIBCRYPTO
> > -		"\t-p, --private-key <privkey file>  private key file\n"
> > -		"\t-c, --certificate <cert file>     signer's certificate file\n"
> > -		"\t-m, --monotonic-count <count>     monotonic count\n"
> > -		"\t-d, --dump_sig              dump signature (*.p7)\n"
> > +			"\t-p, --private-key <privkey file>  private key file\n"
> > +			"\t-c, --certificate <cert file>     signer's certificate file\n"
> > +			"\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 +621,50 @@ 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 = NULL;
> > +	int ret = -1;
> > +	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 = fopen(path, "w");
> > +	if (!f) {
> > +		fprintf(stderr, "cannot open %s\n", path);
> > +		goto err;
> > +	}
> > +
> > +	capsule_guid = fw_accept ? fw_accept_guid : fw_revert_guid;
> > +
> > +	memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t));
> > +	header.header_size = sizeof(header);
> > +	header.flags = 0;
> > +
> > +	header.capsule_image_size = fw_accept ?
> > +		sizeof(header) + sizeof(efi_guid_t) : sizeof(header);
> > +
> > +	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
> > @@ -625,6 +692,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 +760,44 @@ 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);
> >  		}
> >  	}
> >  
> > +	if (accept_fw_capsule && revert_fw_capsule) {
> > +		fprintf(stderr,
> > +			"Select either of Accept or Revert capsule generation\n");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	empty_capsule = (accept_fw_capsule || revert_fw_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))) {
> 
> Well, the error condition looks complicated due to mixing two cases
> and can be hard to maintain in the future. How about
> 	if (!empty_capsule &&
>                 ((argc != optind + 2) || !guid ||
>                  ((privkey_file && !cert_file) ||
>                   (!privkey_file && cert_file))) ||
>             empty_capsule &&
>                 ((argc != optind + 1) ||
>                  (accept_fw_capsule && revert_fw_capsule) ||
>                  (accept_fw_capsule && !guid))  # arguable as mentioned above
>                  (revert_fw_capsule && guid))
>         ...

I've got one concern here; Can we sign an empty capsule file?
I think we should.
If so, the help message (by print_usage()) doesn't reflect it.

-Takahiro Akashi

> 
> >  		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) {
> 
> The third argument can be simplified to "accept_fw_capsule".
> 
> -Takahiro Akashi
> 
> > +			fprintf(stderr, "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
> >