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

Thu Dec 19 16:37:37 CET 2024

Hi Simon,

On Thu, 19 Dec 2024 at 10:08, Simon Glass <sjg at chromium.org> wrote:

> 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.
>
> I think we still need a condition here indicating that the kconfig of
"handoff via bloblist" is enabled.
And this "handoff via bloblist" kconfig should mean to handoff everything
via bloblist, including DT.

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

Raymond