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....
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
