Re: [PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
Hi Ilias,
On Fri, 21 Jun 2024 at 09:52, Ilias Apalodimas
wrote:
>
> Hi Simon,
>
> On Fri, 21 Jun 2024 at 17:57, Simon Glass wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
> > wrote:
> > >
> > > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, 21 Jun 2024 at 02:06, 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. That board does use the TPM, but not with measured
> > > > > boot,
> > > > > nor EFI.
> > > > >
> > > > > Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > > add them to MEASURED_BOOT as well.
> > > > >
> > > > > 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
> > > > > Reviewed-by: Heinrich Schuchardt
> > > > > Signed-off-by: Simon Glass
> > > > > ---
> > > > >
> > > > > (no changes since v2)
> > > >
> > > > There was a discussion in the previous version, bout enabling these on
> > > > CMD_TPM as they are required.
> >
> > For reference [1]
> >
> > >
> > > Or switch this to an imply instead so you can disable it ?
> >
> > The thing is, we need to be able to easily disable unwanted features.
> > My reading of that thread is that U-Boot doesn't use SHA384/512 from
> > the 'tpm2 pcr_extend' command today.
>
> It does, it was mentioned on the thread you referenced. commit
> 89aa8463cdf39 ("tpm-v2: allow algorithm name to be configured for
> pcr_read and pcr_extend")
>
> > Even if it did, I would think it
> > better to allow the supported algorithms to be controlled. At worst, I
> > would expect a separate option to disable that subcommand which would
> > remove the implies.
>
> To control the algos you need to compile, you need to control the
> algorithms the TPM supports. Otherwise, you are leaving security
> holes. We don't configure the supported PCR banks today iirc.
>
> If we need the fix for those platforms for the release so badly, why
> don't we just disable the TPM? It's going to be useless and
> potentially dangerous without SHA support. Any idea why those
> platforms use it for?
>
> >
> > Some tpm code recently added puts a table and some functions in
> > tpm-v2.h - could someone fix that? tpm-common.c would be a better
> > place.
>
> Do you mean commit 954b95e77ef0a8? If that's the one, I don't mind
> moving it there.
> Those values though are described in [0], which is primarily for v2.0.
> Nothing prevents you from using it in v1.2 but all of the values apart
> from SHA1 are useless. Also, we don't support measured boot with 1.2
> anyway so I didn't see the point back then.
>
> > Also we already have a list of hash algorithms in U-Boot
> > (common/hash.c) so should use that, >and respect the fact that certain
> > algos may not be supported.
>
> That seems tailored for a very specific reason. Do you want to fill it
> in with EFI internal definitions?
> We could adjust some lookup functions which seems common.
>
> >
> > If there is a requirement that (for a certain situation / protocol) we
> > need to support everything, then let's have a Kconfig for that.
>
> We do. The difference here is that the *TPM at runtime* dictates what
> you need. So you can't blindly disable stuff.
>
> > It is
> > what my patch was trying to work around, perhaps. For boards which
> > don't care about extending TPM in this way, or don't use the more
> > expensive algorithms, it avoids lots of code bloat.
>
> Ok, I disagree here. That's potentially dangerous. Because someone,
> might need the TPM, easily misconfigure it, and end up with broken
> measurements.
>
> >
> > One of the principles of U-Boot is to allow configurability in these
> > situations. It shouldn't be too hard to have the best of both worlds
> > (supports all reasonable cases by default / allow unwanted cases to be
> > dropped).
>
> It's not. We can have the TPM configure the supported algorithms once it
> starts.
> IMHO that's the proper way to do it. Everything else is just papering
> over the problem -- which even worse no one will care to fix.
>
> >
> > So I'll await your reply on the above before figuring out where to go
> > with this. For this release I'd just like to get the board working,
> > but I understand that the TPM stuff is very-much in flux, perhaps in
> > -next.
>
> This is a very complex problem. Even if we configure the TPMs banks
> based on the supported algoriths, a previous stage loader might have
> enabled more. So you end up breaking that in that case...
>
> Let me see if I can configure the TPM on boot
I just don't understand any of this very well.
My request is, please can you find a way to allow the TPM to be
enabled for a board that doesn't use the pcr_extend subcommand with
SHA3
Re: [PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
Hi Simon,
On Fri, 21 Jun 2024 at 17:57, Simon Glass wrote:
>
> Hi Ilias,
>
> On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
> wrote:
> >
> > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> > wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, 21 Jun 2024 at 02:06, 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. That board does use the TPM, but not with measured boot,
> > > > nor EFI.
> > > >
> > > > Since EFI_TCG2_PROTOCOL already selects these options, we just need to
> > > > add them to MEASURED_BOOT as well.
> > > >
> > > > 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
> > > > Reviewed-by: Heinrich Schuchardt
> > > > Signed-off-by: Simon Glass
> > > > ---
> > > >
> > > > (no changes since v2)
> > >
> > > There was a discussion in the previous version, bout enabling these on
> > > CMD_TPM as they are required.
>
> For reference [1]
>
> >
> > Or switch this to an imply instead so you can disable it ?
>
> The thing is, we need to be able to easily disable unwanted features.
> My reading of that thread is that U-Boot doesn't use SHA384/512 from
> the 'tpm2 pcr_extend' command today.
It does, it was mentioned on the thread you referenced. commit
89aa8463cdf39 ("tpm-v2: allow algorithm name to be configured for
pcr_read and pcr_extend")
> Even if it did, I would think it
> better to allow the supported algorithms to be controlled. At worst, I
> would expect a separate option to disable that subcommand which would
> remove the implies.
To control the algos you need to compile, you need to control the
algorithms the TPM supports. Otherwise, you are leaving security
holes. We don't configure the supported PCR banks today iirc.
If we need the fix for those platforms for the release so badly, why
don't we just disable the TPM? It's going to be useless and
potentially dangerous without SHA support. Any idea why those
platforms use it for?
>
> Some tpm code recently added puts a table and some functions in
> tpm-v2.h - could someone fix that? tpm-common.c would be a better
> place.
Do you mean commit 954b95e77ef0a8? If that's the one, I don't mind
moving it there.
Those values though are described in [0], which is primarily for v2.0.
Nothing prevents you from using it in v1.2 but all of the values apart
from SHA1 are useless. Also, we don't support measured boot with 1.2
anyway so I didn't see the point back then.
> Also we already have a list of hash algorithms in U-Boot
> (common/hash.c) so should use that, >and respect the fact that certain
> algos may not be supported.
That seems tailored for a very specific reason. Do you want to fill it
in with EFI internal definitions?
We could adjust some lookup functions which seems common.
>
> If there is a requirement that (for a certain situation / protocol) we
> need to support everything, then let's have a Kconfig for that.
We do. The difference here is that the *TPM at runtime* dictates what
you need. So you can't blindly disable stuff.
> It is
> what my patch was trying to work around, perhaps. For boards which
> don't care about extending TPM in this way, or don't use the more
> expensive algorithms, it avoids lots of code bloat.
Ok, I disagree here. That's potentially dangerous. Because someone,
might need the TPM, easily misconfigure it, and end up with broken
measurements.
>
> One of the principles of U-Boot is to allow configurability in these
> situations. It shouldn't be too hard to have the best of both worlds
> (supports all reasonable cases by default / allow unwanted cases to be
> dropped).
It's not. We can have the TPM configure the supported algorithms once it starts.
IMHO that's the proper way to do it. Everything else is just papering
over the problem -- which even worse no one will care to fix.
>
> So I'll await your reply on the above before figuring out where to go
> with this. For this release I'd just like to get the board working,
> but I understand that the TPM stuff is very-much in flux, perhaps in
> -next.
This is a very complex problem. Even if we configure the TPMs banks
based on the supported algoriths, a previous stage loader might have
enabled more. So you end up breaking that in that case...
Let me see if I can configure the TPM on boot
Cheers
/Ilias
>
> Regards,
> Simon
>
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-...@chromium.org/
[0]
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
-- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
Re: [PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
Hi Ilias, On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas wrote: > > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas > wrote: > > > > Hi Simon, > > > > On Fri, 21 Jun 2024 at 02:06, 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. That board does use the TPM, but not with measured boot, > > > nor EFI. > > > > > > Since EFI_TCG2_PROTOCOL already selects these options, we just need to > > > add them to MEASURED_BOOT as well. > > > > > > 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 > > > Reviewed-by: Heinrich Schuchardt > > > Signed-off-by: Simon Glass > > > --- > > > > > > (no changes since v2) > > > > There was a discussion in the previous version, bout enabling these on > > CMD_TPM as they are required. For reference [1] > > Or switch this to an imply instead so you can disable it ? The thing is, we need to be able to easily disable unwanted features. My reading of that thread is that U-Boot doesn't use SHA384/512 from the 'tpm2 pcr_extend' command today. Even if it did, I would think it better to allow the supported algorithms to be controlled. At worst, I would expect a separate option to disable that subcommand which would remove the implies. Some tpm code recently added puts a table and some functions in tpm-v2.h - could someone fix that? tpm-common.c would be a better place. Also we already have a list of hash algorithms in U-Boot (common/hash.c) so should use that, and respect the fact that certain algos may not be supported. If there is a requirement that (for a certain situation / protocol) we need to support everything, then let's have a Kconfig for that. It is what my patch was trying to work around, perhaps. For boards which don't care about extending TPM in this way, or don't use the more expensive algorithms, it avoids lots of code bloat. One of the principles of U-Boot is to allow configurability in these situations. It shouldn't be too hard to have the best of both worlds (supports all reasonable cases by default / allow unwanted cases to be dropped). So I'll await your reply on the above before figuring out where to go with this. For this release I'd just like to get the board working, but I understand that the TPM stuff is very-much in flux, perhaps in -next. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-...@chromium.org/
Re: [PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas wrote: > > Hi Simon, > > On Fri, 21 Jun 2024 at 02:06, 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. That board does use the TPM, but not with measured boot, > > nor EFI. > > > > Since EFI_TCG2_PROTOCOL already selects these options, we just need to > > add them to MEASURED_BOOT as well. > > > > 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 > > Reviewed-by: Heinrich Schuchardt > > Signed-off-by: Simon Glass > > --- > > > > (no changes since v2) > > There was a discussion in the previous version, bout enabling these on > CMD_TPM as they are required. Or switch this to an imply instead so you can disable it ? Regards /Ilias > > Thanks > /Ilias > > >
Re: [PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
Hi Simon, On Fri, 21 Jun 2024 at 02:06, 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. That board does use the TPM, but not with measured boot, > nor EFI. > > Since EFI_TCG2_PROTOCOL already selects these options, we just need to > add them to MEASURED_BOOT as well. > > 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 > Reviewed-by: Heinrich Schuchardt > Signed-off-by: Simon Glass > --- > > (no changes since v2) There was a discussion in the previous version, bout enabling these on CMD_TPM as they are required. Thanks /Ilias > > Changes in v2: > - Put the conditions under EFI_TCG2_PROTOCOL > - Consider MEASURED_BOOT too > > boot/Kconfig | 4 > lib/Kconfig | 4 > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/boot/Kconfig b/boot/Kconfig > index 6f3096c15a6..b061891e109 100644 > --- a/boot/Kconfig > +++ b/boot/Kconfig > @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT > config MEASURED_BOOT > bool "Measure boot images and configuration when booting without EFI" > depends on HASH && TPM_V2 > + select SHA1 > + select SHA256 > + select SHA384 > + select SHA512 > help > This option enables measurement of the boot process when booting > without UEFI . Measurement involves creating cryptographic hashes > diff --git a/lib/Kconfig b/lib/Kconfig > index 189e6eb31aa..568892fce44 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -438,10 +438,6 @@ config TPM > bool "Trusted Platform Module (TPM) Support" > depends on DM > imply DM_RNG > - select SHA1 > - select SHA256 > - select SHA384 > - select SHA512 > 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 > -- > 2.34.1 >
[PATCH v3 10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL
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. That board does use the TPM, but not with measured boot, nor EFI. Since EFI_TCG2_PROTOCOL already selects these options, we just need to add them to MEASURED_BOOT as well. 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 Reviewed-by: Heinrich Schuchardt Signed-off-by: Simon Glass --- (no changes since v2) Changes in v2: - Put the conditions under EFI_TCG2_PROTOCOL - Consider MEASURED_BOOT too boot/Kconfig | 4 lib/Kconfig | 4 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/boot/Kconfig b/boot/Kconfig index 6f3096c15a6..b061891e109 100644 --- a/boot/Kconfig +++ b/boot/Kconfig @@ -734,6 +734,10 @@ config LEGACY_IMAGE_FORMAT config MEASURED_BOOT bool "Measure boot images and configuration when booting without EFI" depends on HASH && TPM_V2 + select SHA1 + select SHA256 + select SHA384 + select SHA512 help This option enables measurement of the boot process when booting without UEFI . Measurement involves creating cryptographic hashes diff --git a/lib/Kconfig b/lib/Kconfig index 189e6eb31aa..568892fce44 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -438,10 +438,6 @@ config TPM bool "Trusted Platform Module (TPM) Support" depends on DM imply DM_RNG - select SHA1 - select SHA256 - select SHA384 - select SHA512 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 -- 2.34.1
