| Message ID | 20240605032521.1142768-3-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Bug-fixes for a few boards | expand |
On 6/5/24 05:25, Simon Glass wrote: > It does not make sense to enable all SHA algorithms unless they are > needed. It bloats the code and in this case, causes chromebook_link to > fail to build. Why would chromebook_link fail to build? Is TPM used by U-Boot on that board at all? > > Add a condition to TPM to correct this. Note that the original commit > combines refactoring and new features, which makes it hard to see what > is going on. > > Fixes: 97707f12fda tpm: Support boot measurements > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > lib/Kconfig | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/Kconfig b/lib/Kconfig > index 189e6eb31aa..70b32362ada 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -438,10 +438,10 @@ config TPM > bool "Trusted Platform Module (TPM) Support" > depends on DM > imply DM_RNG > - select SHA1 > - select SHA256 > - select SHA384 > - select SHA512 > + select SHA1 if EFI_TCG2_PROTOCOL > + select SHA256 if EFI_TCG2_PROTOCOL > + select SHA384 if EFI_TCG2_PROTOCOL > + select SHA512 if EFI_TCG2_PROTOCOL You need to consider CONFIG_MEASURED_BOOT which allows measured boot in the non-UEFI case. Please, take into account lib/tpm-v1.c:20: #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too" This #error should be replaced by a Kconfig constraint. I would prefer the select statements to be in lib/efi_loader/Kconfig under EFI_TCG2_PROTOCOL. @Ilias, Eddie: Why do we require SHA1 which is considered insecure? Shouldn't we change tpm2_supported_algorithms[] to include only algorithms selected in the configuration? This would allow replacing all the select statements in Simon's patch by imply. Best regards Heinrich > help > This enables support for TPMs which can be used to provide security > features for your board. The TPM can be connected via LPC or I2C
Hi Heinrich On Wed, 5 Jun 2024 at 07:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 6/5/24 05:25, Simon Glass wrote: > > It does not make sense to enable all SHA algorithms unless they are > > needed. It bloats the code and in this case, causes chromebook_link to > > fail to build. > > Why would chromebook_link fail to build? > Is TPM used by U-Boot on that board at all? > > > > > Add a condition to TPM to correct this. Note that the original commit > > combines refactoring and new features, which makes it hard to see what > > is going on. > > > > Fixes: 97707f12fda tpm: Support boot measurements > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > lib/Kconfig | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/lib/Kconfig b/lib/Kconfig > > index 189e6eb31aa..70b32362ada 100644 > > --- a/lib/Kconfig > > +++ b/lib/Kconfig > > @@ -438,10 +438,10 @@ config TPM > > bool "Trusted Platform Module (TPM) Support" > > depends on DM > > imply DM_RNG > > - select SHA1 > > - select SHA256 > > - select SHA384 > > - select SHA512 > > + select SHA1 if EFI_TCG2_PROTOCOL > > + select SHA256 if EFI_TCG2_PROTOCOL > > + select SHA384 if EFI_TCG2_PROTOCOL > > + select SHA512 if EFI_TCG2_PROTOCOL > > You need to consider CONFIG_MEASURED_BOOT which allows measured boot in > the non-UEFI case. > > Please, take into account > > lib/tpm-v1.c:20: > #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too" > > This #error should be replaced by a Kconfig constraint. > > I would prefer the select statements to be in lib/efi_loader/Kconfig > under EFI_TCG2_PROTOCOL. > > @Ilias, Eddie: > > Why do we require SHA1 which is considered insecure? > > Shouldn't we change tpm2_supported_algorithms[] to include only > algorithms selected in the configuration? This would allow replacing all > the select statements in Simon's patch by imply. The algorithms used and configured by the TPM are device runtime decisions, not compile-time ones and to my knowledge, there are no devices out there that disable SHA1. Failing to extend a PCR in an active bank is a security vulnerability. It would allow the unsealing of secrets if an attacker can replay a good set of measurements into an unused bank. We could potentially fix that in the future since we can configure the TPM active banks on boot, but IIRC we don't support that yet. Regards /Ilias > > Best regards > > Heinrich > > > help > > This enables support for TPMs which can be used to provide security > > features for your board. The TPM can be connected via LPC or I2C >
Hi Heinrich, On Tue, 4 Jun 2024 at 22:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 6/5/24 05:25, Simon Glass wrote: > > It does not make sense to enable all SHA algorithms unless they are > > needed. It bloats the code and in this case, causes chromebook_link to > > fail to build. > > Why would chromebook_link fail to build? Because U-Boot suddenly grew a lot due to that commit. It broke out of the bounds of its SPI-flash region. > Is TPM used by U-Boot on that board at all? Yes, but only for tests. > > > > > Add a condition to TPM to correct this. Note that the original commit > > combines refactoring and new features, which makes it hard to see what > > is going on. > > > > Fixes: 97707f12fda tpm: Support boot measurements > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > lib/Kconfig | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/lib/Kconfig b/lib/Kconfig > > index 189e6eb31aa..70b32362ada 100644 > > --- a/lib/Kconfig > > +++ b/lib/Kconfig > > @@ -438,10 +438,10 @@ config TPM > > bool "Trusted Platform Module (TPM) Support" > > depends on DM > > imply DM_RNG > > - select SHA1 > > - select SHA256 > > - select SHA384 > > - select SHA512 > > + select SHA1 if EFI_TCG2_PROTOCOL > > + select SHA256 if EFI_TCG2_PROTOCOL > > + select SHA384 if EFI_TCG2_PROTOCOL > > + select SHA512 if EFI_TCG2_PROTOCOL > > You need to consider CONFIG_MEASURED_BOOT which allows measured boot in > the non-UEFI case. > > Please, take into account > > lib/tpm-v1.c:20: > #error "TPM_AUTH_SESSIONS require SHA1 to be configured, too" > > This #error should be replaced by a Kconfig constraint. > > I would prefer the select statements to be in lib/efi_loader/Kconfig > under EFI_TCG2_PROTOCOL. > > @Ilias, Eddie: > > Why do we require SHA1 which is considered insecure? > > Shouldn't we change tpm2_supported_algorithms[] to include only > algorithms selected in the configuration? This would allow replacing all > the select statements in Simon's patch by imply. I attempted that in v2. > > Best regards > > Heinrich > > > help > > This enables support for TPMs which can be used to provide security > > features for your board. The TPM can be connected via LPC or I2C > Regards, Simon
diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..70b32362ada 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,10 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG - select SHA1 - select SHA256 - select SHA384 - select SHA512 + select SHA1 if EFI_TCG2_PROTOCOL + select SHA256 if EFI_TCG2_PROTOCOL + select SHA384 if EFI_TCG2_PROTOCOL + select SHA512 if EFI_TCG2_PROTOCOL help This enables support for TPMs which can be used to provide security features for your board. The TPM can be connected via LPC or I2C
It does not make sense to enable all SHA algorithms unless they are needed. It bloats the code and in this case, causes chromebook_link to fail to build. Add a condition to TPM to correct this. Note that the original commit combines refactoring and new features, which makes it hard to see what is going on. Fixes: 97707f12fda tpm: Support boot measurements Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/Kconfig | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)