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 4EF2AC433F5 for ; Mon, 24 Jan 2022 02:08:52 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7E357811DF; Mon, 24 Jan 2022 03:08:49 +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="d7unLl8z"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id DA23A8192E; Mon, 24 Jan 2022 03:08:46 +0100 (CET) Received: from mail-pg1-x536.google.com (mail-pg1-x536.google.com [IPv6:2607:f8b0:4864:20::536]) (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 E67BC81109 for ; Mon, 24 Jan 2022 03:08:42 +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-pg1-x536.google.com with SMTP id 187so13658692pga.10 for ; Sun, 23 Jan 2022 18:08:42 -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=zXvYGr2Med3cslcbO/6JX2T+/mQfb81qoYSzy+L0QVw=; b=d7unLl8zHH/YgIOVzDMSSLKIVm7swPsF6im8ed9ikqYmHgzwX2eGcip2NwWRBt3OfC PbFEeX3RO+XZjVOXbJnvTVzg95aRlF5UNQzxk3Rxs7/gLzT0q8AZrWBYIEd2cDHv7mQ2 wSocRDP1eIxTNCjDU5xoMdFf3TqnN3gGDpo3TvxHlEgT8W+Osa2WpxSkIDb2DL6qVWeG ntK+bcVugbkjTNunxdP+1Uk25Dk3gZxAprrnfPMoMeJGJGEzf0EUzQ8hQ5QmL6IT4O4N KPlMWXk4dRwr5E+yA6s/U2/DyiuzYPyNADSbMCPIeFHW1Ggt7D0WVSZMidTGwKJfTR9N Fasg== 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=zXvYGr2Med3cslcbO/6JX2T+/mQfb81qoYSzy+L0QVw=; b=WaAxZwYPteGY1YddQzM+rwbb3+e1Qq8xrx3p6J4Y2qeGhnEUYaDiDpj0ELtcACqz3m MCnrPcJzffO3JZFzGUMqJfzz/Uci438du7Cfrje/ELIYYw5imp4QaCoZVz7xgPVYEehL Lptdbk74pGNIve1YQ6jNDe+kkO7thJG68Qbc1Wo7RgLiQqOwePFG12jMeRzymZwHFbQC vSr4GpjU+a+RBhd9iwwAUG4ygvVCd92uZKsup4x91vTJAFJNn6GSIalg1EFh87SWt6gb 5Bz1X3NGT6uGjrP4IZvprjxy5JJB2Z1uW+eOSvtzdtxEsdw9IBqDSd5AZvP4JnQk20qp bOfg== X-Gm-Message-State: AOAM532RDePJkGgxdN24ynQ41vf0bAhNsN1NMFQvfnJo711RPIU0XwbT HrhAXilKCKBOswlSw8qdBgvuuw== X-Google-Smtp-Source: ABdhPJxXoX2+9cffy7G42sqkPqOor8L3cloAMxhjNPQkcixgW+FHwrs1d7ImdjRKpW70V6y1vFYw6A== X-Received: by 2002:a05:6a00:1a15:b0:4c1:ec02:640 with SMTP id g21-20020a056a001a1500b004c1ec020640mr12360144pfv.81.1642990120978; Sun, 23 Jan 2022 18:08:40 -0800 (PST) Received: from laputa ([2400:4050:c3e1:100:db1:8b95:c482:df85]) by smtp.gmail.com with ESMTPSA id l15sm10016422pgh.27.2022.01.23.18.08.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 23 Jan 2022 18:08:40 -0800 (PST) Date: Mon, 24 Jan 2022 11:08:35 +0900 From: AKASHI Takahiro To: Sughosh Ganu Cc: u-boot@lists.denx.de, Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Heinrich Schuchardt , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere Subject: Re: [RFC PATCH v3 9/9] mkeficapsule: Add support for generating empty capsules Message-ID: <20220124020835.GA48616@laputa> Mail-Followup-To: AKASHI Takahiro , Sughosh Ganu , u-boot@lists.denx.de, Masami Hiramatsu , Patrick Delaunay , Patrice Chotard , Heinrich Schuchardt , Alexander Graf , Simon Glass , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Tom Rini , Etienne Carriere References: <20220119185548.16730-1-sughosh.ganu@linaro.org> <20220119185548.16730-10-sughosh.ganu@linaro.org> <20220120021344.GB42867@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 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 > 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 > > > --- > > > > > > 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] \n" > > > - "Options:\n" > > > + if (empty_capsule) { > > > + fprintf(stderr, "Usage: %s [options] \n", > > > + tool_name); > > > + } else { > > > + fprintf(stderr, "Usage: %s [options] \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 for image blob type\n" > > > @@ -75,8 +84,9 @@ static void print_usage(void) > > > "\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 +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 > > >