Re: [PATCH v2 3/5] ieee1275/tcg2: Refactor grub_ieee1275_tpm_init

[Stefan Berger]

On 11/26/24 9:44 AM, Daniel Kiper wrote:
> On Mon, Nov 25, 2024 at 05:41:40PM -0500, Stefan Berger wrote:
> > Move tpm_get_tpm_version into grub_ieee1275_tpm_init and invalidate
>
> s/tpm_get_tpm_version/tpm_get_tpm_version()/
> s/grub_ieee1275_tpm_init/grub_ieee1275_tpm_init()/
>
> > grub_ieee1275_tpm_ihandle in case no TPM 2 could be detected. Try the
> > initialization only once. Use the grub_ieee1275_tpm_ihandle as
>
> Could you explain why you change logic here? I am not against that but
> I think it should be clarified why...

Ok, will do. It's basically to ensure that grub_tpm_present() always 
returns the same result for this here:

GRUB_MOD_INIT (tpm)
{
  /*
   * Even though this now calls ibmvtpm's grub_tpm_present() from 
GRUB_MOD_INIT(),
   * it does seem to call it late enough in the initialization sequence so
   * that whatever discovered "device nodes" before this GRUB_MOD_INIT() is
   * called, enables the ibmvtpm driver to see the device nodes.
   */
  if (!grub_tpm_present())
    return;
  grub_verifier_register (&grub_tpm_verifier);
}

GRUB_MOD_FINI (tpm)
{
  if (!grub_tpm_present())
    return;
  grub_verifier_unregister (&grub_tpm_verifier);
}


> > indicator for an available TPM instead of grub_ieee1275_tpm_version,
> > which can now be removed.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >   grub-core/commands/ieee1275/ibmvtpm.c |  2 +-
> >   grub-core/lib/ieee1275/tcg2.c         | 43 +++++++++++----------------
> >   include/grub/ieee1275/tpm.h           |  1 -
> >   3 files changed, 18 insertions(+), 28 deletions(-)
> >
> > diff --git a/grub-core/commands/ieee1275/ibmvtpm.c 
> > b/grub-core/commands/ieee1275/ibmvtpm.c
> > index 284673217..16552b136 100644
> > --- a/grub-core/commands/ieee1275/ibmvtpm.c
> > +++ b/grub-core/commands/ieee1275/ibmvtpm.c
> > @@ -100,7 +100,7 @@ grub_tpm_measure (unsigned char *buf, grub_size_t size, 
> > grub_uint8_t pcr,
> >     grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", 
> > %s\n",
> >                 pcr, size, description);
> >
> > -  if (grub_ieee1275_tpm_version == 2)
> > +  if (grub_ieee1275_tpm_ihandle != IEEE1275_IHANDLE_INVALID)
> >       return tpm2_log_event (buf, size, pcr, description);
> >
> >     return GRUB_ERR_NONE;
> > diff --git a/grub-core/lib/ieee1275/tcg2.c b/grub-core/lib/ieee1275/tcg2.c
> > index 8c4306ae8..8f29b3c1e 100644
> > --- a/grub-core/lib/ieee1275/tcg2.c
> > +++ b/grub-core/lib/ieee1275/tcg2.c
> > @@ -23,39 +23,30 @@
> >   #include <grub/mm.h>
> >   #include <grub/misc.h>
> >
> > -grub_ieee1275_ihandle_t grub_ieee1275_tpm_ihandle;
> > -grub_uint8_t grub_ieee1275_tpm_version;
> > -
> > -static void
> > -tpm_get_tpm_version (void)
> > -{
> > -  grub_ieee1275_phandle_t vtpm;
> > -  char buffer[20];
> > -
> > -  if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
> > -      !grub_ieee1275_get_property (vtpm, "compatible", buffer,
> > -                                  sizeof (buffer), NULL) &&
> > -      !grub_strcmp (buffer, "IBM,vtpm20"))
> > -    grub_ieee1275_tpm_version = 2;
> > -}
> > +grub_ieee1275_ihandle_t grub_ieee1275_tpm_ihandle = IEEE1275_IHANDLE_INVALID;
> >
> >   grub_err_t
> >   grub_ieee1275_tpm_init (void)
> >   {
> > -  static int init_success = 0;
> > +  grub_ieee1275_phandle_t vtpm;
> > +  static int init_tried = 0;
>
> We have a bool in the GRUB. Please use it...
>
> > +  char buffer[20];
> >
> > -  if (!init_success)
> > +  if (!init_tried)
> >       {
> > -      if (grub_ieee1275_open ("/vdevice/vtpm", &grub_ieee1275_tpm_ihandle) < 0)
> > -       {
> > -         grub_ieee1275_tpm_ihandle = IEEE1275_IHANDLE_INVALID;
> > -         return GRUB_ERR_UNKNOWN_DEVICE;
> > -       }
> > -
> > -      init_success = 1;
> > -
> > -      tpm_get_tpm_version ();
> > +      init_tried = 1;
> > +
> > +      if (grub_ieee1275_open ("/vdevice/vtpm",
> > +                             &grub_ieee1275_tpm_ihandle) < 0 ||
>
> You do not need wrap this line...
>
> > +         grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) ||
> > +         grub_ieee1275_get_property (vtpm, "compatible", buffer,
> > +                                     sizeof (buffer), NULL) ||
>
> Same here...
>
> > +         grub_strcmp (buffer, "IBM,vtpm20"))
>
> Should not you call grub_ieee1275_close() before invalidating
> grub_ieee1275_tpm_ihandle?
>
> > +       grub_ieee1275_tpm_ihandle = IEEE1275_IHANDLE_INVALID;
> >       }
> >
> > +  if (grub_ieee1275_tpm_ihandle == IEEE1275_IHANDLE_INVALID)
> > +    return GRUB_ERR_UNKNOWN_DEVICE;
> > +
> >     return GRUB_ERR_NONE;
> >   }
>
> Daniel
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel