From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 03C2DC433EF for ; Thu, 16 Jun 2022 01:01:45 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 283E483EF9; Thu, 16 Jun 2022 03:01:43 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="GltLpW4l"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1434C83F40; Thu, 16 Jun 2022 03:01:41 +0200 (CEST) Received: from mail-pl1-x629.google.com (mail-pl1-x629.google.com [IPv6:2607:f8b0:4864:20::629]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 74F1B83E3A for ; Thu, 16 Jun 2022 03:01:36 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pl1-x629.google.com with SMTP id k7so24119plg.7 for ; Wed, 15 Jun 2022 18:01:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=A6hnPQRoqnJL+v69ZtE9VSZNydTTLudzb3KWbLHlA/g=; b=GltLpW4lg7I9F0calMt1cyJGe9DgZbyRLdoAxXf3Dl77SDYCteCLsOvEj0xhg6JyCt TZQCFZ67NJhSqsQ+16n/xdVkM4hPv90SFvo4schCBjITuLA+LxlZrzPfytiKvaQFYzVN uljgv37HKDhoL73CuqSLYBDqMAr6MA6vp25RqXuIFZf2Dc66QEmqC/AyWSt/0qaCPXhs E4o9cIFEjgGUztOYnhPr+6WxF6lGac0fGr0v6/KGGXazvRvaRVAnV7dRhP/prY+uf3/5 UoL2YuoF2fWfNf7QeSf/QGGFfvJf1BNkI9nbZJSvSxodc9gYoUBWsqiDvaegGohFhR7w kRoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=A6hnPQRoqnJL+v69ZtE9VSZNydTTLudzb3KWbLHlA/g=; b=R7IHFppHg3hK5XpQ8x6OsFczqJqXjqDuADwmNF7U28slylZ1hlaHd+XlS7+Gsv4yKW Tu9j0WxPGgxAcLjgd67UmGs/apAE/JkuKKJ+MgBOXwWNLLKblCjwWZvmTV988f0OdS9l 1StHCKe4dcRaGBCSF/4SEPogiMyxHSx3EdK8dNZFdB4/NDtGSf1Y+lL71wtzFZt79U3P nwguIO6rdysHztEtJA/tK/RP2IpOA1lXVSX9p0HjxwGstLZCTcpMXwNpsdG3OAYgPoXq im/khmiPur6ieHJ2dKwKnmuAR1Nky3FoyOIVSLaZIrYCaCRdnOgiqWLBP2Wp3YYbYp+n GxHQ== X-Gm-Message-State: AJIora9L1KxXunnlBFFklPCQlPUbjSD3HOBcMBgC0YCZI3R0okPVNMr5 1ahkRpRJVrYI5vnZo2x99i7QRQ== X-Google-Smtp-Source: AGRyM1sgd7ml8imT1NrgamMTRtnMglw7sEvSMG6TMDVBIB4pgoFMz3iDBepFvIXLDQypPYTD/Vnhjw== X-Received: by 2002:a17:90b:3e88:b0:1e8:875e:366f with SMTP id rj8-20020a17090b3e8800b001e8875e366fmr13156629pjb.155.1655341294503; Wed, 15 Jun 2022 18:01:34 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:d0aa:98f9:9060:950b]) by smtp.gmail.com with ESMTPSA id b196-20020a6334cd000000b003fdfb59bd85sm232139pga.12.2022.06.15.18.01.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jun 2022 18:01:34 -0700 (PDT) Date: Thu, 16 Jun 2022 10:01:28 +0900 From: Takahiro Akashi To: Sughosh Ganu Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Ilias Apalodimas , Patrick Delaunay , Patrice Chotard , Simon Glass , Bin Meng , Tom Rini , Etienne Carriere , Michal Simek , Jose Marinho , Jassi Brar Subject: Re: [PATCH v5 11/23] mkeficapsule: Add support for generating empty capsules Message-ID: <20220616010128.GB52308@laputa> Mail-Followup-To: Takahiro Akashi , Sughosh Ganu , u-boot@lists.denx.de, Heinrich Schuchardt , Ilias Apalodimas , Patrick Delaunay , Patrice Chotard , Simon Glass , Bin Meng , Tom Rini , Etienne Carriere , Michal Simek , Jose Marinho , Jassi Brar References: <20220609123010.1017463-1-sughosh.ganu@linaro.org> <20220609123010.1017463-12-sughosh.ganu@linaro.org> <20220615051121.GB58082@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Sughosh, On Wed, Jun 15, 2022 at 04:19:56PM +0530, Sughosh Ganu wrote: > On Wed, 15 Jun 2022 at 10:41, Takahiro Akashi > 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 > > > --- > > > 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? > 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] \n" > > > - "Options:\n" > > > - > > > - "\t-g, --guid guid for image blob type\n" > > > - "\t-i, --index update image index\n" > > > - "\t-I, --instance update hardware instance\n" > > > - "\t-p, --private-key private key file\n" > > > - "\t-c, --certificate signer's certificate file\n" > > > - "\t-m, --monotonic-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] \n", > > > + tool_name); > > > + fprintf(stderr, "Options:\n" > > > + "\t-A, --fw-accept firmware accept capsule\n" > > > + "\t-g, --guid guid for image blob type\n" > > > + "\t-h, --help print a help message\n" > > > + ); > > > + } else { > > > + fprintf(stderr, "Usage: %s [options] \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] \n" > > > + "Options:\n" > > > + > > > + "\t-g, --guid guid for image blob type\n" > > > + "\t-i, --index update image index\n" > > > + "\t-I, --instance update hardware instance\n" > > > + "\t-p, --private-key private key file\n" > > > + "\t-c, --certificate signer's certificate file\n" > > > + "\t-m, --monotonic-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 > > >