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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 9597 bytes --]

On Fri, Jun 21, 2024 at 08:57:51AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 20 Jun 2024 at 17:19, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 05:05:29PM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 19 Jun 2024 at 09:32, Tom Rini <trini@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@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@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@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@gmx.de> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On 14.06.24 09:01, Ilias Apalodimas wrote:
> > > > > > > > > > > > On Fri, 14 Jun 2024 at 09:59, Heinrich Schuchardt <xypron.glpk@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@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@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.
> >
> > I'm still confused, sorry. This sounds like a whole lot more work than
> > setting BOARD_SIZE_LIMIT reasonably so that we fail to link and CI
> > errors out, before we get to buildman and needing to make this case
> > still fail but with fake blobs.
> 
> It is simpler for me, since I don't need to reverse-engineer the space
> requirement for each board. With the patch I sent, I can just add some
> reasonable numbers to each entry and it will do the right thing.

Yes, I very much do not like guessing about 3 numbers instead of
guessing about 1 number and using the standard mechanism we already
have. Please use BOARD_SIZE_LIMIT as this is the standard mechanism to
enforce size limits on U-Boot itself.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]