| Message ID | 20220609123010.1017463-12-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
On 6/9/22 14:29, Sughosh Ganu wrote: > The Dependable Boot specification[1] 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. > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > doc/mkeficapsule.1 | 29 ++++++--- > tools/eficapsule.h | 8 +++ > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > 3 files changed, 151 insertions(+), 25 deletions(-) > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > index 09bdc24295..77ca061efd 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. > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > This type of image file can be generated by > .BR mkimage . > > -.PP > -If you want to use other types than above two, you should explicitly > -specify a guid for the FMP driver. > - > .SH "OPTIONS" > + > .TP > .BI "-g\fR,\fB --guid " guid-string > Specify guid for image blob type. The format is: > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > The first three elements are in little endian, while the rest > -is in big endian. > +is in big endian. The option must be specified for all non empty and > +image acceptance capsules > > .TP > .BI "-i\fR,\fB --index " index > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > --- a/tools/eficapsule.h > +++ b/tools/eficapsule.h > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > + > +static bool empty_capsule; > +static unsigned char capsule; > + > +enum { > + CAPSULE_NORMAL_BLOB = 0, > + CAPSULE_ACCEPT, > + CAPSULE_REVERT, > +} capsule_type; > > static struct option options[] = { > {"guid", required_argument, NULL, 'g'}, > @@ -39,24 +48,47 @@ static struct option options[] = { > {"certificate", required_argument, NULL, 'c'}, > {"monotonic-count", required_argument, NULL, 'm'}, > {"dump-sig", no_argument, NULL, 'd'}, > + {"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-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" > - "\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-h, --help print a help message\n", > - tool_name); > + if (empty_capsule) { > + if (capsule == CAPSULE_ACCEPT) { > + fprintf(stderr, "Usage: %s [options] <output file>\n", My expectation is that this function always provides the same output. If different scenarios allow only specific combinations of arguments you may describe it here. > + 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" > + "Options:\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" > + "\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-A, --fw-accept firmware accept capsule\n" "\t-A, --fw-accept firmware accept capsule, requires GUID\n" > + "\t-R, --fw-revert firmware revert capsule\n" "\t-R, --fw-revert firmware revert capsule, takes no GUID\n" > + "\t-h, --help print a help message\n", > + tool_name); > + } > } > > /** > @@ -564,6 +596,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 > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > case 'd': > dump_sig = 1; > break; > + case 'A': > + capsule |= CAPSULE_ACCEPT; > + break; > + case 'R': > + capsule |= CAPSULE_REVERT; > + break; > case 'h': You must handle '?' and ':' here too. Just use default: > print_usage(); > exit(EXIT_SUCCESS); > } > } > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > + fprintf(stderr, > + "Select either of Accept or Revert capsule generation\n"); > + exit(EXIT_FAILURE); > + } > + > + empty_capsule = (capsule == CAPSULE_ACCEPT || > + capsule == CAPSULE_REVERT); > + > /* check necessary parameters */ > - if ((argc != optind + 2) || !guid || > - ((privkey_file && !cert_file) || > - (!privkey_file && cert_file))) { > + if ((!empty_capsule && > + ((argc != optind + 2) || !guid || > + ((privkey_file && !cert_file) || > + (!privkey_file && cert_file)))) || > + (empty_capsule && > + ((argc != optind + 1) || > + ((capsule == CAPSULE_ACCEPT) && !guid) || > + ((capsule == CAPSULE_REVERT) && guid)))) { Please, write a message explaining to the user what was wrong, e.g. "Option -A does not take a GUID" or "Option -R requires a 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, > + capsule == CAPSULE_ACCEPT) < 0) { > + fprintf(stderr, "Creating empty capsule failed\n"); The called function should provide a message that describes what went wrong. Best regards Heinrich > + 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); > }
On Thu, 9 Jun 2022 at 21:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 6/9/22 14:29, Sughosh Ganu wrote: > > The Dependable Boot specification[1] 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. > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > doc/mkeficapsule.1 | 29 ++++++--- > > tools/eficapsule.h | 8 +++ > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > 3 files changed, 151 insertions(+), 25 deletions(-) <snip> > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > > index 5f74d23b9e..e8eb6b070d 100644 > > --- a/tools/mkeficapsule.c > > +++ b/tools/mkeficapsule.c > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > + > > +static bool empty_capsule; > > +static unsigned char capsule; > > + > > +enum { > > + CAPSULE_NORMAL_BLOB = 0, > > + CAPSULE_ACCEPT, > > + CAPSULE_REVERT, > > +} capsule_type; > > > > static struct option options[] = { > > {"guid", required_argument, NULL, 'g'}, > > @@ -39,24 +48,47 @@ static struct option options[] = { > > {"certificate", required_argument, NULL, 'c'}, > > {"monotonic-count", required_argument, NULL, 'm'}, > > {"dump-sig", no_argument, NULL, 'd'}, > > + {"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-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" > > - "\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-h, --help print a help message\n", > > - tool_name); > > + if (empty_capsule) { > > + if (capsule == CAPSULE_ACCEPT) { > > + fprintf(stderr, "Usage: %s [options] <output file>\n", > > My expectation is that this function always provides the same output. > > If different scenarios allow only specific combinations of arguments you > may describe it here. Okay > > > > + 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" > > + "Options:\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" > > + "\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-A, --fw-accept firmware accept capsule\n" > > "\t-A, --fw-accept firmware accept capsule, requires GUID\n" > > > + "\t-R, --fw-revert firmware revert capsule\n" > > "\t-R, --fw-revert firmware revert capsule, takes no GUID\n" > > > + "\t-h, --help print a help message\n", > > + tool_name); > > + } > > } > > > > /** > > @@ -564,6 +596,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 > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > > case 'd': > > dump_sig = 1; > > break; > > + case 'A': > > + capsule |= CAPSULE_ACCEPT; > > + break; > > + case 'R': > > + capsule |= CAPSULE_REVERT; > > + break; > > case 'h': > > You must handle '?' and ':' here too. Just use > > default: Okay > > > > print_usage(); > > exit(EXIT_SUCCESS); > > } > > } > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > + fprintf(stderr, > > + "Select either of Accept or Revert capsule generation\n"); > > + exit(EXIT_FAILURE); > > + } > > + > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > + capsule == CAPSULE_REVERT); > > + > > /* check necessary parameters */ > > - if ((argc != optind + 2) || !guid || > > - ((privkey_file && !cert_file) || > > - (!privkey_file && cert_file))) { > > + if ((!empty_capsule && > > + ((argc != optind + 2) || !guid || > > + ((privkey_file && !cert_file) || > > + (!privkey_file && cert_file)))) || > > + (empty_capsule && > > + ((argc != optind + 1) || > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > + ((capsule == CAPSULE_REVERT) && guid)))) { > > Please, write a message explaining to the user what was wrong, e.g. > > "Option -A does not take a GUID" or > "Option -R requires a GUID" We are calling the print_usage function where I am adding the additional information that you suggested above. I think that would suffice. Else we are basically repeating the same information twice. > > > > 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, > > + capsule == CAPSULE_ACCEPT) < 0) { > > + fprintf(stderr, "Creating empty capsule failed\n"); > > The called function should provide a message that describes what went wrong. The create_empty_capsule function calls the write_capsule_file function for creation of the capsule file, and passes the string that is to be printed in case of an error condition. -sughosh > > Best regards > > Heinrich > > > > + 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); > > } >
On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > The Dependable Boot specification[1] 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. > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > doc/mkeficapsule.1 | 29 ++++++--- > tools/eficapsule.h | 8 +++ > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > 3 files changed, 151 insertions(+), 25 deletions(-) > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > index 09bdc24295..77ca061efd 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. > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > This type of image file can be generated by > .BR mkimage . > > -.PP > -If you want to use other types than above two, you should explicitly > -specify a guid for the FMP driver. > - > .SH "OPTIONS" > + > .TP > .BI "-g\fR,\fB --guid " guid-string > Specify guid for image blob type. The format is: > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > The first three elements are in little endian, while the rest > -is in big endian. > +is in big endian. The option must be specified for all non empty and > +image acceptance capsules "image acceptance" -> "firmware acceptance" I don't still understand why we need a guid for acceptance while revert doesn't require it. I believe that firmware update is "all or nothing", isn't it? If there is a good reason, please describe a possible/expected scenario. > .TP > .BI "-i\fR,\fB --index " index > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > --- a/tools/eficapsule.h > +++ b/tools/eficapsule.h > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > + > +static bool empty_capsule; > +static unsigned char capsule; > + > +enum { > + CAPSULE_NORMAL_BLOB = 0, > + CAPSULE_ACCEPT, > + CAPSULE_REVERT, > +} capsule_type; > > static struct option options[] = { > {"guid", required_argument, NULL, 'g'}, > @@ -39,24 +48,47 @@ static struct option options[] = { > {"certificate", required_argument, NULL, 'c'}, > {"monotonic-count", required_argument, NULL, 'm'}, > {"dump-sig", no_argument, NULL, 'd'}, > + {"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-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" > - "\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-h, --help print a help message\n", > - tool_name); > + if (empty_capsule) { > + if (capsule == CAPSULE_ACCEPT) { > + 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" > + "Options:\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" > + "\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-A, --fw-accept firmware accept capsule\n" > + "\t-R, --fw-revert firmware revert capsule\n" > + "\t-h, --help print a help message\n", > + tool_name); > + } > } > > /** > @@ -564,6 +596,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)); -> guidcpy() > + 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)); ditto > + 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 > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > case 'd': > dump_sig = 1; > break; > + case 'A': > + capsule |= CAPSULE_ACCEPT; > + break; > + case 'R': > + capsule |= CAPSULE_REVERT; > + break; > case 'h': > print_usage(); > exit(EXIT_SUCCESS); > } > } > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > + fprintf(stderr, > + "Select either of Accept or Revert capsule generation\n"); > + exit(EXIT_FAILURE); > + } > + > + empty_capsule = (capsule == CAPSULE_ACCEPT || > + capsule == CAPSULE_REVERT); > + So empty_capsule is redundant as empty_capsule is equivalent with "capsule == CAPSULE_NORMAL_BLOB". I think that a single variable, say capsule_type, is enough. > /* check necessary parameters */ > - if ((argc != optind + 2) || !guid || > - ((privkey_file && !cert_file) || > - (!privkey_file && cert_file))) { > + if ((!empty_capsule && > + ((argc != optind + 2) || !guid || > + ((privkey_file && !cert_file) || > + (!privkey_file && cert_file)))) || > + (empty_capsule && > + ((argc != optind + 1) || > + ((capsule == CAPSULE_ACCEPT) && !guid) || > + ((capsule == CAPSULE_REVERT) && 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, > + capsule == CAPSULE_ACCEPT) < 0) { if (capsule_type != CAPSULE_NORMAL_BLOB) create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); Simple is the best :) -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.25.1 >
On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > > The Dependable Boot specification[1] 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. > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > doc/mkeficapsule.1 | 29 ++++++--- > > tools/eficapsule.h | 8 +++ > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > 3 files changed, 151 insertions(+), 25 deletions(-) > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > > index 09bdc24295..77ca061efd 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. > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > > This type of image file can be generated by > > .BR mkimage . > > > > -.PP > > -If you want to use other types than above two, you should explicitly > > -specify a guid for the FMP driver. > > - > > .SH "OPTIONS" > > + > > .TP > > .BI "-g\fR,\fB --guid " guid-string > > Specify guid for image blob type. The format is: > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > > > The first three elements are in little endian, while the rest > > -is in big endian. > > +is in big endian. The option must be specified for all non empty and > > +image acceptance capsules > > "image acceptance" -> "firmware acceptance" Okay > > I don't still understand why we need a guid for acceptance > while revert doesn't require it. > I believe that firmware update is "all or nothing", isn't it? I believe this gives more flexibility in that different components might be required to accept the various firmware images. So, one component might accept the optee_os, while another might be responsible for accepting u-boot. In any case, we do check that all the components have their accepted bit set, and only if so, does the bank boot in the regular state. In case of a firmware revert, it would not matter which firmware component is being reverted -- the platform would simply need to boot from the other bank. Do you see any issue with the current method that we have? > > If there is a good reason, please describe a possible/expected > scenario. Where do you want me to explain this, in the feature documentation? Or do you think this can be elaborated in greater detail in the spec. > > > .TP > > .BI "-i\fR,\fB --index " index > > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > > --- a/tools/eficapsule.h > > +++ b/tools/eficapsule.h > > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > > --- a/tools/mkeficapsule.c > > +++ b/tools/mkeficapsule.c > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > + > > +static bool empty_capsule; > > +static unsigned char capsule; > > + > > +enum { > > + CAPSULE_NORMAL_BLOB = 0, > > + CAPSULE_ACCEPT, > > + CAPSULE_REVERT, > > +} capsule_type; > > > > static struct option options[] = { > > {"guid", required_argument, NULL, 'g'}, > > @@ -39,24 +48,47 @@ static struct option options[] = { > > {"certificate", required_argument, NULL, 'c'}, > > {"monotonic-count", required_argument, NULL, 'm'}, > > {"dump-sig", no_argument, NULL, 'd'}, > > + {"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-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" > > - "\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-h, --help print a help message\n", > > - tool_name); > > + if (empty_capsule) { > > + if (capsule == CAPSULE_ACCEPT) { > > + 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" > > + "Options:\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" > > + "\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-A, --fw-accept firmware accept capsule\n" > > + "\t-R, --fw-revert firmware revert capsule\n" > > + "\t-h, --help print a help message\n", > > + tool_name); > > + } > > } > > > > /** > > @@ -564,6 +596,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)); > > -> guidcpy() This being a host tool, guidcpy cannot be used. You have used memcpy in the create_fwbin() for the same reason I guess. > > > + 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)); > > ditto Same as above. > > > + 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 > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > case 'd': > > dump_sig = 1; > > break; > > + case 'A': > > + capsule |= CAPSULE_ACCEPT; > > + break; > > + case 'R': > > + capsule |= CAPSULE_REVERT; > > + break; > > case 'h': > > print_usage(); > > exit(EXIT_SUCCESS); > > } > > } > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > + fprintf(stderr, > > + "Select either of Accept or Revert capsule generation\n"); > > + exit(EXIT_FAILURE); > > + } > > + > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > + capsule == CAPSULE_REVERT); > > + > > So empty_capsule is redundant as empty_capsule is equivalent with > "capsule == CAPSULE_NORMAL_BLOB". > I think that a single variable, say capsule_type, is enough. I was using empty_capsule primarily to make the check done below look more succinct and readable. But I can change that to capsule != CAPSULE_NORMAL_BLOB. > > > /* check necessary parameters */ > > - if ((argc != optind + 2) || !guid || > > - ((privkey_file && !cert_file) || > > - (!privkey_file && cert_file))) { > > + if ((!empty_capsule && > > + ((argc != optind + 2) || !guid || > > + ((privkey_file && !cert_file) || > > + (!privkey_file && cert_file)))) || > > + (empty_capsule && > > + ((argc != optind + 1) || > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > + ((capsule == CAPSULE_REVERT) && 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, > > + capsule == CAPSULE_ACCEPT) < 0) { > > if (capsule_type != CAPSULE_NORMAL_BLOB) > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); > > Simple is the best :) Common, please don't tell me that the code above is complicated. Just that we can do without the empty_capsule variable, yes. -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.25.1 > >
Sughosh, On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote: > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi > <takahiro.akashi@linaro.org> wrote: > > > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > > > The Dependable Boot specification[1] 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. > > > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > doc/mkeficapsule.1 | 29 ++++++--- > > > tools/eficapsule.h | 8 +++ > > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > > 3 files changed, 151 insertions(+), 25 deletions(-) > > > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > > > index 09bdc24295..77ca061efd 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. > > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > > > This type of image file can be generated by > > > .BR mkimage . > > > > > > -.PP > > > -If you want to use other types than above two, you should explicitly > > > -specify a guid for the FMP driver. > > > - > > > .SH "OPTIONS" > > > + > > > .TP > > > .BI "-g\fR,\fB --guid " guid-string > > > Specify guid for image blob type. The format is: > > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > > > > > The first three elements are in little endian, while the rest > > > -is in big endian. > > > +is in big endian. The option must be specified for all non empty and > > > +image acceptance capsules > > > > "image acceptance" -> "firmware acceptance" > > Okay > > > > > I don't still understand why we need a guid for acceptance > > while revert doesn't require it. > > I believe that firmware update is "all or nothing", isn't it? > > I believe this gives more flexibility in that different components > might be required to accept the various firmware images. So, one > component might accept the optee_os, while another might be > responsible for accepting u-boot. In any case, we do check that all > the components have their accepted bit set, and only if so, does the > bank boot in the regular state. Probably I don't understand the behavior. Let's assume that we have firmware A and firmware B and then update both. When the firmware A is accepted and B is not (not yet issuing acceptance capsule) and I try to reboot the system, what happens? From which bank does the system boot, old one or new one? > In case of a firmware revert, it would > not matter which firmware component is being reverted -- the platform > would simply need to boot from the other bank. Do you see any issue > with the current method that we have? > > > > > If there is a good reason, please describe a possible/expected > > scenario. > > Where do you want me to explain this, in the feature documentation? Or > do you think this can be elaborated in greater detail in the spec. I prefer some explanation in U-Boot doc. > > > > > .TP > > > .BI "-i\fR,\fB --index " index > > > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > > > --- a/tools/eficapsule.h > > > +++ b/tools/eficapsule.h > > > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > > > --- a/tools/mkeficapsule.c > > > +++ b/tools/mkeficapsule.c > > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > > + > > > +static bool empty_capsule; > > > +static unsigned char capsule; > > > + > > > +enum { > > > + CAPSULE_NORMAL_BLOB = 0, > > > + CAPSULE_ACCEPT, > > > + CAPSULE_REVERT, > > > +} capsule_type; > > > > > > static struct option options[] = { > > > {"guid", required_argument, NULL, 'g'}, > > > @@ -39,24 +48,47 @@ static struct option options[] = { > > > {"certificate", required_argument, NULL, 'c'}, > > > {"monotonic-count", required_argument, NULL, 'm'}, > > > {"dump-sig", no_argument, NULL, 'd'}, > > > + {"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-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" > > > - "\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-h, --help print a help message\n", > > > - tool_name); > > > + if (empty_capsule) { > > > + if (capsule == CAPSULE_ACCEPT) { > > > + 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" > > > + "Options:\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" > > > + "\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-A, --fw-accept firmware accept capsule\n" > > > + "\t-R, --fw-revert firmware revert capsule\n" > > > + "\t-h, --help print a help message\n", > > > + tool_name); > > > + } > > > } > > > > > > /** > > > @@ -564,6 +596,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)); > > > > -> guidcpy() > > This being a host tool, guidcpy cannot be used. You have used memcpy > in the create_fwbin() for the same reason I guess. Ah, right. > > > > > + 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)); > > > > ditto > > Same as above. > > > > > > + 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 > > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > > case 'd': > > > dump_sig = 1; > > > break; > > > + case 'A': > > > + capsule |= CAPSULE_ACCEPT; > > > + break; > > > + case 'R': > > > + capsule |= CAPSULE_REVERT; > > > + break; > > > case 'h': > > > print_usage(); > > > exit(EXIT_SUCCESS); > > > } > > > } > > > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > > + fprintf(stderr, > > > + "Select either of Accept or Revert capsule generation\n"); > > > + exit(EXIT_FAILURE); > > > + } > > > + > > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > > + capsule == CAPSULE_REVERT); > > > + > > > > So empty_capsule is redundant as empty_capsule is equivalent with > > "capsule == CAPSULE_NORMAL_BLOB". > > I think that a single variable, say capsule_type, is enough. > > I was using empty_capsule primarily to make the check done below look > more succinct and readable. But I can change that to capsule != > CAPSULE_NORMAL_BLOB. > > > > > > /* check necessary parameters */ > > > - if ((argc != optind + 2) || !guid || > > > - ((privkey_file && !cert_file) || > > > - (!privkey_file && cert_file))) { > > > + if ((!empty_capsule && > > > + ((argc != optind + 2) || !guid || > > > + ((privkey_file && !cert_file) || > > > + (!privkey_file && cert_file)))) || > > > + (empty_capsule && > > > + ((argc != optind + 1) || > > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > > + ((capsule == CAPSULE_REVERT) && 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, > > > + capsule == CAPSULE_ACCEPT) < 0) { > > > > if (capsule_type != CAPSULE_NORMAL_BLOB) > > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); > > > > Simple is the best :) > > Common, please don't tell me that the code above is complicated. Just > that we can do without the empty_capsule variable, yes. Sorry, not complicated, but having two variables make little sense. -Takahiro Akashi > -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.25.1 > > >
hi Takahiro, On Thu, 16 Jun 2022 at 06:31, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > Sughosh, > > On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote: > > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi > > <takahiro.akashi@linaro.org> wrote: > > > > > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > > > > The Dependable Boot specification[1] 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. > > > > > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > --- > > > > doc/mkeficapsule.1 | 29 ++++++--- > > > > tools/eficapsule.h | 8 +++ > > > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > > > 3 files changed, 151 insertions(+), 25 deletions(-) > > > > > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > > > > index 09bdc24295..77ca061efd 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. > > > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > > > > This type of image file can be generated by > > > > .BR mkimage . > > > > > > > > -.PP > > > > -If you want to use other types than above two, you should explicitly > > > > -specify a guid for the FMP driver. > > > > - > > > > .SH "OPTIONS" > > > > + > > > > .TP > > > > .BI "-g\fR,\fB --guid " guid-string > > > > Specify guid for image blob type. The format is: > > > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > > > > > > > The first three elements are in little endian, while the rest > > > > -is in big endian. > > > > +is in big endian. The option must be specified for all non empty and > > > > +image acceptance capsules > > > > > > "image acceptance" -> "firmware acceptance" > > > > Okay > > > > > > > > I don't still understand why we need a guid for acceptance > > > while revert doesn't require it. > > > I believe that firmware update is "all or nothing", isn't it? > > > > I believe this gives more flexibility in that different components > > might be required to accept the various firmware images. So, one > > component might accept the optee_os, while another might be > > responsible for accepting u-boot. In any case, we do check that all > > the components have their accepted bit set, and only if so, does the > > bank boot in the regular state. > > Probably I don't understand the behavior. > Let's assume that we have firmware A and firmware B and then > update both. > When the firmware A is accepted and B is not (not yet issuing > acceptance capsule) and I try to reboot the system, what happens? > From which bank does the system boot, old one or new one? Once any/all of the images have been updated, on subsequent reboot, the platform would boot in Trial State from the updated bank. I have introduced an EFI variable, TrialeStateCtr for counting the number of times the system is booting in the trial state. The system remains in trial state as long as all the images from the updated bank have not been accepted. The platform boots in trial state for a particular number of iterations(configurable), and once that count has exceeded, the active bank value gets changed to the previous_active_index, and the platform subsequently boots from the other bank. If all the images do get accepted while the platform is in trial state, the platform transitions to the regular state, and continues booting from that bank. -sughosh > > > In case of a firmware revert, it would > > not matter which firmware component is being reverted -- the platform > > would simply need to boot from the other bank. Do you see any issue > > with the current method that we have? > > > > > > > > If there is a good reason, please describe a possible/expected > > > scenario. > > > > Where do you want me to explain this, in the feature documentation? Or > > do you think this can be elaborated in greater detail in the spec. > > I prefer some explanation in U-Boot doc. > > > > > > > > .TP > > > > .BI "-i\fR,\fB --index " index > > > > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > > > > --- a/tools/eficapsule.h > > > > +++ b/tools/eficapsule.h > > > > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > > > > --- a/tools/mkeficapsule.c > > > > +++ b/tools/mkeficapsule.c > > > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > > > + > > > > +static bool empty_capsule; > > > > +static unsigned char capsule; > > > > + > > > > +enum { > > > > + CAPSULE_NORMAL_BLOB = 0, > > > > + CAPSULE_ACCEPT, > > > > + CAPSULE_REVERT, > > > > +} capsule_type; > > > > > > > > static struct option options[] = { > > > > {"guid", required_argument, NULL, 'g'}, > > > > @@ -39,24 +48,47 @@ static struct option options[] = { > > > > {"certificate", required_argument, NULL, 'c'}, > > > > {"monotonic-count", required_argument, NULL, 'm'}, > > > > {"dump-sig", no_argument, NULL, 'd'}, > > > > + {"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-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" > > > > - "\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-h, --help print a help message\n", > > > > - tool_name); > > > > + if (empty_capsule) { > > > > + if (capsule == CAPSULE_ACCEPT) { > > > > + 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" > > > > + "Options:\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" > > > > + "\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-A, --fw-accept firmware accept capsule\n" > > > > + "\t-R, --fw-revert firmware revert capsule\n" > > > > + "\t-h, --help print a help message\n", > > > > + tool_name); > > > > + } > > > > } > > > > > > > > /** > > > > @@ -564,6 +596,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)); > > > > > > -> guidcpy() > > > > This being a host tool, guidcpy cannot be used. You have used memcpy > > in the create_fwbin() for the same reason I guess. > > Ah, right. > > > > > > > > + 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)); > > > > > > ditto > > > > Same as above. > > > > > > > > > + 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 > > > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > > > case 'd': > > > > dump_sig = 1; > > > > break; > > > > + case 'A': > > > > + capsule |= CAPSULE_ACCEPT; > > > > + break; > > > > + case 'R': > > > > + capsule |= CAPSULE_REVERT; > > > > + break; > > > > case 'h': > > > > print_usage(); > > > > exit(EXIT_SUCCESS); > > > > } > > > > } > > > > > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > > > + fprintf(stderr, > > > > + "Select either of Accept or Revert capsule generation\n"); > > > > + exit(EXIT_FAILURE); > > > > + } > > > > + > > > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > > > + capsule == CAPSULE_REVERT); > > > > + > > > > > > So empty_capsule is redundant as empty_capsule is equivalent with > > > "capsule == CAPSULE_NORMAL_BLOB". > > > I think that a single variable, say capsule_type, is enough. > > > > I was using empty_capsule primarily to make the check done below look > > more succinct and readable. But I can change that to capsule != > > CAPSULE_NORMAL_BLOB. > > > > > > > > > /* check necessary parameters */ > > > > - if ((argc != optind + 2) || !guid || > > > > - ((privkey_file && !cert_file) || > > > > - (!privkey_file && cert_file))) { > > > > + if ((!empty_capsule && > > > > + ((argc != optind + 2) || !guid || > > > > + ((privkey_file && !cert_file) || > > > > + (!privkey_file && cert_file)))) || > > > > + (empty_capsule && > > > > + ((argc != optind + 1) || > > > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > > > + ((capsule == CAPSULE_REVERT) && 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, > > > > + capsule == CAPSULE_ACCEPT) < 0) { > > > > > > if (capsule_type != CAPSULE_NORMAL_BLOB) > > > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); > > > > > > Simple is the best :) > > > > Common, please don't tell me that the code above is complicated. Just > > that we can do without the empty_capsule variable, yes. > > Sorry, not complicated, but having two variables make little sense. > > -Takahiro Akashi > > > -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.25.1 > > > >
Sughosh, On Thu, Jun 16, 2022 at 12:42:08PM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Thu, 16 Jun 2022 at 06:31, Takahiro Akashi > <takahiro.akashi@linaro.org> wrote: > > > > Sughosh, > > > > On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote: > > > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > > > > > The Dependable Boot specification[1] 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. > > > > > > > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > --- > > > > > doc/mkeficapsule.1 | 29 ++++++--- > > > > > tools/eficapsule.h | 8 +++ > > > > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > > > > 3 files changed, 151 insertions(+), 25 deletions(-) > > > > > > > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > > > > > index 09bdc24295..77ca061efd 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. > > > > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > > > > > This type of image file can be generated by > > > > > .BR mkimage . > > > > > > > > > > -.PP > > > > > -If you want to use other types than above two, you should explicitly > > > > > -specify a guid for the FMP driver. > > > > > - > > > > > .SH "OPTIONS" > > > > > + > > > > > .TP > > > > > .BI "-g\fR,\fB --guid " guid-string > > > > > Specify guid for image blob type. The format is: > > > > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > > > > > > > > > The first three elements are in little endian, while the rest > > > > > -is in big endian. > > > > > +is in big endian. The option must be specified for all non empty and > > > > > +image acceptance capsules > > > > > > > > "image acceptance" -> "firmware acceptance" > > > > > > Okay > > > > > > > > > > > I don't still understand why we need a guid for acceptance > > > > while revert doesn't require it. > > > > I believe that firmware update is "all or nothing", isn't it? > > > > > > I believe this gives more flexibility in that different components > > > might be required to accept the various firmware images. So, one > > > component might accept the optee_os, while another might be > > > responsible for accepting u-boot. In any case, we do check that all > > > the components have their accepted bit set, and only if so, does the > > > bank boot in the regular state. > > > > Probably I don't understand the behavior. > > Let's assume that we have firmware A and firmware B and then > > update both. > > When the firmware A is accepted and B is not (not yet issuing > > acceptance capsule) and I try to reboot the system, what happens? > > From which bank does the system boot, old one or new one? > > Once any/all of the images have been updated, on subsequent reboot, > the platform would boot in Trial State from the updated bank. I have > introduced an EFI variable, TrialeStateCtr for counting the number of > times the system is booting in the trial state. The system remains in > trial state as long as all the images from the updated bank have not > been accepted. The platform boots in trial state for a particular > number of iterations(configurable), and once that count has exceeded, > the active bank value gets changed to the previous_active_index, and > the platform subsequently boots from the other bank. If all the images > do get accepted while the platform is in trial state, the platform > transitions to the regular state, and continues booting from that > bank. Thank you for the details. But if I understand correctly, why do we need a partially-accepted status? When some are accepted but others not, the system boots from an update bank in any way. Right? So at the end, all that we should do is to accept the boot "from a new bank" permanently (and transit to, what you say, regular state), or to let the system boot from an old bank (again, in a regular state). I don't see any good reason for having a partial acceptance. We should continue this discussion, but anyhow, you should think of adding such a description above in U-Boot doc. -Takahiro Akashi > -sughosh > > > > > > In case of a firmware revert, it would > > > not matter which firmware component is being reverted -- the platform > > > would simply need to boot from the other bank. Do you see any issue > > > with the current method that we have? > > > > > > > > > > > If there is a good reason, please describe a possible/expected > > > > scenario. > > > > > > Where do you want me to explain this, in the feature documentation? Or > > > do you think this can be elaborated in greater detail in the spec. > > > > I prefer some explanation in U-Boot doc. > > > > > > > > > > > .TP > > > > > .BI "-i\fR,\fB --index " index > > > > > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > > > > > --- a/tools/eficapsule.h > > > > > +++ b/tools/eficapsule.h > > > > > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > > > > > --- a/tools/mkeficapsule.c > > > > > +++ b/tools/mkeficapsule.c > > > > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > > > > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > > > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > > > > + > > > > > +static bool empty_capsule; > > > > > +static unsigned char capsule; > > > > > + > > > > > +enum { > > > > > + CAPSULE_NORMAL_BLOB = 0, > > > > > + CAPSULE_ACCEPT, > > > > > + CAPSULE_REVERT, > > > > > +} capsule_type; > > > > > > > > > > static struct option options[] = { > > > > > {"guid", required_argument, NULL, 'g'}, > > > > > @@ -39,24 +48,47 @@ static struct option options[] = { > > > > > {"certificate", required_argument, NULL, 'c'}, > > > > > {"monotonic-count", required_argument, NULL, 'm'}, > > > > > {"dump-sig", no_argument, NULL, 'd'}, > > > > > + {"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-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" > > > > > - "\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-h, --help print a help message\n", > > > > > - tool_name); > > > > > + if (empty_capsule) { > > > > > + if (capsule == CAPSULE_ACCEPT) { > > > > > + 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" > > > > > + "Options:\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" > > > > > + "\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-A, --fw-accept firmware accept capsule\n" > > > > > + "\t-R, --fw-revert firmware revert capsule\n" > > > > > + "\t-h, --help print a help message\n", > > > > > + tool_name); > > > > > + } > > > > > } > > > > > > > > > > /** > > > > > @@ -564,6 +596,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)); > > > > > > > > -> guidcpy() > > > > > > This being a host tool, guidcpy cannot be used. You have used memcpy > > > in the create_fwbin() for the same reason I guess. > > > > Ah, right. > > > > > > > > > > > + 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)); > > > > > > > > ditto > > > > > > Same as above. > > > > > > > > > > > > + 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 > > > > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > > > > case 'd': > > > > > dump_sig = 1; > > > > > break; > > > > > + case 'A': > > > > > + capsule |= CAPSULE_ACCEPT; > > > > > + break; > > > > > + case 'R': > > > > > + capsule |= CAPSULE_REVERT; > > > > > + break; > > > > > case 'h': > > > > > print_usage(); > > > > > exit(EXIT_SUCCESS); > > > > > } > > > > > } > > > > > > > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > > > > + fprintf(stderr, > > > > > + "Select either of Accept or Revert capsule generation\n"); > > > > > + exit(EXIT_FAILURE); > > > > > + } > > > > > + > > > > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > > > > + capsule == CAPSULE_REVERT); > > > > > + > > > > > > > > So empty_capsule is redundant as empty_capsule is equivalent with > > > > "capsule == CAPSULE_NORMAL_BLOB". > > > > I think that a single variable, say capsule_type, is enough. > > > > > > I was using empty_capsule primarily to make the check done below look > > > more succinct and readable. But I can change that to capsule != > > > CAPSULE_NORMAL_BLOB. > > > > > > > > > > > > /* check necessary parameters */ > > > > > - if ((argc != optind + 2) || !guid || > > > > > - ((privkey_file && !cert_file) || > > > > > - (!privkey_file && cert_file))) { > > > > > + if ((!empty_capsule && > > > > > + ((argc != optind + 2) || !guid || > > > > > + ((privkey_file && !cert_file) || > > > > > + (!privkey_file && cert_file)))) || > > > > > + (empty_capsule && > > > > > + ((argc != optind + 1) || > > > > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > > > > + ((capsule == CAPSULE_REVERT) && 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, > > > > > + capsule == CAPSULE_ACCEPT) < 0) { > > > > > > > > if (capsule_type != CAPSULE_NORMAL_BLOB) > > > > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); > > > > > > > > Simple is the best :) > > > > > > Common, please don't tell me that the code above is complicated. Just > > > that we can do without the empty_capsule variable, yes. > > > > Sorry, not complicated, but having two variables make little sense. > > > > -Takahiro Akashi > > > > > -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.25.1 > > > > >
Takahiro, On Fri, 17 Jun 2022 at 06:16, Takahiro Akashi <takahiro.akashi@linaro.org> wrote: > > Sughosh, > > On Thu, Jun 16, 2022 at 12:42:08PM +0530, Sughosh Ganu wrote: > > hi Takahiro, > > > > On Thu, 16 Jun 2022 at 06:31, Takahiro Akashi > > <takahiro.akashi@linaro.org> wrote: > > > > > > Sughosh, > > > > > > On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote: > > > > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi > > > > <takahiro.akashi@linaro.org> wrote: > > > > > > > > > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, Sughosh Ganu wrote: > > > > > > The Dependable Boot specification[1] 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. > > > > > > > > > > > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > > > > --- > > > > > > doc/mkeficapsule.1 | 29 ++++++--- > > > > > > tools/eficapsule.h | 8 +++ > > > > > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > > > > > > 3 files changed, 151 insertions(+), 25 deletions(-) > > > > > > > > > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > > > > > > index 09bdc24295..77ca061efd 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. > > > > > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > > > > > > This type of image file can be generated by > > > > > > .BR mkimage . > > > > > > > > > > > > -.PP > > > > > > -If you want to use other types than above two, you should explicitly > > > > > > -specify a guid for the FMP driver. > > > > > > - > > > > > > .SH "OPTIONS" > > > > > > + > > > > > > .TP > > > > > > .BI "-g\fR,\fB --guid " guid-string > > > > > > Specify guid for image blob type. The format is: > > > > > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > > > > > > > > > > > The first three elements are in little endian, while the rest > > > > > > -is in big endian. > > > > > > +is in big endian. The option must be specified for all non empty and > > > > > > +image acceptance capsules > > > > > > > > > > "image acceptance" -> "firmware acceptance" > > > > > > > > Okay > > > > > > > > > > > > > > I don't still understand why we need a guid for acceptance > > > > > while revert doesn't require it. > > > > > I believe that firmware update is "all or nothing", isn't it? > > > > > > > > I believe this gives more flexibility in that different components > > > > might be required to accept the various firmware images. So, one > > > > component might accept the optee_os, while another might be > > > > responsible for accepting u-boot. In any case, we do check that all > > > > the components have their accepted bit set, and only if so, does the > > > > bank boot in the regular state. > > > > > > Probably I don't understand the behavior. > > > Let's assume that we have firmware A and firmware B and then > > > update both. > > > When the firmware A is accepted and B is not (not yet issuing > > > acceptance capsule) and I try to reboot the system, what happens? > > > From which bank does the system boot, old one or new one? > > > > Once any/all of the images have been updated, on subsequent reboot, > > the platform would boot in Trial State from the updated bank. I have > > introduced an EFI variable, TrialeStateCtr for counting the number of > > times the system is booting in the trial state. The system remains in > > trial state as long as all the images from the updated bank have not > > been accepted. The platform boots in trial state for a particular > > number of iterations(configurable), and once that count has exceeded, > > the active bank value gets changed to the previous_active_index, and > > the platform subsequently boots from the other bank. If all the images > > do get accepted while the platform is in trial state, the platform > > transitions to the regular state, and continues booting from that > > bank. > > Thank you for the details. > But if I understand correctly, why do we need a partially-accepted status? It's not so much about needing a partial update, but allowing for a scenario where multiple components might be tasked with accepting different firmware images. I see this as a kind of flexibility given. I do appreciate your point of view to have a common format for accept and reject capsules, but this format does not hurt IMO as long as it is documented and explained. > When some are accepted but others not, the system boots from an update > bank in any way. Right? > So at the end, all that we should do is to accept the boot "from a new bank" > permanently (and transit to, what you say, regular state), or to let the system > boot from an old bank (again, in a regular state). > > I don't see any good reason for having a partial acceptance. > > We should continue this discussion, but anyhow, you should think of > adding such a description above in U-Boot doc. Okay. Will do. -sughosh > > -Takahiro Akashi > > > > -sughosh > > > > > > > > > In case of a firmware revert, it would > > > > not matter which firmware component is being reverted -- the platform > > > > would simply need to boot from the other bank. Do you see any issue > > > > with the current method that we have? > > > > > > > > > > > > > > If there is a good reason, please describe a possible/expected > > > > > scenario. > > > > > > > > Where do you want me to explain this, in the feature documentation? Or > > > > do you think this can be elaborated in greater detail in the spec. > > > > > > I prefer some explanation in U-Boot doc. > > > > > > > > > > > > > > .TP > > > > > > .BI "-i\fR,\fB --index " index > > > > > > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > > > > > > --- a/tools/eficapsule.h > > > > > > +++ b/tools/eficapsule.h > > > > > > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > > > > > > --- a/tools/mkeficapsule.c > > > > > > +++ b/tools/mkeficapsule.c > > > > > > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > > > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > > > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > > > > > > > > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > > > > > > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > > > > > > + > > > > > > +static bool empty_capsule; > > > > > > +static unsigned char capsule; > > > > > > + > > > > > > +enum { > > > > > > + CAPSULE_NORMAL_BLOB = 0, > > > > > > + CAPSULE_ACCEPT, > > > > > > + CAPSULE_REVERT, > > > > > > +} capsule_type; > > > > > > > > > > > > static struct option options[] = { > > > > > > {"guid", required_argument, NULL, 'g'}, > > > > > > @@ -39,24 +48,47 @@ static struct option options[] = { > > > > > > {"certificate", required_argument, NULL, 'c'}, > > > > > > {"monotonic-count", required_argument, NULL, 'm'}, > > > > > > {"dump-sig", no_argument, NULL, 'd'}, > > > > > > + {"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-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" > > > > > > - "\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-h, --help print a help message\n", > > > > > > - tool_name); > > > > > > + if (empty_capsule) { > > > > > > + if (capsule == CAPSULE_ACCEPT) { > > > > > > + 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" > > > > > > + "Options:\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" > > > > > > + "\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-A, --fw-accept firmware accept capsule\n" > > > > > > + "\t-R, --fw-revert firmware revert capsule\n" > > > > > > + "\t-h, --help print a help message\n", > > > > > > + tool_name); > > > > > > + } > > > > > > } > > > > > > > > > > > > /** > > > > > > @@ -564,6 +596,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)); > > > > > > > > > > -> guidcpy() > > > > > > > > This being a host tool, guidcpy cannot be used. You have used memcpy > > > > in the create_fwbin() for the same reason I guess. > > > > > > Ah, right. > > > > > > > > > > > > > > + 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)); > > > > > > > > > > ditto > > > > > > > > Same as above. > > > > > > > > > > > > > > > + 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 > > > > > > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > > > > > > case 'd': > > > > > > dump_sig = 1; > > > > > > break; > > > > > > + case 'A': > > > > > > + capsule |= CAPSULE_ACCEPT; > > > > > > + break; > > > > > > + case 'R': > > > > > > + capsule |= CAPSULE_REVERT; > > > > > > + break; > > > > > > case 'h': > > > > > > print_usage(); > > > > > > exit(EXIT_SUCCESS); > > > > > > } > > > > > > } > > > > > > > > > > > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > > > > > > + fprintf(stderr, > > > > > > + "Select either of Accept or Revert capsule generation\n"); > > > > > > + exit(EXIT_FAILURE); > > > > > > + } > > > > > > + > > > > > > + empty_capsule = (capsule == CAPSULE_ACCEPT || > > > > > > + capsule == CAPSULE_REVERT); > > > > > > + > > > > > > > > > > So empty_capsule is redundant as empty_capsule is equivalent with > > > > > "capsule == CAPSULE_NORMAL_BLOB". > > > > > I think that a single variable, say capsule_type, is enough. > > > > > > > > I was using empty_capsule primarily to make the check done below look > > > > more succinct and readable. But I can change that to capsule != > > > > CAPSULE_NORMAL_BLOB. > > > > > > > > > > > > > > > /* check necessary parameters */ > > > > > > - if ((argc != optind + 2) || !guid || > > > > > > - ((privkey_file && !cert_file) || > > > > > > - (!privkey_file && cert_file))) { > > > > > > + if ((!empty_capsule && > > > > > > + ((argc != optind + 2) || !guid || > > > > > > + ((privkey_file && !cert_file) || > > > > > > + (!privkey_file && cert_file)))) || > > > > > > + (empty_capsule && > > > > > > + ((argc != optind + 1) || > > > > > > + ((capsule == CAPSULE_ACCEPT) && !guid) || > > > > > > + ((capsule == CAPSULE_REVERT) && 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, > > > > > > + capsule == CAPSULE_ACCEPT) < 0) { > > > > > > > > > > if (capsule_type != CAPSULE_NORMAL_BLOB) > > > > > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT); > > > > > > > > > > Simple is the best :) > > > > > > > > Common, please don't tell me that the code above is complicated. Just > > > > that we can do without the empty_capsule variable, yes. > > > > > > Sorry, not complicated, but having two variables make little sense. > > > > > > -Takahiro Akashi > > > > > > > -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.25.1 > > > > > >
On Thu, 9 Jun 2022 at 14:31, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > The Dependable Boot specification[1] 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. > > [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > doc/mkeficapsule.1 | 29 ++++++--- > tools/eficapsule.h | 8 +++ > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ > 3 files changed, 151 insertions(+), 25 deletions(-) > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 > index 09bdc24295..77ca061efd 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. > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. > This type of image file can be generated by > .BR mkimage . > > -.PP > -If you want to use other types than above two, you should explicitly > -specify a guid for the FMP driver. > - > .SH "OPTIONS" > + > .TP > .BI "-g\fR,\fB --guid " guid-string > Specify guid for image blob type. The format is: > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx > > The first three elements are in little endian, while the rest > -is in big endian. > +is in big endian. The option must be specified for all non empty and > +image acceptance capsules > > .TP > .BI "-i\fR,\fB --index " index > @@ -57,6 +60,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 d63b831443..072a4b5598 100644 > --- a/tools/eficapsule.h > +++ b/tools/eficapsule.h > @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > -static const char *opts_short = "g:i:I:v:p:c:m:dh"; > +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; > + > +static bool empty_capsule; > +static unsigned char capsule; Local function create_fwbin() defines a local variable with that same label. Should be ok to remove this 'capsule' variable if only using 'capsule_type' as suggested by Takahiro-san. > + > +enum { > + CAPSULE_NORMAL_BLOB = 0, > + CAPSULE_ACCEPT, > + CAPSULE_REVERT, > +} capsule_type; > > static struct option options[] = { > {"guid", required_argument, NULL, 'g'}, > @@ -39,24 +48,47 @@ static struct option options[] = { > {"certificate", required_argument, NULL, 'c'}, > {"monotonic-count", required_argument, NULL, 'm'}, > {"dump-sig", no_argument, NULL, 'd'}, > + {"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-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" > - "\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-h, --help print a help message\n", > - tool_name); > + if (empty_capsule) { > + if (capsule == CAPSULE_ACCEPT) { > + 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" > + "Options:\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" > + "\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-A, --fw-accept firmware accept capsule\n" > + "\t-R, --fw-revert firmware revert capsule\n" > + "\t-h, --help print a help message\n", > + tool_name); > + } > } > > /** > @@ -564,6 +596,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; It would be better to initialize the header to known value, e.g. = { }; here. > + 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")) nitpicking: is payload temp variable needed? could pass guid as argument. > + goto err; > + } > + > + ret = 0; > + > +err: > + if (f) > + fclose(f); > + > + return ret; > +} > + > /** > * main - main entry function of mkeficapsule > * @argc: Number of arguments > @@ -639,22 +715,49 @@ int main(int argc, char **argv) > case 'd': > dump_sig = 1; > break; > + case 'A': > + capsule |= CAPSULE_ACCEPT; > + break; > + case 'R': > + capsule |= CAPSULE_REVERT; > + break; > case 'h': > print_usage(); > exit(EXIT_SUCCESS); > } > } > > + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { > + fprintf(stderr, > + "Select either of Accept or Revert capsule generation\n"); > + exit(EXIT_FAILURE); > + } > + > + empty_capsule = (capsule == CAPSULE_ACCEPT || > + capsule == CAPSULE_REVERT); > + > /* check necessary parameters */ > - if ((argc != optind + 2) || !guid || > - ((privkey_file && !cert_file) || > - (!privkey_file && cert_file))) { > + if ((!empty_capsule && > + ((argc != optind + 2) || !guid || > + ((privkey_file && !cert_file) || > + (!privkey_file && cert_file)))) || > + (empty_capsule && > + ((argc != optind + 1) || > + ((capsule == CAPSULE_ACCEPT) && !guid) || > + ((capsule == CAPSULE_REVERT) && 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, > + capsule == CAPSULE_ACCEPT) < 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); > } > -- > 2.25.1 >
diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1 index 09bdc24295..77ca061efd 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. @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file. This type of image file can be generated by .BR mkimage . -.PP -If you want to use other types than above two, you should explicitly -specify a guid for the FMP driver. - .SH "OPTIONS" + .TP .BI "-g\fR,\fB --guid " guid-string Specify guid for image blob type. The format is: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx The first three elements are in little endian, while the rest -is in big endian. +is in big endian. The option must be specified for all non empty and +image acceptance capsules .TP .BI "-i\fR,\fB --index " index @@ -57,6 +60,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 d63b831443..072a4b5598 100644 --- a/tools/eficapsule.h +++ b/tools/eficapsule.h @@ -41,6 +41,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 5f74d23b9e..e8eb6b070d 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -29,7 +29,16 @@ static const char *tool_name = "mkeficapsule"; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; -static const char *opts_short = "g:i:I:v:p:c:m:dh"; +static const char *opts_short = "g:i:I:v:p:c:m:dhAR"; + +static bool empty_capsule; +static unsigned char capsule; + +enum { + CAPSULE_NORMAL_BLOB = 0, + CAPSULE_ACCEPT, + CAPSULE_REVERT, +} capsule_type; static struct option options[] = { {"guid", required_argument, NULL, 'g'}, @@ -39,24 +48,47 @@ static struct option options[] = { {"certificate", required_argument, NULL, 'c'}, {"monotonic-count", required_argument, NULL, 'm'}, {"dump-sig", no_argument, NULL, 'd'}, + {"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-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" - "\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-h, --help print a help message\n", - tool_name); + if (empty_capsule) { + if (capsule == CAPSULE_ACCEPT) { + 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" + "Options:\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" + "\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-A, --fw-accept firmware accept capsule\n" + "\t-R, --fw-revert firmware revert capsule\n" + "\t-h, --help print a help message\n", + tool_name); + } } /** @@ -564,6 +596,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 @@ -639,22 +715,49 @@ int main(int argc, char **argv) case 'd': dump_sig = 1; break; + case 'A': + capsule |= CAPSULE_ACCEPT; + break; + case 'R': + capsule |= CAPSULE_REVERT; + break; case 'h': print_usage(); exit(EXIT_SUCCESS); } } + if (capsule == (CAPSULE_ACCEPT | CAPSULE_REVERT)) { + fprintf(stderr, + "Select either of Accept or Revert capsule generation\n"); + exit(EXIT_FAILURE); + } + + empty_capsule = (capsule == CAPSULE_ACCEPT || + capsule == CAPSULE_REVERT); + /* check necessary parameters */ - if ((argc != optind + 2) || !guid || - ((privkey_file && !cert_file) || - (!privkey_file && cert_file))) { + if ((!empty_capsule && + ((argc != optind + 2) || !guid || + ((privkey_file && !cert_file) || + (!privkey_file && cert_file)))) || + (empty_capsule && + ((argc != optind + 1) || + ((capsule == CAPSULE_ACCEPT) && !guid) || + ((capsule == CAPSULE_REVERT) && 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, + capsule == CAPSULE_ACCEPT) < 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); }
The Dependable Boot specification[1] 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. [1] - https://git.codelinaro.org/linaro/dependable-boot/mbfw/uploads/6f7ddfe3be24e18d4319e108a758d02e/mbfw.pdf Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- doc/mkeficapsule.1 | 29 ++++++--- tools/eficapsule.h | 8 +++ tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------ 3 files changed, 151 insertions(+), 25 deletions(-)