Hi Ilias,
On Fri, 21 Jun 2024 at 09:52, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, 21 Jun 2024 at 17:57, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 20 Jun 2024 at 23:49, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > On Fri, 21 Jun 2024 at 08:32, Ilias Apalodimas
> > > <ilias.apalodi...@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, 21 Jun 2024 at 02:06, Simon Glass <s...@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.g...@gmx.de>
> > > > > Signed-off-by: Simon Glass <s...@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-...@chromium.org/
>
> [0]
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> -- search for EFI_TCG2_BOOT_HASH_ALG_SHA512
