[PATCH v5 11/23] mkeficapsule: Add support for generating empty capsules
Sughosh Ganu
sughosh.ganu at linaro.org
Fri Jun 17 10:01:11 CEST 2022
Takahiro,
On Fri, 17 Jun 2022 at 06:16, Takahiro Akashi
<takahiro.akashi at linaro.org> wrote:
>
> Sughosh,
>
> On Thu, Jun 16, 2022 at 12:42:08PM +0530, Sughosh Ganu wrote:
> > hi Takahiro,
> >
> > On Thu, 16 Jun 2022 at 06:31, Takahiro Akashi
> > <takahiro.akashi at linaro.org> wrote:
> > >
> > > Sughosh,
> > >
> > > On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote:
> > > > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi
> > > > <takahiro.akashi at linaro.org> wrote:
> > > > >
> > > > > On Thu, Jun 09, 2022 at 05:59:58PM +0530, 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.ganu at linaro.org>
> > > > > > ---
> > > > > > doc/mkeficapsule.1 | 29 ++++++---
> > > > > > tools/eficapsule.h | 8 +++
> > > > > > tools/mkeficapsule.c | 139 +++++++++++++++++++++++++++++++++++++------
> > > > > > 3 files changed, 151 insertions(+), 25 deletions(-)
> > > > > >
> > > > > > diff --git a/doc/mkeficapsule.1 b/doc/mkeficapsule.1
> > > > > > index 09bdc24295..77ca061efd 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
> > > > > >
> > > > > > .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.
> > > > > > @@ -36,18 +41,16 @@ multiple binary blobs in a single capsule file.
> > > > > > This type of image file can be generated by
> > > > > > .BR mkimage .
> > > > > >
> > > > > > -.PP
> > > > > > -If you want to use other types than above two, you should explicitly
> > > > > > -specify a guid for the FMP driver.
> > > > > > -
> > > > > > .SH "OPTIONS"
> > > > > > +
> > > > > > .TP
> > > > > > .BI "-g\fR,\fB --guid " guid-string
> > > > > > Specify guid for image blob type. The format is:
> > > > > > xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
> > > > > >
> > > > > > The first three elements are in little endian, while the rest
> > > > > > -is in big endian.
> > > > > > +is in big endian. The option must be specified for all non empty and
> > > > > > +image acceptance capsules
> > > > >
> > > > > "image acceptance" -> "firmware acceptance"
> > > >
> > > > Okay
> > > >
> > > > >
> > > > > I don't still understand why we need a guid for acceptance
> > > > > while revert doesn't require it.
> > > > > I believe that firmware update is "all or nothing", isn't it?
> > > >
> > > > I believe this gives more flexibility in that different components
> > > > might be required to accept the various firmware images. So, one
> > > > component might accept the optee_os, while another might be
> > > > responsible for accepting u-boot. In any case, we do check that all
> > > > the components have their accepted bit set, and only if so, does the
> > > > bank boot in the regular state.
> > >
> > > Probably I don't understand the behavior.
> > > Let's assume that we have firmware A and firmware B and then
> > > update both.
> > > When the firmware A is accepted and B is not (not yet issuing
> > > acceptance capsule) and I try to reboot the system, what happens?
> > > From which bank does the system boot, old one or new one?
> >
> > Once any/all of the images have been updated, on subsequent reboot,
> > the platform would boot in Trial State from the updated bank. I have
> > introduced an EFI variable, TrialeStateCtr for counting the number of
> > times the system is booting in the trial state. The system remains in
> > trial state as long as all the images from the updated bank have not
> > been accepted. The platform boots in trial state for a particular
> > number of iterations(configurable), and once that count has exceeded,
> > the active bank value gets changed to the previous_active_index, and
> > the platform subsequently boots from the other bank. If all the images
> > do get accepted while the platform is in trial state, the platform
> > transitions to the regular state, and continues booting from that
> > bank.
>
> Thank you for the details.
> But if I understand correctly, why do we need a partially-accepted status?
It's not so much about needing a partial update, but allowing for a
scenario where multiple components might be tasked with accepting
different firmware images. I see this as a kind of flexibility given.
I do appreciate your point of view to have a common format for accept
and reject capsules, but this format does not hurt IMO as long as it
is documented and explained.
> When some are accepted but others not, the system boots from an update
> bank in any way. Right?
> So at the end, all that we should do is to accept the boot "from a new bank"
> permanently (and transit to, what you say, regular state), or to let the system
> boot from an old bank (again, in a regular state).
>
> I don't see any good reason for having a partial acceptance.
>
> We should continue this discussion, but anyhow, you should think of
> adding such a description above in U-Boot doc.
Okay. Will do.
-sughosh
>
> -Takahiro Akashi
>
>
> > -sughosh
> >
> > >
> > > > In case of a firmware revert, it would
> > > > not matter which firmware component is being reverted -- the platform
> > > > would simply need to boot from the other bank. Do you see any issue
> > > > with the current method that we have?
> > > >
> > > > >
> > > > > If there is a good reason, please describe a possible/expected
> > > > > scenario.
> > > >
> > > > Where do you want me to explain this, in the feature documentation? Or
> > > > do you think this can be elaborated in greater detail in the spec.
> > >
> > > I prefer some explanation in U-Boot doc.
> > >
> > > > >
> > > > > > .TP
> > > > > > .BI "-i\fR,\fB --index " index
> > > > > > @@ -57,6 +60,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
> > > > > > +.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 d63b831443..072a4b5598 100644
> > > > > > --- a/tools/eficapsule.h
> > > > > > +++ b/tools/eficapsule.h
> > > > > > @@ -41,6 +41,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 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",
> > > > > > + 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-R, --fw-revert firmware revert capsule\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));
> > > > >
> > > > > -> guidcpy()
> > > >
> > > > This being a host tool, guidcpy cannot be used. You have used memcpy
> > > > in the create_fwbin() for the same reason I guess.
> > >
> > > Ah, right.
> > >
> > > > >
> > > > > > + 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));
> > > > >
> > > > > ditto
> > > >
> > > > Same as above.
> > > >
> > > > >
> > > > > > + 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':
> > > > > > 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);
> > > > > > +
> > > > >
> > > > > So empty_capsule is redundant as empty_capsule is equivalent with
> > > > > "capsule == CAPSULE_NORMAL_BLOB".
> > > > > I think that a single variable, say capsule_type, is enough.
> > > >
> > > > I was using empty_capsule primarily to make the check done below look
> > > > more succinct and readable. But I can change that to capsule !=
> > > > CAPSULE_NORMAL_BLOB.
> > > >
> > > > >
> > > > > > /* 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)))) {
> > > > > > 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) {
> > > > >
> > > > > if (capsule_type != CAPSULE_NORMAL_BLOB)
> > > > > create_empty_capsule(..., capsule_type == CAPSULE_ACCEPT);
> > > > >
> > > > > Simple is the best :)
> > > >
> > > > Common, please don't tell me that the code above is complicated. Just
> > > > that we can do without the empty_capsule variable, yes.
> > >
> > > Sorry, not complicated, but having two variables make little sense.
> > >
> > > -Takahiro Akashi
> > >
> > > > -sughosh
> > > >
> > > > >
> > > > > -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.25.1
> > > > > >
More information about the U-Boot
mailing list