diff mbox series

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

Message ID 20220207182001.31270-11-sughosh.ganu@linaro.org
State New
Headers show
Series FWU: Add support for FWU Multi Bank Update feature | expand

Commit Message

Sughosh Ganu Feb. 7, 2022, 6:20 p.m. UTC
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 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(-)

Comments

AKASHI Takahiro Feb. 9, 2022, 3:05 a.m. UTC | #1
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.

=> .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))
        ...

>  		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
>
AKASHI Takahiro Feb. 10, 2022, 1:27 a.m. UTC | #2
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
> >
Sughosh Ganu Feb. 11, 2022, 1:25 p.m. UTC | #3
hi Takahiro,

On Wed, 9 Feb 2022 at 08:35, AKASHI Takahiro <takahiro.akashi@linaro.org> 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.
>
> => .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.

The syntax of the two empty capsules is described in detail in the
documentation patch. Is that not sufficient. Moreover, the usage shows
the arguments for the -A and -R options separately.

>
> >
> >  .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.

The accept capsule is accepting a given firmware image in the active
bank(active_index). The specification requires that all firmware
images be accepted separately and explicitly. With firmware revert,
the idea is that if the OS reverts even one of the images, we need to
switch to booting from a different bank. Hence no GUID specified with
the revert capsule.

>
> 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;

Okay. Actually, I forgot about Ilias's this particular review comment.
Will incorporate.

>
>
> >
> >  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.

Yes, I will have to now rebase on your V11.

>
> > -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.)

I don't get this point. The --guid is needed for the firmware accept
capsule, since the GUID value will be that of the firmware image that
is being accepted. Like I mentioned above, firmware acceptance works
on a per image basis.

>
> > +                             "\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))

Okay. Will change.

>         ...
>
> >               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".

Will change.

-sughosh

>
> -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
> >
Sughosh Ganu Feb. 11, 2022, 1:27 p.m. UTC | #4
On Thu, 10 Feb 2022 at 06:57, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> 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.

We do have plans to sign the empty capsule, although it is not being
added currently. I will keep this on my Todo list. Hope that is fine.
Thanks.

-sughosh

>
> -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
> > >
diff mbox series

Patch

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
 
 .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
+.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;
 
 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,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"
+				"\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))) {
 		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) {
+			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);
 	}