| Message ID | 20220119185548.16730-10-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, From user's point of view, On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote: > The Dependable Boot specification describes the structure of the May we have a pointer or reference to it? > firmware accept and revert capsules. These are empty capsules which > are used for signalling the acceptance or rejection of the updated > firmware by the OS. Add support for generating these empty capsules. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > > Changes since V2: > * New patch for generating empty capsules > > tools/eficapsule.h | 8 ++++ > tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 100 insertions(+), 10 deletions(-) > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h > index 8c1560bb06..6001952bdc 100644 > --- a/tools/eficapsule.h > +++ b/tools/eficapsule.h > @@ -50,6 +50,14 @@ typedef struct { > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > +#define FW_ACCEPT_OS_GUID \ > + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > + > +#define FW_REVERT_OS_GUID \ > + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ > + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) > + > /* flags */ > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > index 161affdd15..643da3849d 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -29,6 +29,7 @@ > #include "eficapsule.h" > > static const char *tool_name = "mkeficapsule"; > +static unsigned char empty_capsule; > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > efi_guid_t efi_guid_image_type_uboot_fit = > @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > #ifdef CONFIG_TOOLS_LIBCRYPTO > -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; > +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; > #else > -static const char *opts_short = "frg:i:I:v:h"; > +static const char *opts_short = "frg:i:I:v:hAR"; > #endif > > static struct option options[] = { > @@ -55,15 +56,23 @@ static struct option options[] = { > {"monotonic-count", required_argument, NULL, 'm'}, > {"dump-sig", no_argument, NULL, 'd'}, > #endif > + {"fw-accept", no_argument, NULL, 'A'}, > + {"fw-revert", no_argument, NULL, 'R'}, > {"help", no_argument, NULL, 'h'}, > {NULL, 0, NULL, 0}, > }; > > static void print_usage(void) > { > - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" > - "Options:\n" > + if (empty_capsule) { > + fprintf(stderr, "Usage: %s [options] <output file>\n", > + tool_name); > + } else { > + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", > + tool_name); > + } We should always show both cases regardless of "empty_capsule". Or if you have any restrictions on a combination of options, you'd better describe them more specifically in help message. I'd also like to encourage you to update the man page as well as uefi.rst. > + fprintf(stderr, "Options:\n" > "\t-f, --fit FIT image type\n" > "\t-r, --raw raw image type\n" > "\t-g, --guid <guid string> guid for image blob type\n" > @@ -75,8 +84,9 @@ static void print_usage(void) > "\t-m, --monotonic-count <count> monotonic count\n" > "\t-d, --dump_sig dump signature (*.p7)\n" > #endif > - "\t-h, --help print a help message\n", > - tool_name); > + "\t-A, --fw-accept firmware accept capsule\n" > + "\t-R, --fw-revert firmware revert capsule\n" > + "\t-h, --help print a help message\n"); > } > > /** > @@ -598,6 +608,59 @@ 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; > + int ret; > + 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 = NULL; > + ret = -1; > + > + f = fopen(path, "w"); > + if (!f) { > + printf("cannot open %s\n", path); To stderr as Heinrich has requested. > + goto err; > + } > + > + if (fw_accept) > + capsule_guid = fw_accept_guid; > + else > + capsule_guid = fw_revert_guid; > + > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > + header.header_size = sizeof(header); > + header.flags = 0; > + > + if (fw_accept) { > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > + } else { > + header.capsule_image_size = sizeof(header); > + } I wonder why we don't need GUID in revert case (and why need GUID in fw case. Since we want to add A/B update, there seems to be no ambiguity. > + 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 > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > unsigned char uuid_buf[16]; > unsigned long index, instance; > uint64_t mcount; > + unsigned char accept_fw_capsule, revert_fw_capsule; > char *privkey_file, *cert_file; > int c, idx; > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > mcount = 0; > privkey_file = NULL; > cert_file = NULL; > + accept_fw_capsule = 0; > + revert_fw_capsule = 0; > dump_sig = 0; > for (;;) { > c = getopt_long(argc, argv, opts_short, options, &idx); > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > dump_sig = 1; > break; > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > + case 'A': > + accept_fw_capsule = 1; > + break; > + case 'R': > + revert_fw_capsule = 1; > + break; > case 'h': > print_usage(); > exit(EXIT_SUCCESS); > } > } > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); Please check that two options are exclusive here. > /* check necessary parameters */ > - if ((argc != optind + 2) || !guid || > - ((privkey_file && !cert_file) || > + if ((!empty_capsule && argc != optind + 2) || > + (empty_capsule && argc != optind + 1) || > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > (!privkey_file && cert_file))) { > print_usage(); > exit(EXIT_FAILURE); > } > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > - mcount, privkey_file, cert_file) < 0) { > + if (empty_capsule) { > + if (create_empty_capsule(argv[argc - 1], guid, > + accept_fw_capsule ? 1 : 0) < 0) { > + printf("Creating empty capsule failed\n"); To stderr -Takahiro Akashi > + exit(EXIT_FAILURE); > + } > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > + index, instance, mcount, privkey_file, > + cert_file) < 0) { > fprintf(stderr, "Creating firmware capsule failed\n"); > exit(EXIT_FAILURE); > } > -- > 2.17.1 >
hi Takahiro, On Thu, 20 Jan 2022 at 07:43, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Sughosh, > > From user's point of view, > > On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote: > > The Dependable Boot specification describes the structure of the > > May we have a pointer or reference to it? Okay. > > > firmware accept and revert capsules. These are empty capsules which > > are used for signalling the acceptance or rejection of the updated > > firmware by the OS. Add support for generating these empty capsules. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > > > Changes since V2: > > * New patch for generating empty capsules > > > > tools/eficapsule.h | 8 ++++ > > tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- > > 2 files changed, 100 insertions(+), 10 deletions(-) > > > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h > > index 8c1560bb06..6001952bdc 100644 > > --- a/tools/eficapsule.h > > +++ b/tools/eficapsule.h > > @@ -50,6 +50,14 @@ typedef struct { > > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > > > +#define FW_ACCEPT_OS_GUID \ > > + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > > + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > > + > > +#define FW_REVERT_OS_GUID \ > > + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ > > + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) > > + > > /* flags */ > > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > > index 161affdd15..643da3849d 100644 > > --- a/tools/mkeficapsule.c > > +++ b/tools/mkeficapsule.c > > @@ -29,6 +29,7 @@ > > #include "eficapsule.h" > > > > static const char *tool_name = "mkeficapsule"; > > +static unsigned char empty_capsule; > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > efi_guid_t efi_guid_image_type_uboot_fit = > > @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > #ifdef CONFIG_TOOLS_LIBCRYPTO > > -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; > > +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; > > #else > > -static const char *opts_short = "frg:i:I:v:h"; > > +static const char *opts_short = "frg:i:I:v:hAR"; > > #endif > > > > static struct option options[] = { > > @@ -55,15 +56,23 @@ static struct option options[] = { > > {"monotonic-count", required_argument, NULL, 'm'}, > > {"dump-sig", no_argument, NULL, 'd'}, > > #endif > > + {"fw-accept", no_argument, NULL, 'A'}, > > + {"fw-revert", no_argument, NULL, 'R'}, > > {"help", no_argument, NULL, 'h'}, > > {NULL, 0, NULL, 0}, > > }; > > > > static void print_usage(void) > > { > > - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" > > - "Options:\n" > > + if (empty_capsule) { > > + fprintf(stderr, "Usage: %s [options] <output file>\n", > > + tool_name); > > + } else { > > + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", > > + tool_name); > > + } > > We should always show both cases regardless of "empty_capsule". > > Or if you have any restrictions on a combination of options, > you'd better describe them more specifically in help message. Yes, there are restrictions on the combination of options. I will call a separate function, like empty_capsule_usage for these. > > I'd also like to encourage you to update the man page as well as uefi.rst. Okay > > > + fprintf(stderr, "Options:\n" > > "\t-f, --fit FIT image type\n" > > "\t-r, --raw raw image type\n" > > "\t-g, --guid <guid string> guid for image blob type\n" > > @@ -75,8 +84,9 @@ static void print_usage(void) > > "\t-m, --monotonic-count <count> monotonic count\n" > > "\t-d, --dump_sig dump signature (*.p7)\n" > > #endif > > - "\t-h, --help print a help message\n", > > - tool_name); > > + "\t-A, --fw-accept firmware accept capsule\n" > > + "\t-R, --fw-revert firmware revert capsule\n" > > + "\t-h, --help print a help message\n"); > > } > > > > /** > > @@ -598,6 +608,59 @@ 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; > > + int ret; > > + 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 = NULL; > > + ret = -1; > > + > > + f = fopen(path, "w"); > > + if (!f) { > > + printf("cannot open %s\n", path); > > To stderr as Heinrich has requested. I thought I saw an email from Heinrich in which he said that he did not want a fprintf call, and was going to revert those hunks from your patch. I will recheck this bit. > > > + goto err; > > + } > > + > > + if (fw_accept) > > + capsule_guid = fw_accept_guid; > > + else > > + capsule_guid = fw_revert_guid; > > + > > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > > + header.header_size = sizeof(header); > > + header.flags = 0; > > + > > + if (fw_accept) { > > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > > + } else { > > + header.capsule_image_size = sizeof(header); > > + } > > I wonder why we don't need GUID in revert case (and why need GUID > in fw case. Since we want to add A/B update, there seems to be > no ambiguity. The revert capsule is used not as a rejection of a specific individual image, but for reverting the platform to the other bank. Which does not require a image specific GUID. > > > + 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 > > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > > unsigned char uuid_buf[16]; > > unsigned long index, instance; > > uint64_t mcount; > > + unsigned char accept_fw_capsule, revert_fw_capsule; > > char *privkey_file, *cert_file; > > int c, idx; > > > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > > mcount = 0; > > privkey_file = NULL; > > cert_file = NULL; > > + accept_fw_capsule = 0; > > + revert_fw_capsule = 0; > > dump_sig = 0; > > for (;;) { > > c = getopt_long(argc, argv, opts_short, options, &idx); > > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > > dump_sig = 1; > > break; > > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > > + case 'A': > > + accept_fw_capsule = 1; > > + break; > > + case 'R': > > + revert_fw_capsule = 1; > > + break; > > case 'h': > > print_usage(); > > exit(EXIT_SUCCESS); > > } > > } > > > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); > > Please check that two options are exclusive here. Okay > > > /* check necessary parameters */ > > - if ((argc != optind + 2) || !guid || > > - ((privkey_file && !cert_file) || > > + if ((!empty_capsule && argc != optind + 2) || > > + (empty_capsule && argc != optind + 1) || > > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > > (!privkey_file && cert_file))) { > > print_usage(); > > exit(EXIT_FAILURE); > > } > > > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > > - mcount, privkey_file, cert_file) < 0) { > > + if (empty_capsule) { > > + if (create_empty_capsule(argv[argc - 1], guid, > > + accept_fw_capsule ? 1 : 0) < 0) { > > + printf("Creating empty capsule failed\n"); > > To stderr Okay, will check. -sughosh > > -Takahiro Akashi > > > + exit(EXIT_FAILURE); > > + } > > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > > + index, instance, mcount, privkey_file, > > + cert_file) < 0) { > > fprintf(stderr, "Creating firmware capsule failed\n"); > > exit(EXIT_FAILURE); > > } > > -- > > 2.17.1 > >
Hi Sughosh, On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote: > The Dependable Boot specification describes the structure of the > firmware accept and revert capsules. These are empty capsules which > are used for signalling the acceptance or rejection of the updated > firmware by the OS. Add support for generating these empty capsules. > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > --- > > Changes since V2: > * New patch for generating empty capsules > > tools/eficapsule.h | 8 ++++ > tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 100 insertions(+), 10 deletions(-) > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h > index 8c1560bb06..6001952bdc 100644 > --- a/tools/eficapsule.h > +++ b/tools/eficapsule.h > @@ -50,6 +50,14 @@ typedef struct { > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > +#define FW_ACCEPT_OS_GUID \ > + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > + > +#define FW_REVERT_OS_GUID \ > + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ > + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) > + > /* flags */ > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > index 161affdd15..643da3849d 100644 > --- a/tools/mkeficapsule.c > +++ b/tools/mkeficapsule.c > @@ -29,6 +29,7 @@ > #include "eficapsule.h" > > static const char *tool_name = "mkeficapsule"; > +static unsigned char empty_capsule; > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > efi_guid_t efi_guid_image_type_uboot_fit = > @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > #ifdef CONFIG_TOOLS_LIBCRYPTO > -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; > +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; > #else > -static const char *opts_short = "frg:i:I:v:h"; > +static const char *opts_short = "frg:i:I:v:hAR"; > #endif > > static struct option options[] = { > @@ -55,15 +56,23 @@ static struct option options[] = { > {"monotonic-count", required_argument, NULL, 'm'}, > {"dump-sig", no_argument, NULL, 'd'}, > #endif > + {"fw-accept", no_argument, NULL, 'A'}, > + {"fw-revert", no_argument, NULL, 'R'}, > {"help", no_argument, NULL, 'h'}, > {NULL, 0, NULL, 0}, > }; > > static void print_usage(void) > { > - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" > - "Options:\n" > + if (empty_capsule) { > + fprintf(stderr, "Usage: %s [options] <output file>\n", > + tool_name); > + } else { > + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", > + tool_name); > + } > > + fprintf(stderr, "Options:\n" > "\t-f, --fit FIT image type\n" > "\t-r, --raw raw image type\n" > "\t-g, --guid <guid string> guid for image blob type\n" > @@ -75,8 +84,9 @@ static void print_usage(void) > "\t-m, --monotonic-count <count> monotonic count\n" > "\t-d, --dump_sig dump signature (*.p7)\n" > #endif > - "\t-h, --help print a help message\n", > - tool_name); > + "\t-A, --fw-accept firmware accept capsule\n" > + "\t-R, --fw-revert firmware revert capsule\n" > + "\t-h, --help print a help message\n"); > } > > /** > @@ -598,6 +608,59 @@ 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; > + int ret; > + 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 = NULL; > + ret = -1; Can we init those at their declaration? > + > + f = fopen(path, "w"); > + if (!f) { > + printf("cannot open %s\n", path); > + goto err; > + } > + > + if (fw_accept) > + capsule_guid = fw_accept_guid; > + else > + capsule_guid = fw_revert_guid; ternary operator would look better here. > + > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > + header.header_size = sizeof(header); > + header.flags = 0; Is it the flags only you need? Or maybe memset the entire headeri to 0? > + > + if (fw_accept) { > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > + } else { > + header.capsule_image_size = sizeof(header); > + } ternary again? > + > + 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 > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > unsigned char uuid_buf[16]; > unsigned long index, instance; > uint64_t mcount; > + unsigned char accept_fw_capsule, revert_fw_capsule; > char *privkey_file, *cert_file; > int c, idx; > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > mcount = 0; > privkey_file = NULL; > cert_file = NULL; > + accept_fw_capsule = 0; > + revert_fw_capsule = 0; > dump_sig = 0; > for (;;) { > c = getopt_long(argc, argv, opts_short, options, &idx); > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > dump_sig = 1; > break; > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > + case 'A': > + accept_fw_capsule = 1; > + break; > + case 'R': > + revert_fw_capsule = 1; > + break; > case 'h': > print_usage(); > exit(EXIT_SUCCESS); > } > } > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); Why do we need 3 variables here? Would it be better to have an enum and just use a single variable like "is_accept_capsule"? > + > /* check necessary parameters */ > - if ((argc != optind + 2) || !guid || > - ((privkey_file && !cert_file) || > + if ((!empty_capsule && argc != optind + 2) || > + (empty_capsule && argc != optind + 1) || > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > (!privkey_file && cert_file))) { > print_usage(); > exit(EXIT_FAILURE); > } > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > - mcount, privkey_file, cert_file) < 0) { > + if (empty_capsule) { > + if (create_empty_capsule(argv[argc - 1], guid, > + accept_fw_capsule ? 1 : 0) < 0) { > + printf("Creating empty capsule failed\n"); > + exit(EXIT_FAILURE); > + } > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > + index, instance, mcount, privkey_file, > + cert_file) < 0) { > fprintf(stderr, "Creating firmware capsule failed\n"); > exit(EXIT_FAILURE); > } > -- > 2.17.1 > Thanks /Ilias
On Fri, Jan 21, 2022 at 12:18:38PM +0530, Sughosh Ganu wrote: > hi Takahiro, > > On Thu, 20 Jan 2022 at 07:43, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > Hi Sughosh, > > > > From user's point of view, > > > > On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote: > > > The Dependable Boot specification describes the structure of the > > > > May we have a pointer or reference to it? > > Okay. > > > > > > firmware accept and revert capsules. These are empty capsules which > > > are used for signalling the acceptance or rejection of the updated > > > firmware by the OS. Add support for generating these empty capsules. > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > > --- > > > > > > Changes since V2: > > > * New patch for generating empty capsules > > > > > > tools/eficapsule.h | 8 ++++ > > > tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- > > > 2 files changed, 100 insertions(+), 10 deletions(-) > > > > > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h > > > index 8c1560bb06..6001952bdc 100644 > > > --- a/tools/eficapsule.h > > > +++ b/tools/eficapsule.h > > > @@ -50,6 +50,14 @@ typedef struct { > > > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > > > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > > > > > +#define FW_ACCEPT_OS_GUID \ > > > + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > > > + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > > > + > > > +#define FW_REVERT_OS_GUID \ > > > + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ > > > + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) > > > + > > > /* flags */ > > > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > > > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > > > index 161affdd15..643da3849d 100644 > > > --- a/tools/mkeficapsule.c > > > +++ b/tools/mkeficapsule.c > > > @@ -29,6 +29,7 @@ > > > #include "eficapsule.h" > > > > > > static const char *tool_name = "mkeficapsule"; > > > +static unsigned char empty_capsule; > > > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > > efi_guid_t efi_guid_image_type_uboot_fit = > > > @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = > > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > > > #ifdef CONFIG_TOOLS_LIBCRYPTO > > > -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; > > > +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; > > > #else > > > -static const char *opts_short = "frg:i:I:v:h"; > > > +static const char *opts_short = "frg:i:I:v:hAR"; > > > #endif > > > > > > static struct option options[] = { > > > @@ -55,15 +56,23 @@ static struct option options[] = { > > > {"monotonic-count", required_argument, NULL, 'm'}, > > > {"dump-sig", no_argument, NULL, 'd'}, > > > #endif > > > + {"fw-accept", no_argument, NULL, 'A'}, > > > + {"fw-revert", no_argument, NULL, 'R'}, > > > {"help", no_argument, NULL, 'h'}, > > > {NULL, 0, NULL, 0}, > > > }; > > > > > > static void print_usage(void) > > > { > > > - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" > > > - "Options:\n" > > > + if (empty_capsule) { > > > + fprintf(stderr, "Usage: %s [options] <output file>\n", > > > + tool_name); > > > + } else { > > > + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", > > > + tool_name); > > > + } > > > > We should always show both cases regardless of "empty_capsule". > > > > Or if you have any restrictions on a combination of options, > > you'd better describe them more specifically in help message. > > Yes, there are restrictions on the combination of options. I will call > a separate function, like empty_capsule_usage for these. > > > > > I'd also like to encourage you to update the man page as well as uefi.rst. > > Okay > > > > > > + fprintf(stderr, "Options:\n" > > > "\t-f, --fit FIT image type\n" > > > "\t-r, --raw raw image type\n" > > > "\t-g, --guid <guid string> guid for image blob type\n" > > > @@ -75,8 +84,9 @@ static void print_usage(void) > > > "\t-m, --monotonic-count <count> monotonic count\n" > > > "\t-d, --dump_sig dump signature (*.p7)\n" > > > #endif > > > - "\t-h, --help print a help message\n", > > > - tool_name); > > > + "\t-A, --fw-accept firmware accept capsule\n" > > > + "\t-R, --fw-revert firmware revert capsule\n" > > > + "\t-h, --help print a help message\n"); > > > } > > > > > > /** > > > @@ -598,6 +608,59 @@ 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; > > > + int ret; > > > + 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 = NULL; > > > + ret = -1; > > > + > > > + f = fopen(path, "w"); > > > + if (!f) { > > > + printf("cannot open %s\n", path); > > > > To stderr as Heinrich has requested. > > I thought I saw an email from Heinrich in which he said that he did > not want a fprintf call, and was going to revert those hunks from your > patch. I will recheck this bit. I think that his said comment goes only against the help message. (I object it though.) > > > > > + goto err; > > > + } > > > + > > > + if (fw_accept) > > > + capsule_guid = fw_accept_guid; > > > + else > > > + capsule_guid = fw_revert_guid; > > > + > > > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > > > + header.header_size = sizeof(header); > > > + header.flags = 0; > > > + > > > + if (fw_accept) { > > > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > > > + } else { > > > + header.capsule_image_size = sizeof(header); > > > + } > > > > I wonder why we don't need GUID in revert case (and why need GUID > > in fw case. Since we want to add A/B update, there seems to be > > no ambiguity. > > The revert capsule is used not as a rejection of a specific individual > image, but for reverting the platform to the other bank. Which does > not require a image specific GUID. If so, why not apply the same rule to *accept* case to make the change permanent? -Takahiro Akashi > > > > > + 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 > > > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > > > unsigned char uuid_buf[16]; > > > unsigned long index, instance; > > > uint64_t mcount; > > > + unsigned char accept_fw_capsule, revert_fw_capsule; > > > char *privkey_file, *cert_file; > > > int c, idx; > > > > > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > > > mcount = 0; > > > privkey_file = NULL; > > > cert_file = NULL; > > > + accept_fw_capsule = 0; > > > + revert_fw_capsule = 0; > > > dump_sig = 0; > > > for (;;) { > > > c = getopt_long(argc, argv, opts_short, options, &idx); > > > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > > > dump_sig = 1; > > > break; > > > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > > > + case 'A': > > > + accept_fw_capsule = 1; > > > + break; > > > + case 'R': > > > + revert_fw_capsule = 1; > > > + break; > > > case 'h': > > > print_usage(); > > > exit(EXIT_SUCCESS); > > > } > > > } > > > > > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); > > > > Please check that two options are exclusive here. > > Okay > > > > > > /* check necessary parameters */ > > > - if ((argc != optind + 2) || !guid || > > > - ((privkey_file && !cert_file) || > > > + if ((!empty_capsule && argc != optind + 2) || > > > + (empty_capsule && argc != optind + 1) || > > > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > > > (!privkey_file && cert_file))) { > > > print_usage(); > > > exit(EXIT_FAILURE); > > > } > > > > > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > > > - mcount, privkey_file, cert_file) < 0) { > > > + if (empty_capsule) { > > > + if (create_empty_capsule(argv[argc - 1], guid, > > > + accept_fw_capsule ? 1 : 0) < 0) { > > > + printf("Creating empty capsule failed\n"); > > > > To stderr > > Okay, will check. > > -sughosh > > > > > -Takahiro Akashi > > > > > + exit(EXIT_FAILURE); > > > + } > > > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > > > + index, instance, mcount, privkey_file, > > > + cert_file) < 0) { > > > fprintf(stderr, "Creating firmware capsule failed\n"); > > > exit(EXIT_FAILURE); > > > } > > > -- > > > 2.17.1 > > >
Hi, 2022年1月24日(月) 11:08 AKASHI Takahiro <takahiro.akashi@linaro.org>: > > > > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > > > > + header.header_size = sizeof(header); > > > > + header.flags = 0; > > > > + > > > > + if (fw_accept) { > > > > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > > > > + } else { > > > > + header.capsule_image_size = sizeof(header); > > > > + } > > > > > > I wonder why we don't need GUID in revert case (and why need GUID > > > in fw case. Since we want to add A/B update, there seems to be > > > no ambiguity. > > > > The revert capsule is used not as a rejection of a specific individual > > image, but for reverting the platform to the other bank. Which does > > not require a image specific GUID. > > If so, why not apply the same rule to *accept* case to make the change > permanent? Perhaps, we can make a special "acceptance" capsule file which accept all image types. I guess originally it is considered to test each image by the image provider and each provider decides the firmware is acceptable or not. For example, TF-A test program passed the test but U-Boot test program doesn't. In that case, we may need a partial acceptance flags. However, at least the DeveloperBox has only one image type and maybe most of platforms doesn't want to split it. Thus the acceptance image-type uuid can be optional. Thank you, > > -Takahiro Akashi > > > > > > > > + 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 > > > > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > > > > unsigned char uuid_buf[16]; > > > > unsigned long index, instance; > > > > uint64_t mcount; > > > > + unsigned char accept_fw_capsule, revert_fw_capsule; > > > > char *privkey_file, *cert_file; > > > > int c, idx; > > > > > > > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > > > > mcount = 0; > > > > privkey_file = NULL; > > > > cert_file = NULL; > > > > + accept_fw_capsule = 0; > > > > + revert_fw_capsule = 0; > > > > dump_sig = 0; > > > > for (;;) { > > > > c = getopt_long(argc, argv, opts_short, options, &idx); > > > > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > > > > dump_sig = 1; > > > > break; > > > > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > > > > + case 'A': > > > > + accept_fw_capsule = 1; > > > > + break; > > > > + case 'R': > > > > + revert_fw_capsule = 1; > > > > + break; > > > > case 'h': > > > > print_usage(); > > > > exit(EXIT_SUCCESS); > > > > } > > > > } > > > > > > > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); > > > > > > Please check that two options are exclusive here. > > > > Okay > > > > > > > > > /* check necessary parameters */ > > > > - if ((argc != optind + 2) || !guid || > > > > - ((privkey_file && !cert_file) || > > > > + if ((!empty_capsule && argc != optind + 2) || > > > > + (empty_capsule && argc != optind + 1) || > > > > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > > > > (!privkey_file && cert_file))) { > > > > print_usage(); > > > > exit(EXIT_FAILURE); > > > > } > > > > > > > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > > > > - mcount, privkey_file, cert_file) < 0) { > > > > + if (empty_capsule) { > > > > + if (create_empty_capsule(argv[argc - 1], guid, > > > > + accept_fw_capsule ? 1 : 0) < 0) { > > > > + printf("Creating empty capsule failed\n"); > > > > > > To stderr > > > > Okay, will check. > > > > -sughosh > > > > > > > > -Takahiro Akashi > > > > > > > + exit(EXIT_FAILURE); > > > > + } > > > > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > > > > + index, instance, mcount, privkey_file, > > > > + cert_file) < 0) { > > > > fprintf(stderr, "Creating firmware capsule failed\n"); > > > > exit(EXIT_FAILURE); > > > > } > > > > -- > > > > 2.17.1 > > > > -- Masami Hiramatsu
hi Ilias, On Fri, 21 Jan 2022 at 18:30, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sughosh, > > On Thu, Jan 20, 2022 at 12:25:48AM +0530, Sughosh Ganu wrote: > > The Dependable Boot specification describes the structure of the > > firmware accept and revert capsules. These are empty capsules which > > are used for signalling the acceptance or rejection of the updated > > firmware by the OS. Add support for generating these empty capsules. > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> > > --- > > > > Changes since V2: > > * New patch for generating empty capsules > > > > tools/eficapsule.h | 8 ++++ > > tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- > > 2 files changed, 100 insertions(+), 10 deletions(-) > > > > diff --git a/tools/eficapsule.h b/tools/eficapsule.h > > index 8c1560bb06..6001952bdc 100644 > > --- a/tools/eficapsule.h > > +++ b/tools/eficapsule.h > > @@ -50,6 +50,14 @@ typedef struct { > > EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ > > 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) > > > > +#define FW_ACCEPT_OS_GUID \ > > + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ > > + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) > > + > > +#define FW_REVERT_OS_GUID \ > > + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ > > + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) > > + > > /* flags */ > > #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > > index 161affdd15..643da3849d 100644 > > --- a/tools/mkeficapsule.c > > +++ b/tools/mkeficapsule.c > > @@ -29,6 +29,7 @@ > > #include "eficapsule.h" > > > > static const char *tool_name = "mkeficapsule"; > > +static unsigned char empty_capsule; > > > > efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; > > efi_guid_t efi_guid_image_type_uboot_fit = > > @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = > > efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > #ifdef CONFIG_TOOLS_LIBCRYPTO > > -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; > > +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; > > #else > > -static const char *opts_short = "frg:i:I:v:h"; > > +static const char *opts_short = "frg:i:I:v:hAR"; > > #endif > > > > static struct option options[] = { > > @@ -55,15 +56,23 @@ static struct option options[] = { > > {"monotonic-count", required_argument, NULL, 'm'}, > > {"dump-sig", no_argument, NULL, 'd'}, > > #endif > > + {"fw-accept", no_argument, NULL, 'A'}, > > + {"fw-revert", no_argument, NULL, 'R'}, > > {"help", no_argument, NULL, 'h'}, > > {NULL, 0, NULL, 0}, > > }; > > > > static void print_usage(void) > > { > > - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" > > - "Options:\n" > > + if (empty_capsule) { > > + fprintf(stderr, "Usage: %s [options] <output file>\n", > > + tool_name); > > + } else { > > + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", > > + tool_name); > > + } > > > > + fprintf(stderr, "Options:\n" > > "\t-f, --fit FIT image type\n" > > "\t-r, --raw raw image type\n" > > "\t-g, --guid <guid string> guid for image blob type\n" > > @@ -75,8 +84,9 @@ static void print_usage(void) > > "\t-m, --monotonic-count <count> monotonic count\n" > > "\t-d, --dump_sig dump signature (*.p7)\n" > > #endif > > - "\t-h, --help print a help message\n", > > - tool_name); > > + "\t-A, --fw-accept firmware accept capsule\n" > > + "\t-R, --fw-revert firmware revert capsule\n" > > + "\t-h, --help print a help message\n"); > > } > > > > /** > > @@ -598,6 +608,59 @@ 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; > > + int ret; > > + 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 = NULL; > > + ret = -1; > > Can we init those at their declaration? Okay. I followed the initialisation style that was used in other functions. But I can init the variables like you mention. > > > + > > + f = fopen(path, "w"); > > + if (!f) { > > + printf("cannot open %s\n", path); > > + goto err; > > + } > > + > > + if (fw_accept) > > + capsule_guid = fw_accept_guid; > > + else > > + capsule_guid = fw_revert_guid; > > ternary operator would look better here. Okay > > > + > > + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); > > + header.header_size = sizeof(header); > > + header.flags = 0; > > Is it the flags only you need? Or maybe memset the entire headeri to 0? All members of the capsule header structure are being initialised. So no need to memset the struct to 0. > > + > > + if (fw_accept) { > > + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); > > + } else { > > + header.capsule_image_size = sizeof(header); > > + } > > ternary again? Okay > > > + > > + 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 > > @@ -616,6 +679,7 @@ int main(int argc, char **argv) > > unsigned char uuid_buf[16]; > > unsigned long index, instance; > > uint64_t mcount; > > + unsigned char accept_fw_capsule, revert_fw_capsule; > > char *privkey_file, *cert_file; > > int c, idx; > > > > @@ -625,6 +689,8 @@ int main(int argc, char **argv) > > mcount = 0; > > privkey_file = NULL; > > cert_file = NULL; > > + accept_fw_capsule = 0; > > + revert_fw_capsule = 0; > > dump_sig = 0; > > for (;;) { > > c = getopt_long(argc, argv, opts_short, options, &idx); > > @@ -691,22 +757,38 @@ int main(int argc, char **argv) > > dump_sig = 1; > > break; > > #endif /* CONFIG_TOOLS_LIBCRYPTO */ > > + case 'A': > > + accept_fw_capsule = 1; > > + break; > > + case 'R': > > + revert_fw_capsule = 1; > > + break; > > case 'h': > > print_usage(); > > exit(EXIT_SUCCESS); > > } > > } > > > > + empty_capsule = (accept_fw_capsule || revert_fw_capsule); > > Why do we need 3 variables here? > Would it be better to have an enum and just use a single variable like "is_accept_capsule"? For printing the usage, I need to differentiate between an accept capsule and revert capsule, since they take different options. If you don't have a strong view on this, I will keep it as is. -sughosh > > > + > > /* check necessary parameters */ > > - if ((argc != optind + 2) || !guid || > > - ((privkey_file && !cert_file) || > > + if ((!empty_capsule && argc != optind + 2) || > > + (empty_capsule && argc != optind + 1) || > > + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || > > (!privkey_file && cert_file))) { > > print_usage(); > > exit(EXIT_FAILURE); > > } > > > > - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, > > - mcount, privkey_file, cert_file) < 0) { > > + if (empty_capsule) { > > + if (create_empty_capsule(argv[argc - 1], guid, > > + accept_fw_capsule ? 1 : 0) < 0) { > > + printf("Creating empty capsule failed\n"); > > + exit(EXIT_FAILURE); > > + } > > + } else if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, > > + index, instance, mcount, privkey_file, > > + cert_file) < 0) { > > fprintf(stderr, "Creating firmware capsule failed\n"); > > exit(EXIT_FAILURE); > > } > > -- > > 2.17.1 > > > > > Thanks > /Ilias
diff --git a/tools/eficapsule.h b/tools/eficapsule.h index 8c1560bb06..6001952bdc 100644 --- a/tools/eficapsule.h +++ b/tools/eficapsule.h @@ -50,6 +50,14 @@ typedef struct { EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \ 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7) +#define FW_ACCEPT_OS_GUID \ + EFI_GUID(0x0c996046, 0xbcc0, 0x4d04, 0x85, 0xec, \ + 0xe1, 0xfc, 0xed, 0xf1, 0xc6, 0xf8) + +#define FW_REVERT_OS_GUID \ + EFI_GUID(0xacd58b4b, 0xc0e8, 0x475f, 0x99, 0xb5, \ + 0x6b, 0x3f, 0x7e, 0x07, 0xaa, 0xf0) + /* flags */ #define CAPSULE_FLAGS_PERSIST_ACROSS_RESET 0x00010000 diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index 161affdd15..643da3849d 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -29,6 +29,7 @@ #include "eficapsule.h" static const char *tool_name = "mkeficapsule"; +static unsigned char empty_capsule; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_image_type_uboot_fit = @@ -38,9 +39,9 @@ efi_guid_t efi_guid_image_type_uboot_raw = efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; #ifdef CONFIG_TOOLS_LIBCRYPTO -static const char *opts_short = "frg:i:I:v:p:c:m:dh"; +static const char *opts_short = "frg:i:I:v:p:c:m:dhAR"; #else -static const char *opts_short = "frg:i:I:v:h"; +static const char *opts_short = "frg:i:I:v:hAR"; #endif static struct option options[] = { @@ -55,15 +56,23 @@ static struct option options[] = { {"monotonic-count", required_argument, NULL, 'm'}, {"dump-sig", no_argument, NULL, 'd'}, #endif + {"fw-accept", no_argument, NULL, 'A'}, + {"fw-revert", no_argument, NULL, 'R'}, {"help", no_argument, NULL, 'h'}, {NULL, 0, NULL, 0}, }; static void print_usage(void) { - fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n" - "Options:\n" + if (empty_capsule) { + fprintf(stderr, "Usage: %s [options] <output file>\n", + tool_name); + } else { + fprintf(stderr, "Usage: %s [options] <image blob> <output file>\n", + tool_name); + } + fprintf(stderr, "Options:\n" "\t-f, --fit FIT image type\n" "\t-r, --raw raw image type\n" "\t-g, --guid <guid string> guid for image blob type\n" @@ -75,8 +84,9 @@ static void print_usage(void) "\t-m, --monotonic-count <count> monotonic count\n" "\t-d, --dump_sig dump signature (*.p7)\n" #endif - "\t-h, --help print a help message\n", - tool_name); + "\t-A, --fw-accept firmware accept capsule\n" + "\t-R, --fw-revert firmware revert capsule\n" + "\t-h, --help print a help message\n"); } /** @@ -598,6 +608,59 @@ 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; + int ret; + 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 = NULL; + ret = -1; + + f = fopen(path, "w"); + if (!f) { + printf("cannot open %s\n", path); + goto err; + } + + if (fw_accept) + capsule_guid = fw_accept_guid; + else + capsule_guid = fw_revert_guid; + + memcpy(&header.capsule_guid, &capsule_guid, sizeof(efi_guid_t)); + header.header_size = sizeof(header); + header.flags = 0; + + if (fw_accept) { + header.capsule_image_size = sizeof(header) + sizeof(efi_guid_t); + } else { + header.capsule_image_size = 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 @@ -616,6 +679,7 @@ int main(int argc, char **argv) unsigned char uuid_buf[16]; unsigned long index, instance; uint64_t mcount; + unsigned char accept_fw_capsule, revert_fw_capsule; char *privkey_file, *cert_file; int c, idx; @@ -625,6 +689,8 @@ int main(int argc, char **argv) mcount = 0; privkey_file = NULL; cert_file = NULL; + accept_fw_capsule = 0; + revert_fw_capsule = 0; dump_sig = 0; for (;;) { c = getopt_long(argc, argv, opts_short, options, &idx); @@ -691,22 +757,38 @@ int main(int argc, char **argv) dump_sig = 1; break; #endif /* CONFIG_TOOLS_LIBCRYPTO */ + case 'A': + accept_fw_capsule = 1; + break; + case 'R': + revert_fw_capsule = 1; + break; case 'h': print_usage(); exit(EXIT_SUCCESS); } } + empty_capsule = (accept_fw_capsule || revert_fw_capsule); + /* check necessary parameters */ - if ((argc != optind + 2) || !guid || - ((privkey_file && !cert_file) || + if ((!empty_capsule && argc != optind + 2) || + (empty_capsule && argc != optind + 1) || + (!revert_fw_capsule && !guid) || ((privkey_file && !cert_file) || (!privkey_file && cert_file))) { print_usage(); exit(EXIT_FAILURE); } - if (create_fwbin(argv[argc - 1], argv[argc - 2], guid, index, instance, - mcount, privkey_file, cert_file) < 0) { + if (empty_capsule) { + if (create_empty_capsule(argv[argc - 1], guid, + accept_fw_capsule ? 1 : 0) < 0) { + printf("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 describes the structure of the firmware accept and revert capsules. These are empty capsules which are used for signalling the acceptance or rejection of the updated firmware by the OS. Add support for generating these empty capsules. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V2: * New patch for generating empty capsules tools/eficapsule.h | 8 ++++ tools/mkeficapsule.c | 102 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 100 insertions(+), 10 deletions(-)