Hi Ilas, > -----Original Message----- > From: Ilias Apalodimas <[email protected]> > Sent: Tuesday, August 27, 2024 10:05 AM > To: Benjamin BARATTE <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected] > Subject: Re: [PATCH 3/3] tpm_tcg2: hash algo optimization > > Hi Benjamin, > > > On Fri, 23 Aug 2024 at 15:29, Benjamin BARATTE > <[email protected]> wrote: > > > > Hi @Ilias Apalodimas, > > > > > > ST Restricted > > > -----Original Message----- > > > From: Ilias Apalodimas <[email protected]> > > > Sent: Monday, July 29, 2024 4:10 PM > > > To: Benjamin BARATTE <[email protected]> > > > Cc: [email protected]; [email protected]; > > > [email protected]; [email protected]; > > > [email protected]; [email protected]; > [email protected]; > > > [email protected]; [email protected]; [email protected] > > > Subject: Re: [PATCH 3/3] tpm_tcg2: hash algo optimization > > > > > > On Mon, Jul 15, 2024 at 01:33:19PM +0000, Benjamin BARATTE wrote: > > > > To properly enable code optimization with hash algorithm, all the > > > > reference of the hash algo should condition to hash enablement. > > > > It is possible to fine tune the list of hash algorithms based on > > > > dTPM configuration. > > > > Therefore if dTPM supports only one bank, on one hash algorithm > > > > could be selected (TCG PTP specification mention in case of single > > > > bank support that SHA256 must be support and default value) > > > > > > Yes, but... > > > In order to apply this we need a function that *configures* the TPM > > > with only the supported compiled algorithms. If a TPM is configured > > > with more than what u-boot supports, there might be security > > > implications since we will fail to cap all active PCRs. On top of > > > that the EFI TCG explicitly says that all the active PCR banks needs to be > updated when extending measurements. > > > > > > > I'm agree with you, but this is more a configuration issue in that case. > > Today to do such configuration, we need to do it with tpm2-tools under > > Linux But if U-boot is preventing the Linux kernel to boot, this will become > problematic. > > It's the same configuration issue for u-boot. If you enable all hashing > algorithms Linux will be just fine. > > > And U-boot API to configure the TPM PCR will be more than welcome in U- > boot. > > Configuring the TPM PCR available banks shouldnt be too much code, we > already have a function to read the available ones. > > > > > This will allow industrial user to manage this configuration in U-boot > > instead > of Linux. > > Failing to cap all available PCR banks might lead to misconfiguration errors > and > eventually compromise the TPM security -- e.g unseal keys you shouldnt. > There's an extended discussion about the same issue here [0]. Keeping that in > mind, I don't want to pick patches that allow people to shoot themselves in > the foot. > > [0] https://lore.kernel.org/linux-integrity/20240531010331.134441-7- > [email protected]/ >
OK understood. What could be done is to generate an error if, at runtime, there is a mismatch between U-Boot compilation and TPM configuration. Best Regards, Benjamin > Thanks > /Ilias > > > > > > Best Regards, > > > > Benjamin > > > > > Thanks > > > /Ilias > > > > > > > > > > > Signed-off-by: Benjamin BARATTE <[email protected]> > > > > --- > > > > > > > > include/tpm-v2.h | 8 -------- > > > > lib/efi_loader/Kconfig | 4 ---- > > > > lib/tpm_tcg2.c | 38 > ++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 38 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h index > > > > 9848e1fd10..ec3504de44 100644 > > > > --- a/include/tpm-v2.h > > > > +++ b/include/tpm-v2.h > > > > @@ -285,38 +285,30 @@ struct digest_info { > > > > > > > > > > > > static const struct digest_info hash_algo_list[] = { -#if > > > > IS_ENABLED(CONFIG_SHA1) > > > > { > > > > "sha1", > > > > TPM2_ALG_SHA1, > > > > TCG2_BOOT_HASH_ALG_SHA1, > > > > TPM2_SHA1_DIGEST_SIZE, > > > > }, > > > > -#endif > > > > -#if IS_ENABLED(CONFIG_SHA256) > > > > { > > > > "sha256", > > > > TPM2_ALG_SHA256, > > > > TCG2_BOOT_HASH_ALG_SHA256, > > > > TPM2_SHA256_DIGEST_SIZE, > > > > }, > > > > -#endif > > > > -#if IS_ENABLED(CONFIG_SHA384) > > > > { > > > > "sha384", > > > > TPM2_ALG_SHA384, > > > > TCG2_BOOT_HASH_ALG_SHA384, > > > > TPM2_SHA384_DIGEST_SIZE, > > > > }, > > > > -#endif > > > > -#if IS_ENABLED(CONFIG_SHA512) > > > > { > > > > "sha512", > > > > TPM2_ALG_SHA512, > > > > TCG2_BOOT_HASH_ALG_SHA512, > > > > TPM2_SHA512_DIGEST_SIZE, > > > > }, > > > > -#endif > > > > { > > > > "sha3_256", > > > > TPM2_ALG_SHA3_256, diff --git > > > > a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig index > > > > ee71f41714..512fb710b5 100644 > > > > --- a/lib/efi_loader/Kconfig > > > > +++ b/lib/efi_loader/Kconfig > > > > @@ -405,10 +405,6 @@ config EFI_TCG2_PROTOCOL > > > > bool "EFI_TCG2_PROTOCOL support" > > > > default y > > > > depends on TPM_V2 > > > > - select SHA1 > > > > - select SHA256 > > > > - select SHA384 > > > > - select SHA512 > > > > select HASH > > > > select SMBIOS_PARSER > > > > help > > > > diff --git a/lib/tpm_tcg2.c b/lib/tpm_tcg2.c index > > > > 7f868cc883..66573b838d 100644 > > > > --- a/lib/tpm_tcg2.c > > > > +++ b/lib/tpm_tcg2.c > > > > @@ -96,9 +96,15 @@ int tcg2_create_digest(struct udevice *dev, > > > > const u8 > > > *input, u32 length, > > > > struct tpml_digest_values *digest_list) { > > > > u8 final[sizeof(union tpmu_ha)]; > > > > +#if IS_ENABLED(CONFIG_SHA256) > > > > sha256_context ctx_256; > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA384) || IS_ENABLED(CONFIG_SHA512) > > > > sha512_context ctx_512; > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA1) > > > > sha1_context ctx; > > > > +#endif > > > > u32 active; > > > > size_t i; > > > > u32 len; > > > > @@ -114,30 +120,38 @@ int tcg2_create_digest(struct udevice *dev, > > > > const > > > u8 *input, u32 length, > > > > continue; > > > > > > > > switch (hash_algo_list[i].hash_alg) { > > > > +#if IS_ENABLED(CONFIG_SHA1) > > > > case TPM2_ALG_SHA1: > > > > sha1_starts(&ctx); > > > > sha1_update(&ctx, input, length); > > > > sha1_finish(&ctx, final); > > > > len = TPM2_SHA1_DIGEST_SIZE; > > > > break; > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA256) > > > > case TPM2_ALG_SHA256: > > > > sha256_starts(&ctx_256); > > > > sha256_update(&ctx_256, input, length); > > > > sha256_finish(&ctx_256, final); > > > > len = TPM2_SHA256_DIGEST_SIZE; > > > > break; > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA384) > > > > case TPM2_ALG_SHA384: > > > > sha384_starts(&ctx_512); > > > > sha384_update(&ctx_512, input, length); > > > > sha384_finish(&ctx_512, final); > > > > len = TPM2_SHA384_DIGEST_SIZE; > > > > break; > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA512) > > > > case TPM2_ALG_SHA512: > > > > sha512_starts(&ctx_512); > > > > sha512_update(&ctx_512, input, length); > > > > sha512_finish(&ctx_512, final); > > > > len = TPM2_SHA512_DIGEST_SIZE; > > > > break; > > > > +#endif > > > > default: > > > > printf("%s: unsupported algorithm %x\n", > > > > __func__, > > > > hash_algo_list[i].hash_alg); @@ > > > > -236,10 > > > > +250,18 @@ static int tcg2_log_init(struct udevice *dev, struct > > > tcg2_event_log *elog) > > > > continue; > > > > > > > > switch (hash_algo_list[i].hash_alg) { > > > > +#if IS_ENABLED(CONFIG_SHA1) > > > > case TPM2_ALG_SHA1: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA256) > > > > case TPM2_ALG_SHA256: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA384) > > > > case TPM2_ALG_SHA384: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA512) > > > > case TPM2_ALG_SHA512: > > > > +#endif > > > > count++; > > > > break; > > > > default: > > > > @@ -337,10 +359,18 @@ static int tcg2_replay_eventlog(struct > > > tcg2_event_log *elog, > > > > algo = get_unaligned_le16(log + pos); > > > > pos += offsetof(struct tpmt_ha, digest); > > > > switch (algo) { > > > > +#if IS_ENABLED(CONFIG_SHA1) > > > > case TPM2_ALG_SHA1: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA256) > > > > case TPM2_ALG_SHA256: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA384) > > > > case TPM2_ALG_SHA384: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA512) > > > > case TPM2_ALG_SHA512: > > > > +#endif > > > > len = tpm2_algorithm_to_len(algo); > > > > break; > > > > default: > > > > @@ -450,10 +480,18 @@ static int tcg2_log_parse(struct udevice > > > > *dev, > > > struct tcg2_event_log *elog) > > > > return 0; > > > > > > > > switch (algo) { > > > > +#if IS_ENABLED(CONFIG_SHA1) > > > > case TPM2_ALG_SHA1: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA256) > > > > case TPM2_ALG_SHA256: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA384) > > > > case TPM2_ALG_SHA384: > > > > +#endif > > > > +#if IS_ENABLED(CONFIG_SHA512) > > > > case TPM2_ALG_SHA512: > > > > +#endif > > > > len = get_unaligned_le16(&event- > >digest_sizes[i].digest_size); > > > > if (tpm2_algorithm_to_len(algo) != len) > > > > return 0; > > > > -- > > > > 2.34.1 > > > > > > > > ST Restricted
