diff mbox series

[v3,10/18] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

Message ID 20240620230625.1797397-11-sjg@chromium.org
State Superseded
Delegated to: Tom Rini
Headers show
Series Bug-fixes for a few boards | expand

Commit Message

Simon Glass June 20, 2024, 11:06 p.m. UTC 
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 <xypron.glpk@gmx.de>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

(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(-)

Comments

Ilias Apalodimas June 21, 2024, 5:32 a.m. UTC | #1 
Hi Simon,

On Fri, 21 Jun 2024 at 02:06, Simon Glass <sjg@chromium.org> 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 <xypron.glpk@gmx.de>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (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
>
Ilias Apalodimas June 21, 2024, 5:48 a.m. UTC | #2 
On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 21 Jun 2024 at 02:06, Simon Glass <sjg@chromium.org> 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 <xypron.glpk@gmx.de>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> > (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
> >
>
Simon Glass June 21, 2024, 2:57 p.m. UTC | #3 
Hi Ilias,

On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, 21 Jun 2024 at 02:06, Simon Glass <sjg@chromium.org> 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 <xypron.glpk@gmx.de>
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >
> > > (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-sjg@chromium.org/
Ilias Apalodimas June 21, 2024, 3:52 p.m. UTC | #4 
Hi Simon,

On Fri, 21 Jun 2024 at 17:57, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, 21 Jun 2024 at 02:06, Simon Glass <sjg@chromium.org> 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 <xypron.glpk@gmx.de>
> > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > >
> > > > (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-sjg@chromium.org/

[0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
-- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
Simon Glass June 21, 2024, 5:50 p.m. UTC | #5 
Hi Ilias,

On Fri, 21 Jun 2024 at 09:52, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 21 Jun 2024 at 17:57, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, 21 Jun 2024 at 02:06, Simon Glass <sjg@chromium.org> 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 <xypron.glpk@gmx.de>
> > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >
> > > > > (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
SHA384/512? I just want to get that 5KB back for this board. I'm happy
with the defaults being a certain way, just not wanting to require
this code for boards which don't need it.

Regards,
Simon


>
> Cheers
> /Ilias
> >
> > Regards,
> > Simon
> >
> > [1] https://patchwork.ozlabs.org/project/uboot/patch/20240610145920.3302001-3-sjg@chromium.org/
>
> [0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> -- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
diff mbox series

Patch

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