[PATCH v2 2/9] tpm: Avoid code bloat when not using EFI_TCG2_PROTOCOL

[Simon Glass]

Hi Tom,

On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Jun 18, 2024 at 09:03:37PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 18 Jun 2024 at 08:15, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 06:43:51AM -0600, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Mon, 17 Jun 2024 at 11:16, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Mon, Jun 17, 2024 at 07:53:22AM -0600, Simon Glass wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On Sat, 15 Jun 2024 at 01:03, Ilias Apalodimas
> > > > > > <ilias.apalodimas at linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Heinrich
> > > > > > >
> > > > > > > resending the reply, I accidentally sent half of the message...
> > > > > > >
> > > > > > > On Fri, 14 Jun 2024 at 12:04, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > > > >>
> > > > > > > > >> On 6/14/24 08:03, Ilias Apalodimas wrote:
> > > > > > > > >>> Hi Simon,
> > > > > > > > >>>
> > > > > > > > >>> On Mon, 10 Jun 2024 at 17:59, Simon Glass <sjg at 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
> > > > > > > > >>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > >>>> ---
> > > > > > > > >>>>
> > > > > > > > >>>> 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
> > > > > > > > >>>
> > > > > > > > >>> I am not sure this is the right way to deal with your problem.
> > > > > > > > >>> The TPM main functionality is to measure and extend PCRs, so shaXXXX
> > > > > > > > >>> is really required. To make things even worse, you don't know the PCR
> > > > > > > > >>> banks that are enabled beforehand. This is a runtime config of the
> > > > > > > > >>> TPM.
> > > > > > > > >>
> > > > > > > > >> If neither MEASURED_BOOT nor EFI_TCG2_PROTOCOL is selected, U-Boot
> > > > > > > > >> cannot extend PCRs. So it seems fine to let these two select the
> > > > > > > > >> complete set of hashing algorithms. As Simon pointed out for
> > > > > > > > >> EFI_TCG2_PROTOCOL this is already done in lib/efi_loader/Kconfig.
> > > > > > > > >
> > > > > > > > > It can. The cmd we have can extend those pcrs -- e.g tpm2 pcr_extend 8
> > > > > > > > > 0xb0000000
> > > > > >
> > > > > > That's pretty normal for U-Boot though, since we want to avoid lots of
> > > > > > growth for things people might want control over. We can enable or
> > > > > > disable the SHA for the board, if this functionality is used outside
> > > > > > of measured boot and tcg2, but someone is enabling the tpm command.
> > > > > >
> > > > > > > >
> > > > > > > > So this patch should also consider CMD_TPM_V2 and CMD_TPM_V1.
> > > > > > > >
> > > > > > > > TPM v1 only needs SHA-1.
> > > > > > >
> > > > > > > I still prefer to imply all algos.
> > > > > >
> > > > > > 'imply' would be OK in this case as I can disable it for that board. I
> > > > > > don't think it is in the spirit of U-Boot though.
> > > > > >
> > > > > > isn't someone checking the growth in U-Boot? Or do so few boards have
> > > > > > TPMs that it didn't register? The size growth was 3.2KB on
> > > > > > chromebook_link.
> > > > >
> > > > > As always, yes, nearly every PR (I don't check the ones that touch just
> > > > > a single board for example) gets a world build before/after. In this
> > > > > case I likely assumed that it was acceptable growth for enabling
> > > > > features. It sounds like some of the chromebook boards need to be
> > > > > setting the features to cause link failure if a size is exceeded?
> > > >
> > > > The problem is that some Intel platforms have binary blobs, so the
> > > > size isn't known unless you have a real blob.
> > >
> > > Alright, but can't we put in some limit based on what the current blobs
> > > are, or look at the last few blob releases and see if they change much
> > > in size and put something in? Other platforms have blobs and size
> > > limits...
> >
> > Yes we can duplicate the entry sizes from binman into Kconfig options.
> > But I am not a fan of that...two variables controlling the same thing
> > and both can break, with nothing to connect them other than the poor
> > user.
> >
> > I wonder if some magic could solve this, inferring limits from the
> > binman image. But that is getting into yet another domain...
>
> I'm sorry, I just don't see what makes this case different from all of
> the other cases where we have to include blobs. We know there's X amount
> of flash available, blobs tend to take up Y and so we set
> BOARD_SIZE_LIMIT to Z where there's some room for growth in U-Boot but
> it takes in to account whatever limits the blobs place on us.

The difference is that now we are using Binman, we have the limit in
the image description, so adding it to Kconfig a) involves
calculations and perhaps guesswork and b) results in two limits stored
in different places which may conflict.

I thought of a way to handle this in Binman, so will send that in the
next version.

Regards,
Simon