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

[Daniel Kiper]

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

> 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