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

Thu Jan 2 16:25:15 CET 2025

Hi Simon,

On Wed, 1 Jan 2025 at 17:15, Simon Glass <sjg at chromium.org> wrote:

> Hi Tom,
>
> On Fri, 20 Dec 2024 at 05:36, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Thu, Dec 19, 2024 at 10:37:37AM -0500, Raymond Mao wrote:
> > > 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.
>
> No, I don't agree with that at all.
>
> What actually breaks if you look for the TPM log in the bloblist?
> Please answer this question.
>
>
As I said, we need an kconfig here to decide whether a user should look for
TPM log (and all other handoff information defined by the Firmware Handoff
specification) from the bloblist or not.
We don't have such kconfig now.

> >
> > There's at least two sets of challenges here. One, being solved by
> > vexpress64 right now, is that we didn't have CONFIG_BLOBLIST_PASSAGE as
> > an actual option. And in that case, there's no U-Boot before full U-Boot
> > and the bloblist exists for us. Two, U-Boot is what is creating the
> > bloblist. The contentious parts are *when* it's created and *where* it
> > resides prior to full U-Boot seeing it.
>
> There isn't contention, so far as I am aware. The normal case is that
> U-Boot creates and uses the bloblist itself. Pre-U-Boot blobs (like
> TF-A, sadly) are not the normal case and should be discouraged in an
> open-source project. That doesn't mean we shouldn't support them, but
> it is the tail wagging the dog.
>
> TBH, I am confused with this statement which means we should not use the
bloblist library from the beginning to hand over data from the previous
stage.
If U-Boot bloblist only intends to consume the data created by itself, we
have to introduce another library to do the handoff, like what was done in
TF-A and OP-TEE -
then finally we can have a standard handoff library that can be used in all
projects and keep bloblist as it was.

[snip]

Regards,
Raymond