hi Ilias,
On Fri, 21 Jan 2022 at 18:30, Ilias Apalodimas
<ilias.apalodi...@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.g...@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
