On Wed, Feb 09, 2022 at 12:05:06PM +0900, AKASHI Takahiro wrote:
> Hi Sughosh,
>
> On Mon, Feb 07, 2022 at 11:50:00PM +0530, Sughosh Ganu wrote:
> > The Dependable Boot specification describes the structure of the
>
> What is this specification? Please specify the link to the doc.
>
> > 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 V3:
> > * Add related documentation for empty capsules in the mkeficapsule man
> > page.
> > * Add separate usage for empty capsules, with corresponding valid
> > options.
> > * Use ternary operators where possible.
> > * Put a exclusivity check for the empty capsule options.
> >
> > doc/mkeficapsule.1 | 23 +++++++-
> > tools/eficapsule.h | 8 +++
> > tools/mkeficapsule.c | 131 ++++++++++++++++++++++++++++++++++++-------
> > 3 files changed, 139 insertions(+), 23 deletions(-)
> >
> > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
> > index 8babb27ee8..75fc15906a 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
>
> With this formatting, "capsule-file" will get italic.
oops, I meant to say "roman."
> => .RI [ options "] [" image-blob "] " capsule-file
>
> Right?
>
> Furthermore, I think we can describe the command syntax of the two
> different cases (normal or empty capsule) more specifically.
>
> >
> > .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.
> > @@ -43,7 +48,7 @@ specify a guid for the FMP driver.
> > .SH "OPTIONS"
> > One of
> > .BR --fit ", " --raw " or " --guid
> > -option must be specified.
> > +option must be specified for non empty capsules.
> >
> > .TP
> > .BR -f ", " --fit
> > @@ -69,6 +74,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
>
> I don't still understand why we need GUID for accept empty capsule.
> We should have only one choice, whether all the new firmware be
> permanently applied or completely reverted.
>
> That's A/B update, isn't it?
>
> > +.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 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..e5dbec3a92 100644
> > --- a/tools/mkeficapsule.c
> > +++ b/tools/mkeficapsule.c
> > @@ -29,6 +29,7 @@
> > #include "eficapsule.h"
> >
> > static const char *tool_name = "mkeficapsule";
> > +unsigned char accept_fw_capsule, revert_fw_capsule, empty_capsule;
>
> Bool? but those variables are redundant.
>
> As Ilias suggested, introducing a new enum type here can
> simplify the code in the following code.
> enum {
> CAPSULE_NORMAL_BLOB = 0,
> CAPSULE_ACCEPT,
> CAPSULE_REVERT,
> } capsule_type;
>
>
> >
> > 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
>
> Please rebase your patch to my v10 or later.
> I have already removed the dependency on openssl library.
>
> > -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,28 +56,50 @@ 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"
> > -
> > - "\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"
> > - "\t-i, --index <index> update image index\n"
> > - "\t-I, --instance <instance> update hardware instance\n"
> > + if (empty_capsule) {
> > + if (accept_fw_capsule) {
> > + 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"
>
> While I doubt the necessity of "--guid,"
> why not accept "-f" or "-r" as a guid of image blob type?
> (It seems that your actual code does.)
>
> > + "\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",
> > + 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"
> > + "\t-i, --index <index> update image index\n"
> > + "\t-I, --instance <instance> update hardware
> > instance\n"
> > #ifdef CONFIG_TOOLS_LIBCRYPTO
> > - "\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-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"
> > #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 +621,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
> > @@ -625,6 +692,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 +760,44 @@ 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);
> > }
> > }
> >
> > + if (accept_fw_capsule && revert_fw_capsule) {
> > + fprintf(stderr,
> > + "Select either of Accept or Revert capsule
> > generation\n");
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + 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))) {
>
> Well, the error condition looks complicated due to mixing two cases
> and can be hard to maintain in the future. How about
> if (!empty_capsule &&
> ((argc != optind + 2) || !guid ||
> ((privkey_file && !cert_file) ||
> (!privkey_file && cert_file))) ||
> empty_capsule &&
> ((argc != optind + 1) ||
> (accept_fw_capsule && revert_fw_capsule) ||
> (accept_fw_capsule && !guid)) # arguable as mentioned above
> (revert_fw_capsule && guid))
> ...
I've got one concern here; Can we sign an empty capsule file?
I think we should.
If so, the help message (by print_usage()) doesn't reflect it.
-Takahiro Akashi
>
> > 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) {
>
> The third argument can be simplified to "accept_fw_capsule".
>
> -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.17.1
> >
