Re: [RFC PATCH v3 9/9] mkeficapsule: Add support for generating empty capsules

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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
> > >