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