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 05610C433EF for ; Wed, 9 Feb 2022 03:05:22 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 37C7583D81; Wed, 9 Feb 2022 04:05:20 +0100 (CET) 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="FDOPx8Co"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2648583DA2; Wed, 9 Feb 2022 04:05:19 +0100 (CET) Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) (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 A1F8F83D2F for ; Wed, 9 Feb 2022 04:05:14 +0100 (CET) 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-pj1-x1036.google.com with SMTP id v13-20020a17090ac90d00b001b87bc106bdso3798045pjt.4 for ; Tue, 08 Feb 2022 19:05:14 -0800 (PST) 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=WGXcp8mnuN0gQnILeISZRkBsO/eAPLwdTaaJFSpe+zw=; b=FDOPx8Co1sCoIvjB/ICN572tsHyXseBWyUVwvkXwAyPJHTFmN72GX24KY5gwqQbs0X kkMTt7DzQyxxGjzh5Y+gtgsbqsNJiSQHaZtn83KDzSY44kTxTL3/qTAmMUQvWi2pUqDK XXUViI/5D97dlgIYZpLENW0Ts8U/6D5vxr7F66SHKIC3L5zqKuszBYl4JXWLvYXtfSaN FJ0IKRUbMAiH8jUTzePls9uBHcJY+VhkLzlrmjOez2nnXv4NFnM743R8eNqidWNRnQj+ npHN9UOspGgEaAATgB4NaxMch4lcvv5eG9UesNccFxsgmmd76NECosApp6gsCyDCUQzD TezQ== 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=WGXcp8mnuN0gQnILeISZRkBsO/eAPLwdTaaJFSpe+zw=; b=r+dow0S2yIWAghqzwcIqvrJU2lB8soVwRsJ870dO7CXvfquxrzZOZ3CvCf0zE+mr65 lcuBVDpvd0YA38OdM+7AXzkSaAqcDCVhf8v/yQ92GwE45/mOhNGf+sU6p5BMNvQNbcYt xEH6BGY2XZiGwKYTEuDHOUWNiAuwiNrKklJowve+FUVVlkVytQe+e/H4WN/FhwZBao9Z VJbP/J4eskTzs6N4zu8l5VI34SuAm7JfzqBw9L3Lvn2RfmCfp94xUFFfcIIleFgMSc2+ LIOu6qqdcal9MTG4UOCTsVqqScGUH9nzdp2jLsvzAJY2oqTMSOUOaMNZuP6zYEeg6bGN DZow== X-Gm-Message-State: AOAM533Nn6/jXnKSWILfP+lS9labchr45xWRSo8wl8Tnwkgys9NwYFlt QOLlSABmq6nFJ0nqhu6my7HPQg== X-Google-Smtp-Source: ABdhPJwkPkiJeaklcgKG2wrccIuhAm/EGDX0F622mtG0P43+Gpm2npAkK5bvj33O3Hmxw0CVeuNRyg== X-Received: by 2002:a17:902:d4ca:: with SMTP id o10mr184157plg.28.1644375912649; Tue, 08 Feb 2022 19:05:12 -0800 (PST) Received: from laputa (p914133-ipoe.ipoe.ocn.ne.jp. [153.243.15.132]) by smtp.gmail.com with ESMTPSA id b10sm17977962pfv.31.2022.02.08.19.05.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Feb 2022 19:05:12 -0800 (PST) Date: Wed, 9 Feb 2022 12:05:06 +0900 From: AKASHI Takahiro To: Sughosh Ganu Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere Subject: Re: [PATCH v4 10/11] mkeficapsule: Add support for generating empty capsules Message-ID: <20220209030506.GA26765@laputa> Mail-Followup-To: AKASHI Takahiro , Sughosh Ganu , u-boot@lists.denx.de, Heinrich Schuchardt , Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere References: <20220207182001.31270-1-sughosh.ganu@linaro.org> <20220207182001.31270-11-sughosh.ganu@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220207182001.31270-11-sughosh.ganu@linaro.org> 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 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 > --- > > 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. => .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] \n" > - "Options:\n" > - > - "\t-f, --fit FIT image type\n" > - "\t-r, --raw raw image type\n" > - "\t-g, --guid guid for image blob type\n" > - "\t-i, --index update image index\n" > - "\t-I, --instance update hardware instance\n" > + if (empty_capsule) { > + if (accept_fw_capsule) { > + 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" 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] \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", > + tool_name); > + fprintf(stderr, "Options:\n" > + "\t-f, --fit FIT image type\n" > + "\t-r, --raw raw image type\n" > + "\t-g, --guid guid for image blob type\n" > + "\t-i, --index update image index\n" > + "\t-I, --instance update hardware instance\n" > #ifdef CONFIG_TOOLS_LIBCRYPTO > - "\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-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" > #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)) ... > 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 >