On Thu, 9 Jun 2022 at 21:58, Heinrich Schuchardt <xypron.g...@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.g...@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); > > } >