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 306E9C47DD9 for ; Sat, 30 Mar 2024 07:21:01 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 6D76188275; Sat, 30 Mar 2024 08:20:59 +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="U3PRkUN8"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6682E88276; Sat, 30 Mar 2024 08:20:58 +0100 (CET) Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) (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 68A3F88150 for ; Sat, 30 Mar 2024 08:20:55 +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=ilias.apalodimas@linaro.org Received: by mail-lj1-x22d.google.com with SMTP id 38308e7fff4ca-2d4515ec3aaso21322301fa.1 for ; Sat, 30 Mar 2024 00:20:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1711783255; x=1712388055; darn=lists.denx.de; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=xf1Z7lGI8RgFDZS6LJrIyRqBBecDy5dFmccwjNTmM2s=; b=U3PRkUN8Roe1Kab7ihEPEwr2lDOtgFSXiRLIQm2kPtMVnSBLYwSoAtPp4rh5a/kuZZ Y/3PboWj1dk/E3N9dVzctzgt9qyy9Y4Zsk8btchoikR2HKnXoervryT8pI0i+3CHOpqk 81y3UmMOk/c8gCtpmm529xHsA0y/YZ7dMqbT4VHRW10xerQ89k0ep3c4xCfxvcJH8d6y 0cj8mTLeIraTIImCrFNRqzUyre+4g2XLxUXE+byt20ARrZH0ej2Rtv6tZPoB9PGK9C5I zNksQPgUCBxJVcca8Y7zI8V4myNtV5bmS2iJlqOXNWmS5rwGWxBLKFRYZ3/Hu1vlr07H UkXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1711783255; x=1712388055; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=xf1Z7lGI8RgFDZS6LJrIyRqBBecDy5dFmccwjNTmM2s=; b=SqbYIKfpgkK0zF6cur0I8pT4sepRGPLl7ZtqVEqRYvDnDxyf/uqaLNEYph6tEsgCCt pbnM8vwRKMz0KUSpQcTQavKT5MN1skm/CxejRT2T5rG4mvEwECPw9DCOO300Iv/OhgPS Su4uhoYSNQG/5CMz/mbZT7zke8f+MhyK5n5NnUdCWPtkx9stMy6tZ+22DRIe3bhxtqu9 hWj/xKqdiTUnpKqBlkeJTz+gBZs0mJ8j1Ph6XcBTZcamf6oMsUqa1adhvXu36mRivNiz OymDXOknN+wIQ1RTiMvuem0fEXPqKlYw5mWLkNqE57Qo75x+RiK4QlVYh6FJkGezDbFC bNAA== X-Gm-Message-State: AOJu0Yym2y9qx9lQrLPUj5L4UsaXJAghJEvpWvjd0SlKmQA69Cbrp9P2 Bko3lWYhUcV2DkuCdOubCuY1VbcvVH8pW5/SqDr8oshhlbH7IZ8vAMM46zFZzUozDJZfB51NnJR NsQLSLGCDvS3C0O3DQ2rrNU+SuCqelE9Y0EsE9QQDVp61B5rb X-Google-Smtp-Source: AGHT+IH/g7/CC+RdbFcpshNC0TouGD5+5ddG7oJ0QgM399xZEUOIXxkXvxOcTMvAuXbGnwQknFhOVUjgvsJOvgxkkI8= X-Received: by 2002:a2e:81c5:0:b0:2d7:11:6793 with SMTP id s5-20020a2e81c5000000b002d700116793mr1607473ljg.7.1711783254418; Sat, 30 Mar 2024 00:20:54 -0700 (PDT) MIME-Version: 1.0 References: <20240328170110.1960631-1-tharvey@gateworks.com> In-Reply-To: From: Ilias Apalodimas Date: Sat, 30 Mar 2024 09:20:18 +0200 Message-ID: Subject: Re: [PATCH v2] tpm-v2: allow algoirthm name to be configured for pcr_read and pcr_extend To: Tim Harvey Cc: u-boot@lists.denx.de, Eddie James , slee@gateworks.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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.8 at phobos.denx.de X-Virus-Status: Clean On Sat, 30 Mar 2024 at 09:15, Ilias Apalodimas wrote: > > Hi Tim > > On Sat, 30 Mar 2024 at 02:40, Tim Harvey wrote: > > > > On Thu, Mar 28, 2024 at 11:18=E2=80=AFPM Ilias Apalodimas > > wrote: > > > > > > Hi Tim, > > > > > > On Thu, 28 Mar 2024 at 19:01, Tim Harvey wrot= e: > > > > > > > > For pcr_read and pcr_extend commands allow the digest algorithm to = be > > > > specified by an additional argument. If not specified it will defau= lt to > > > > SHA256 for backwards compatibility. > > > > > > > > A follow-on to this could be to extend all PCR banks with the detec= ted > > > > algo when the argument is 'auto'. > > > > > > > > Signed-off-by: Tim Harvey > > > > --- > > > > v2: > > > > - use tpm2_algorithm_to_len > > > > - use enum tpm2_algorithms > > > > - make function names and parameter names more consistent with exi= sting > > > > tpm-v2 functions > > > > - fix various spelling errors > > > > --- > > > > cmd/tpm-v2.c | 49 ++++++++++++++++++++++++++++++++++----------= ---- > > > > include/tpm-v2.h | 18 ++++++++++++++++++ > > > > lib/tpm-v2.c | 34 +++++++++++++++++++++++++++++++++ > > > > 3 files changed, 87 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c > > > > index 7e479b9dfe36..2343b4d9cb9e 100644 > > > > --- a/cmd/tpm-v2.c > > > > +++ b/cmd/tpm-v2.c > > > > @@ -99,11 +99,19 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *c= mdtp, int flag, int argc, > > > > struct tpm_chip_priv *priv; > > > > u32 index =3D simple_strtoul(argv[1], NULL, 0); > > > > void *digest =3D map_sysmem(simple_strtoul(argv[2], NULL, 0= ), 0); > > > > + int algo =3D TPM2_ALG_SHA256; > > > > + int algo_len; > > > > int ret; > > > > u32 rc; > > > > > > > > - if (argc !=3D 3) > > > > + if (argc < 3 || argc > 4) > > > > return CMD_RET_USAGE; > > > > + if (argc =3D=3D 4) { > > > > + algo =3D tpm2_name_to_algorithm(argv[3]); > > > > + if (algo < 0) > > > > + return CMD_RET_FAILURE; > > > > + } > > > > + algo_len =3D tpm2_algorithm_to_len(algo); > > > > > > > > ret =3D get_tpm(&dev); > > > > if (ret) > > > > @@ -116,8 +124,12 @@ static int do_tpm2_pcr_extend(struct cmd_tbl *= cmdtp, int flag, int argc, > > > > if (index >=3D priv->pcr_count) > > > > return -EINVAL; > > > > > > > > - rc =3D tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, digest, > > > > - TPM2_DIGEST_LEN); > > > > + rc =3D tpm2_pcr_extend(dev, index, algo, digest, algo_len); > > > > + if (!rc) { > > > > + printf("PCR #%u extended with %d byte %s digest\n",= index, > > > > + algo_len, tpm2_algorithm_name(algo)); > > > > + print_byte_string(digest, algo_len); > > > > + } > > > > > > > > unmap_sysmem(digest); > > > > > > > > @@ -127,15 +139,23 @@ static int do_tpm2_pcr_extend(struct cmd_tbl = *cmdtp, int flag, int argc, > > > > static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int ar= gc, > > > > char *const argv[]) > > > > { > > > > + enum tpm2_algorithms algo =3D TPM2_ALG_SHA256; > > > > struct udevice *dev; > > > > struct tpm_chip_priv *priv; > > > > u32 index, rc; > > > > + int algo_len; > > > > unsigned int updates; > > > > void *data; > > > > int ret; > > > > > > > > - if (argc !=3D 3) > > > > + if (argc < 3 || argc > 4) > > > > return CMD_RET_USAGE; > > > > + if (argc =3D=3D 4) { > > > > + algo =3D tpm2_name_to_algorithm(argv[3]); > > > > + if (algo < 0) > > > > + return CMD_RET_FAILURE; > > > > + } > > > > + algo_len =3D tpm2_algorithm_to_len(algo); > > > > > > > > ret =3D get_tpm(&dev); > > > > if (ret) > > > > @@ -151,11 +171,12 @@ static int do_tpm_pcr_read(struct cmd_tbl *cm= dtp, int flag, int argc, > > > > > > > > data =3D map_sysmem(simple_strtoul(argv[2], NULL, 0), 0); > > > > > > > > - rc =3D tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2= _ALG_SHA256, > > > > - data, TPM2_DIGEST_LEN, &updates); > > > > + rc =3D tpm2_pcr_read(dev, index, priv->pcr_select_min, algo= , > > > > + data, algo_len, &updates); > > > > if (!rc) { > > > > - printf("PCR #%u content (%u known updates):\n", ind= ex, updates); > > > > - print_byte_string(data, TPM2_DIGEST_LEN); > > > > + printf("PCR #%u %s %d byte content (%u known update= s):\n", index, > > > > + tpm2_algorithm_name(algo), algo_len, updates= ); > > > > + print_byte_string(data, algo_len); > > > > } > > > > > > > > unmap_sysmem(data); > > > > @@ -415,14 +436,14 @@ U_BOOT_CMD(tpm2, CONFIG_SYS_MAXARGS, 1, do_tp= m, "Issue a TPMv2.x command", > > > > " is one of:\n" > > > > " * TPM2_RH_LOCKOUT\n" > > > > " * TPM2_RH_PLATFORM\n" > > > > -"pcr_extend \n" > > > > -" Extend PCR # with digest at .\n" > > > > +"pcr_extend []\n" > > > > +" Extend PCR # with digest at with digest_al= go.\n" > > > > " : index of the PCR\n" > > > > -" : address of a 32-byte SHA256 digest\n" > > > > -"pcr_read \n" > > > > -" Read PCR # to memory address .\n" > > > > +" : address of digest of digest_algo type (default= s to SHA256)\n" > > > > +"pcr_read []\n" > > > > +" Read PCR # to memory address with .\n" > > > > " : index of the PCR\n" > > > > -" : address to store the a 32-byte SHA256 digest\n= " > > > > +" : address of digest of digest_algo type (default= s to SHA256)\n" > > > > "get_capability \n" > > > > " Read and display entries indexed by /.\n" > > > > " Values are 4 bytes long and are written at .\n" > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h > > > > index 33dd103767c4..933882fcbf97 100644 > > > > --- a/include/tpm-v2.h > > > > +++ b/include/tpm-v2.h > > > > @@ -965,4 +965,22 @@ u32 tpm2_enable_nvcommits(struct udevice *dev,= uint vendor_cmd, > > > > */ > > > > u32 tpm2_auto_start(struct udevice *dev); > > > > > > > > +/** > > > > + * tpm2_name_to_algorithm() - Return an algorithm id given a suppo= rted > > > > + * algorithm name > > > > + * > > > > + * @name: algorithm name > > > > + * Return: enum tpm2_algorithms or -EINVAL > > > > + */ > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name); > > > > + > > > > +/** > > > > + * tpm2_algorithm_name() - Return an algorithm name string for a > > > > + * supported algorithm id > > > > + * > > > > + * @algorithm_id: algorithm defined in enum tpm2_algorithms > > > > + * Return: algorithm name string or "" > > > > + */ > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms); > > > > + > > > > #endif /* __TPM_V2_H */ > > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > > > index 68eaaa639f89..19b2535682e1 100644 > > > > --- a/lib/tpm-v2.c > > > > +++ b/lib/tpm-v2.c > > > > @@ -29,6 +29,13 @@ const enum tpm2_algorithms tpm2_supported_algori= thms[4] =3D { > > > > TPM2_ALG_SHA512, > > > > }; > > > > > > > > +const char *tpm2_supported_algorithm_names[4] =3D { > > > > + "sha1", > > > > + "sha256", > > > > + "sha384", > > > > + "sha512", > > > > +}; > > > > + > > > > > > Overall this looks correct(but we don't need the [4] in the definitio= n > > > tbh). However, keeping the info in 2 different places is not ideal. > > > We used to have a nice struct array that had all the info tightly tog= ether [0]. > > > > > > The struct looked like > > > > > > struct digest_info { > > > u16 hash_alg; > > > u32 hash_mask; > > > u16 hash_len; > > > }; > > > > > > static const struct digest_info hash_algo_list[] =3D { > > > { > > > TPM2_ALG_SHA1, > > > EFI_TCG2_BOOT_HASH_ALG_SHA1, > > > TPM2_SHA1_DIGEST_SIZE, > > > }, > > > { > > > TPM2_ALG_SHA256, > > > EFI_TCG2_BOOT_HASH_ALG_SHA256, > > > TPM2_SHA256_DIGEST_SIZE, > > > }, > > > { > > > TPM2_ALG_SHA384, > > > EFI_TCG2_BOOT_HASH_ALG_SHA384, > > > TPM2_SHA384_DIGEST_SIZE, > > > }, > > > { > > > TPM2_ALG_SHA512, > > > EFI_TCG2_BOOT_HASH_ALG_SHA512, > > > TPM2_SHA512_DIGEST_SIZE, > > > }, > > > }; > > > > > > In short, we need to > > > - define the algorithm registry as defined by the TCG spec > > > /* Algorithm Registry */ > > > #define EFI_TCG2_BOOT_HASH_ALG_SHA1 0x00000001 > > > #define EFI_TCG2_BOOT_HASH_ALG_SHA256 0x00000002 > > > #define EFI_TCG2_BOOT_HASH_ALG_SHA384 0x00000004 > > > #define EFI_TCG2_BOOT_HASH_ALG_SHA512 0x00000008 > > > #define EFI_TCG2_BOOT_HASH_ALG_SM3_256 0x00000010 > > > > > > > the above defines do not make sense to me if they are truely supposed > > to be masks compared to what we currently have: > > > > enum tpm2_algorithms { > > TPM2_ALG_SHA1 =3D 0x04, > > TPM2_ALG_XOR =3D 0x0A, > > TPM2_ALG_SHA256 =3D 0x0B, > > TPM2_ALG_SHA384 =3D 0x0C, > > TPM2_ALG_SHA512 =3D 0x0D, > > TPM2_ALG_NULL =3D 0x10, > > TPM2_ALG_SM3_256 =3D 0x12, > > }; > > #define tpm2_algorithm_to_mask(a) (1 << (a)) > > Yes, this is a bit confusing. We are *not* using those definitions > when calling tpm2_algorithm_to_mask(). Instead const enum of > tpm2_supported_algorithms[] is used which yields the same values. > > > > > Is digest_info.hash_mask from 2023.04 something else or were those > > defines just wrong (struct digest_info didn't seem to be used anywhere > > in 2023.04) > > It's probably present in 2022.X versions. The link included in the > mail is from 2022.01 > https://source.denx.de/u-boot/custodians/u-boot-tpm/-/blob/v2022.01-rc3/l= ib/efi_loader/efi_tcg2.c?ref_type=3Dtags#L75 > > > > > I'm ok with changing tpm2_supported_algorithms to a struct which > > defines name, algorithm_id, mask and len if I understand what values > > they should be. > > I hope that's clear now. Basically if you checkout v2023.01 all the > functions are in lib/efi_loader/efi_tcg.c > You will need > > static const struct digest_info hash_algo_list[], alg_to_len and > alg_to_mask functions. You can then add a new const char filed with > the names you want to support in the command line > > Thanks for picking this up! > > /Ilias > > > > > Sounds like your suggesting: > > - add back struct digest_info > > - make tpm2_supported_algorithms be an array of struct digtest_info > > - use that array for tpm2_algorithm_to_len, tpm2_algorithm_to_mask, > > tpm2_algorithm_to_len and tpm2_name_to_algorithm Forgot to answer this one - add back struct digest_info - get rid of supported algorithms, since you wont need it anymore - on struct digest_info add a string you can use on the command line and the functions you need to retrieve/compare it makes sense? Thanks /Ilias > > > > Tim > > > > > - Bring back the functions that exist in that branch. Pre 2023.07 all > > > measured boot code lived in lib/efi_loader/efi_tcg.c, so we just need > > > to copy them over > > > - Add a new struct member with the name and a new function to retriev= e it. > > > > > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/blob/v2022.= 01-rc3/lib/efi_loader/efi_tcg2.c?ref_type=3Dtags#L75 > > > > > > I understand that's more than what your patch tries to achieve here. = I > > > am fine pulling this as-is and you/I can fix it up later unless you > > > don't mind doing it in a v3. > > > > > > Thanks > > > /Ilias > > > > int tcg2_get_active_pcr_banks(struct udevice *dev, u32 *active_pcr= _banks) > > > > { > > > > u32 supported =3D 0; > > > > @@ -1555,3 +1562,30 @@ u32 tpm2_enable_nvcommits(struct udevice *de= v, uint vendor_cmd, > > > > > > > > return 0; > > > > } > > > > + > > > > +enum tpm2_algorithms tpm2_name_to_algorithm(const char *name) > > > > +{ > > > > + enum tpm2_algorithms algo; > > > > + size_t i; > > > > + > > > > + for (i =3D 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++= i) { > > > > + algo =3D tpm2_supported_algorithms[i]; > > > > + if (!strcasecmp(name, tpm2_supported_algorithm_name= s[i])) > > > > + return algo; > > > > + } > > > > + printf("%s: unsupported algorithm %s\n", __func__, name); > > > > + > > > > + return -EINVAL; > > > > +} > > > > + > > > > +const char *tpm2_algorithm_name(enum tpm2_algorithms algo) > > > > +{ > > > > + size_t i; > > > > + > > > > + for (i =3D 0; i < ARRAY_SIZE(tpm2_supported_algorithms); ++= i) { > > > > + if (tpm2_supported_algorithms[i] =3D=3D algo) > > > > + return tpm2_supported_algorithm_names[i]; > > > > + } > > > > + > > > > + return ""; > > > > +} > > > > -- > > > > 2.25.1 > > > >