[PATCH 2/2] tpm: get tpm event log from bloblist

[Simon Glass]

Hi,

On Mon, 16 Dec 2024 at 11:30, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Dec 16, 2024 at 10:37:30AM -0700, Simon Glass wrote:
> > Hi Raymond,
> >
> > On Mon, 16 Dec 2024 at 10:24, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Mon, 16 Dec 2024 at 10:43, Simon Glass <sjg at chromium.org> wrote:
> > >>
> > >> Hi Raymond,
> > >>
> > >> On Mon, 16 Dec 2024 at 08:29, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >> >
> > >> > Hi Simon,
> > >> >
> > >> > On Mon, 16 Dec 2024 at 10:20, Simon Glass <sjg at chromium.org> wrote:
> > >> >>
> > >> >> Hi Raymond,
> > >> >>
> > >> >> On Mon, 16 Dec 2024 at 07:52, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >> >> >
> > >> >> > Hi Simon,
> > >> >> >
> > >> >> > On Sun, 15 Dec 2024 at 19:25, Simon Glass <sjg at chromium.org> wrote:
> > >> >> >>
> > >> >> >> Hi Raymond,
> > >> >> >>
> > >> >> >> On Fri, 13 Dec 2024 at 15:56, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >> >> >> >
> > >> >> >> > Get tpm event log from bloblist instead of FDT when bloblist is
> > >> >> >> > enabled and valid from previous boot stage.
> > >> >> >> >
> > >> >> >> > Note:
> > >> >> >> > This patch depends on:
> > >> >> >> > [PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options
> > >> >> >> > https://lore.kernel.org/u-boot/20241212151142.1562825-1-trini@konsulko.com/
> > >> >> >> >
> > >> >> >> > Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
> > >> >> >> > ---
> > >> >> >> >  lib/tpm_tcg2.c | 7 +++++++
> > >> >> >> >  1 file changed, 7 insertions(+)
> > >> >> >> >
> > >> >> >> > diff --git a/lib/tpm_tcg2.c b/lib/tpm_tcg2.c
> > >> >> >> > index 7f868cc883..acaf0acb88 100644
> > >> >> >> > --- a/lib/tpm_tcg2.c
> > >> >> >> > +++ b/lib/tpm_tcg2.c
> > >> >> >> > @@ -19,6 +19,7 @@
> > >> >> >> >  #include <linux/unaligned/generic.h>
> > >> >> >> >  #include <linux/unaligned/le_byteshift.h>
> > >> >> >> >  #include "tpm-utils.h"
> > >> >> >> > +#include <bloblist.h>
> > >> >> >> >
> > >> >> >> >  int tcg2_get_pcr_info(struct udevice *dev, u32 *supported_pcr, u32 *active_pcr,
> > >> >> >> >                       u32 *pcr_banks)
> > >> >> >> > @@ -672,6 +673,12 @@ __weak int tcg2_platform_get_log(struct udevice *dev, void **addr, u32 *size)
> > >> >> >> >         *addr = NULL;
> > >> >> >> >         *size = 0;
> > >> >> >> >
> > >> >> >> > +       if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) && !bloblist_maybe_init()) {
> > >> >> >>
> > >> >> >> Please drop the second term...the bloblist is inited by U-Boot already.
> > >> >> >>
> > >> >> >
> > >> >> > As an independent library, from the tpm library's angle, whether the bloblist init was already called or not is agnostic.
> > >> >> > I think this is the purpose of the "maybe" function.
> > >> >>
> > >> >> No. U-Boot does init at the start. We don't want each subsystem
> > >> >> 'maybe' initing each other subsystem that it uses. If you want to
> > >> >> check whether bloblist is available, you can use the GD flag.
> > >> >>
> > >> >
> > >> > OK. I can use the gd instead.
> > >> >
> > >> >>
> > >> >> >
> > >> >> >>
> > >> >> >> Also please drop the first term. If you find it in the bloblist, just use it.
> > >> >> >>
> > >> >> >
> > >> >> > I think here we should have the same logic as the existing one in fdtdec:
> > >> >> > If bloblist is not enabled or errors observed, do a fallback to the original process.
> > >> >> > At least I want to keep the same logic in different places before everything is ready from TF-A to U-boot.
> > >> >>
> > >> >> Yes, your second line is right. But PRIOR_STAGE doesn't exist, nor
> > >> >> should it. Nor do you need it, since you can just check the bloblist
> > >> >> for what you want. If you don't find it, fall back.
> > >> >>
> > >> >
> > >> > Since Firmware Handoff is still an experimental feature and not mandatory yet (at least now),
> > >> > I put everything under PRIOR_STAGE to allow users to disable it by their own decisions.
> > >> > I saw PRIOR_STAGE in one of Tom's recent patches so I have added a dependency claim in my commit message.
> > >>
> > >> Yes, but even if Tom does pick his patch, which I object to, by the
> > >> time your code is called, there will be a bloblist.
> > >>
> > >> So the end result of your test is to allow the user to ignore any TPM
> > >> log in the bloblist. If that's what you want (which seems fine to me),
> > >> add a Kconfig for it.
> > >>
> > >
> > > Per my understanding of PRIOR_STAGE, it means we have a bloblist from the previous stage and we want to use it (as a Transfer List).
> > > If this is the case, it is not for DT only but can be applied for eventlog or anything for Firmware Handoff in the future.
> > > That is the reason I use PRIOR_STAGE here, I don't think we need to add another kconfig.
> >
> > Who knows what it is supposed to mean...anyway, it should not be used
> > like that. The devicetree is special in that we want to get it from
> > the bloblist very early, if available. Nothing else (so far) in the
> > bloblist has this constraint, certainly not the TPM log.
>
> Raymond is exactly right, and the device tree is not special.

That is quite wrong IMO. Devicetree is special because we need it
early and it might come from bloblist before the bloblist has been set
up by U-Boot. It also controls the whole operation of U-Boot. There is
nothing else in U-Boot quite like the devicetree.

> The
> specialness is "do we have a valid bloblist upon entry or not". Which
> means that also no, this series (since it happens late enough) shouldn't
> care about if the previous stage had created it or not, merely as you
> said if the tag for the TPM (or other blob) is found.

I would feel more comfortable having the discussions about what
previous stages need to pass U-Boot a bloblist once we have that
situation. This particular patch seems clear enough, and well
motivated. It just needs to drop the conditions.

>
> And that said, yes, I will be v2'ing what I did, in some manner or
> another as it's not quite right either in terms of being clear about
> what the condition is.

Regards,
Simon