[PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
platform. With this patch grub now measures text and binary data
into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
does.

This patch requires Daniel Axtens's patches for claiming more memory.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 grub-core/Makefile.core.def           |   8 ++
 grub-core/commands/ieee1275/ibmvtpm.c | 118 ++++++++++++++++++++++++++
 grub-core/kern/ieee1275/ibmvtpm.c     |  62 ++++++++++++++
 include/grub/ieee1275/ibmvtpm.h       |  32 +++++++
 4 files changed, 220 insertions(+)
 create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
 create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
 create mode 100644 include/grub/ieee1275/ibmvtpm.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 3f3459b2c..e2a64f8ff 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1166,6 +1166,14 @@ module = {
   enable = powerpc_ieee1275;
 };
 
+module = {
+  name = tpm;
+  common = commands/tpm.c;
+  common = kern/ieee1275/ibmvtpm.c;
+  ieee1275 = commands/ieee1275/ibmvtpm.c;
+  enable = powerpc_ieee1275;
+};
+
 module = {
   name = terminal;
   common = commands/terminal.c;
diff --git a/grub-core/commands/ieee1275/ibmvtpm.c b/grub-core/commands/ieee1275/ibmvtpm.c
new file mode 100644
index 000000000..9b06c76d9
--- /dev/null
+++ b/grub-core/commands/ieee1275/ibmvtpm.c
@@ -0,0 +1,118 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2021  IBM Corporation
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ *  IBM vTPM support code.
+ */
+
+#include <grub/err.h>
+#include <grub/types.h>
+#include <grub/tpm.h>
+#include <grub/ieee1275/ieee1275.h>
+#include <grub/ieee1275/ibmvtpm.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+
+static grub_ieee1275_ihandle_t grub_tpm_ihandle;
+static grub_uint8_t grub_tpm_version;
+
+static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)
+{
+  static int init_called = 0;
+
+  if (!init_called) {
+      init_called = 1;
+      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
+  }
+
+  *tpm_ihandle = grub_tpm_ihandle;
+}
+
+static grub_err_t
+grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
+{
+  static int version_probed = 0;
+  grub_ieee1275_phandle_t vtpm;
+  char buffer[20];
+  grub_ssize_t buffer_size;
+
+  if (!version_probed) {
+     version_probed = 1;
+     if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
+         !grub_ieee1275_get_property (vtpm, "compatible", buffer,
+                                      sizeof buffer, &buffer_size) &&
+         !grub_strcmp (buffer, "IBM,vtpm20")) {
+       grub_tpm_version = 2;
+    }
+  }
+  *protocol_version = grub_tpm_version;
+
+  return 0;
+}
+
+static grub_int8_t
+grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
+		      grub_uint8_t *protocol_version)
+{
+  grub_ieee1275_tpm_init (tpm_handle);
+  if (*tpm_handle == NULL)
+      return 0;
+
+  grub_tpm_get_tpm_version (protocol_version);
+
+  return 1;
+}
+
+static grub_err_t
+grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
+		     grub_size_t size, grub_uint8_t pcr,
+		     const char *description)
+{
+  static int error_displayed;
+  bool succ;
+
+  succ = grub_ieee1275_ibmvtpm_2hash_ext_log (tpm_handle,
+                                           pcr, EV_IPL,
+                                           description,
+                                           grub_strlen(description) + 1,
+                                           buf, size);
+  if (!succ && !error_displayed) {
+    error_displayed = 1;
+    grub_printf("2HASH-EXT-LOG failed: Firmware is likely too old.\n");
+  }
+
+  return 0;
+}
+
+grub_err_t
+grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
+		    const char *description)
+{
+  grub_ieee1275_ihandle_t tpm_handle;
+  grub_uint8_t protocol_version = 0;
+
+  /* Absence of a TPM isn't a failure. */
+  if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
+    return 0;
+
+  grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n",
+                pcr, size, description);
+
+  if (protocol_version == 2)
+      return grub_tpm2_log_event (tpm_handle, buf, size, pcr, description);
+
+  return 0;
+}
diff --git a/grub-core/kern/ieee1275/ibmvtpm.c b/grub-core/kern/ieee1275/ibmvtpm.c
new file mode 100644
index 000000000..525a792b4
--- /dev/null
+++ b/grub-core/kern/ieee1275/ibmvtpm.c
@@ -0,0 +1,62 @@
+/* ibmvtpm.c - Client interface to access the IBM vTPM */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2021  IBM Corporation
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/types.h>
+#include <grub/misc.h>
+#include <grub/ieee1275/ieee1275.h>
+#include <grub/ieee1275/ibmvtpm.h>
+
+bool
+grub_ieee1275_ibmvtpm_2hash_ext_log (grub_uint32_t ihandle,
+                                grub_uint8_t pcrindex,
+                                grub_uint32_t eventtype,
+                                const char *description,
+                                grub_size_t description_size,
+                                void *buf, grub_size_t size)
+{
+  struct tpm_2hash_ext_log
+  {
+    struct grub_ieee1275_common_hdr common;
+    grub_ieee1275_cell_t method;
+    grub_ieee1275_cell_t ihandle;
+    grub_ieee1275_cell_t size;
+    void *buf;
+    grub_ieee1275_cell_t description_size;
+    const char *description;
+    grub_ieee1275_cell_t eventtype;
+    grub_ieee1275_cell_t pcrindex;
+    grub_ieee1275_cell_t catch_result;
+    grub_ieee1275_cell_t rc;
+  }
+  args;
+
+  INIT_IEEE1275_COMMON (&args.common, "call-method", 8, 2);
+  args.method = (grub_ieee1275_cell_t) "2hash-ext-log";
+  args.ihandle = ihandle;
+  args.pcrindex = pcrindex;
+  args.eventtype = eventtype;
+  args.description = description;
+  args.description_size = description_size;
+  args.buf = buf;
+  args.size = (grub_ieee1275_cell_t) size;
+
+  if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
+    return false;
+  return !!args.rc;
+}
diff --git a/include/grub/ieee1275/ibmvtpm.h b/include/grub/ieee1275/ibmvtpm.h
new file mode 100644
index 000000000..bde4f9e69
--- /dev/null
+++ b/include/grub/ieee1275/ibmvtpm.h
@@ -0,0 +1,32 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2021  IBM Corporation
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_IEEE1275_IBMVTPM_HEADER
+#define GRUB_IEEE1275_IBMVTPM_HEADER 1
+
+#include <stdbool.h>
+
+bool EXPORT_FUNC(grub_ieee1275_ibmvtpm_2hash_ext_log) (
+                                  grub_uint32_t ihandle,
+                                  grub_uint8_t pcrindex,
+                                  grub_uint32_t eventtype,
+                                  const char *description,
+                                  grub_size_t description_size,
+                                  void *buf, grub_size_t size);
+
+#endif
-- 
2.31.1

Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Daniel Kiper]

CC-ing folks CC-ed in Daniel's patch series and Eric.

On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
> platform. With this patch grub now measures text and binary data
> into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
> does.
>
> This patch requires Daniel Axtens's patches for claiming more memory.

Would not be it simpler if you take out the memory mgmt changes patches
from Daniel's patch series?

Daniel, are you OK with it?

Additionally, please CC Eric when you post IEEE1275 patches next time.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  grub-core/Makefile.core.def           |   8 ++
>  grub-core/commands/ieee1275/ibmvtpm.c | 118 ++++++++++++++++++++++++++
>  grub-core/kern/ieee1275/ibmvtpm.c     |  62 ++++++++++++++
>  include/grub/ieee1275/ibmvtpm.h       |  32 +++++++
>  4 files changed, 220 insertions(+)
>  create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>  create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>  create mode 100644 include/grub/ieee1275/ibmvtpm.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 3f3459b2c..e2a64f8ff 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1166,6 +1166,14 @@ module = {
>    enable = powerpc_ieee1275;
>  };
>
> +module = {
> +  name = tpm;
> +  common = commands/tpm.c;
> +  common = kern/ieee1275/ibmvtpm.c;
> +  ieee1275 = commands/ieee1275/ibmvtpm.c;

s/ieee1275/common/?

> +  enable = powerpc_ieee1275;
> +};
> +
>  module = {
>    name = terminal;
>    common = commands/terminal.c;
> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c b/grub-core/commands/ieee1275/ibmvtpm.c
> new file mode 100644
> index 000000000..9b06c76d9
> --- /dev/null
> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
> @@ -0,0 +1,118 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2021  IBM Corporation

Copyright (C) 2021  Free Software Foundation, Inc.

Or both if you strongly need IBM...

> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + *  IBM vTPM support code.
> + */
> +
> +#include <grub/err.h>
> +#include <grub/types.h>
> +#include <grub/tpm.h>
> +#include <grub/ieee1275/ieee1275.h>
> +#include <grub/ieee1275/ibmvtpm.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
> +static grub_uint8_t grub_tpm_version;
> +
> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)

You can drop from static functions, variables, etc. names "grub_" prefix.

> +{
> +  static int init_called = 0;
> +
> +  if (!init_called) {
> +      init_called = 1;
> +      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
> +  }

Incorrect formatting. It should be

if (!init_called)
  {
    init_called = 1;
    grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
  }

You can always refer to grub-core/kern/efi/sb.c to get formatting
examples for various pieces of the GRUB code.

Please fix similar issues below too.

> +  *tpm_ihandle = grub_tpm_ihandle;
> +}
> +
> +static grub_err_t
> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
> +{
> +  static int version_probed = 0;
> +  grub_ieee1275_phandle_t vtpm;
> +  char buffer[20];
> +  grub_ssize_t buffer_size;
> +
> +  if (!version_probed) {
> +     version_probed = 1;
> +     if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
> +         !grub_ieee1275_get_property (vtpm, "compatible", buffer,
> +                                      sizeof buffer, &buffer_size) &&
> +         !grub_strcmp (buffer, "IBM,vtpm20")) {
> +       grub_tpm_version = 2;
> +    }
> +  }
> +  *protocol_version = grub_tpm_version;
> +
> +  return 0;

You ignore this value later.

I think the prototype of this function should be following:
  grub_uint8_t get_tpm_version (void)

Which means you should return the TPM version.

> +}
> +
> +static grub_int8_t

static grub_err_t

> +grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
> +		      grub_uint8_t *protocol_version)
> +{
> +  grub_ieee1275_tpm_init (tpm_handle);
> +  if (*tpm_handle == NULL)
> +      return 0;

return GRUB_ERR_UNKNOWN_DEVICE;

> +  grub_tpm_get_tpm_version (protocol_version);
> +
> +  return 1;

return GRUB_ERR_NONE;

> +}
> +
> +static grub_err_t
> +grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
> +		     grub_size_t size, grub_uint8_t pcr,
> +		     const char *description)
> +{
> +  static int error_displayed;
> +  bool succ;

s/bool/grub_err_t/

> +
> +  succ = grub_ieee1275_ibmvtpm_2hash_ext_log (tpm_handle,
> +                                           pcr, EV_IPL,
> +                                           description,
> +                                           grub_strlen(description) + 1,
> +                                           buf, size);
> +  if (!succ && !error_displayed) {
> +    error_displayed = 1;
> +    grub_printf("2HASH-EXT-LOG failed: Firmware is likely too old.\n");

s/grub_printf/grub_error/

> +  }
> +
> +  return 0;
> +}
> +
> +grub_err_t
> +grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
> +		    const char *description)
> +{
> +  grub_ieee1275_ihandle_t tpm_handle;
> +  grub_uint8_t protocol_version = 0;

It seems to me that you have the same information in the grub_tpm_version
global variable. So, I think you should you use it and drop all instances
of protocol_version.

> +  /* Absence of a TPM isn't a failure. */
> +  if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
> +    return 0;
> +
> +  grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n",
> +                pcr, size, description);
> +
> +  if (protocol_version == 2)
> +      return grub_tpm2_log_event (tpm_handle, buf, size, pcr, description);
> +
> +  return 0;
> +}
> diff --git a/grub-core/kern/ieee1275/ibmvtpm.c b/grub-core/kern/ieee1275/ibmvtpm.c
> new file mode 100644
> index 000000000..525a792b4
> --- /dev/null
> +++ b/grub-core/kern/ieee1275/ibmvtpm.c
> @@ -0,0 +1,62 @@
> +/* ibmvtpm.c - Client interface to access the IBM vTPM */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2021  IBM Corporation
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/types.h>
> +#include <grub/misc.h>
> +#include <grub/ieee1275/ieee1275.h>
> +#include <grub/ieee1275/ibmvtpm.h>
> +
> +bool

s/bool/grub_err_t/

> +grub_ieee1275_ibmvtpm_2hash_ext_log (grub_uint32_t ihandle,
> +                                grub_uint8_t pcrindex,
> +                                grub_uint32_t eventtype,
> +                                const char *description,
> +                                grub_size_t description_size,
> +                                void *buf, grub_size_t size)

I cannot see any reason to make this function part of the GRUB kernel.
I think it should be in grub-core/commands/ieee1275/ibmvtpm.c.

> +{
> +  struct tpm_2hash_ext_log

Please define this struct before the function, somewhere after includes.

> +  {
> +    struct grub_ieee1275_common_hdr common;
> +    grub_ieee1275_cell_t method;
> +    grub_ieee1275_cell_t ihandle;
> +    grub_ieee1275_cell_t size;
> +    void *buf;
> +    grub_ieee1275_cell_t description_size;
> +    const char *description;
> +    grub_ieee1275_cell_t eventtype;
> +    grub_ieee1275_cell_t pcrindex;
> +    grub_ieee1275_cell_t catch_result;
> +    grub_ieee1275_cell_t rc;
> +  }

GRUB_PACKED?

> +  args;
> +
> +  INIT_IEEE1275_COMMON (&args.common, "call-method", 8, 2);
> +  args.method = (grub_ieee1275_cell_t) "2hash-ext-log";
> +  args.ihandle = ihandle;
> +  args.pcrindex = pcrindex;
> +  args.eventtype = eventtype;
> +  args.description = description;
> +  args.description_size = description_size;
> +  args.buf = buf;
> +  args.size = (grub_ieee1275_cell_t) size;
> +
> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
> +    return false;
> +  return !!args.rc;
> +}

Daniel

Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Stefan Berger]

On 7/14/21 12:16 PM, Daniel Kiper wrote:
> CC-ing folks CC-ed in Daniel's patch series and Eric.
>
> On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
>> platform. With this patch grub now measures text and binary data
>> into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
>> does.
>>
>> This patch requires Daniel Axtens's patches for claiming more memory.
> Would not be it simpler if you take out the memory mgmt changes patches
> from Daniel's patch series?
You mean reposting this series with his 3 patches upfront? I can 
certainly do that.
>
> Daniel, are you OK with it?
>
> Additionally, please CC Eric when you post IEEE1275 patches next time.


Will do.


>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   grub-core/Makefile.core.def           |   8 ++
>>   grub-core/commands/ieee1275/ibmvtpm.c | 118 ++++++++++++++++++++++++++
>>   grub-core/kern/ieee1275/ibmvtpm.c     |  62 ++++++++++++++
>>   include/grub/ieee1275/ibmvtpm.h       |  32 +++++++
>>   4 files changed, 220 insertions(+)
>>   create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>>   create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>>   create mode 100644 include/grub/ieee1275/ibmvtpm.h
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 3f3459b2c..e2a64f8ff 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1166,6 +1166,14 @@ module = {
>>     enable = powerpc_ieee1275;
>>   };
>>
>> +module = {
>> +  name = tpm;
>> +  common = commands/tpm.c;
>> +  common = kern/ieee1275/ibmvtpm.c;
>> +  ieee1275 = commands/ieee1275/ibmvtpm.c;
> s/ieee1275/common/?
>
>> +  enable = powerpc_ieee1275;
>> +};
>> +
>>   module = {
>>     name = terminal;
>>     common = commands/terminal.c;
>> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c b/grub-core/commands/ieee1275/ibmvtpm.c
>> new file mode 100644
>> index 000000000..9b06c76d9
>> --- /dev/null
>> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2021  IBM Corporation
> Copyright (C) 2021  Free Software Foundation, Inc.
>
> Or both if you strongly need IBM...
>
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + *  IBM vTPM support code.
>> + */
>> +
>> +#include <grub/err.h>
>> +#include <grub/types.h>
>> +#include <grub/tpm.h>
>> +#include <grub/ieee1275/ieee1275.h>
>> +#include <grub/ieee1275/ibmvtpm.h>
>> +#include <grub/mm.h>
>> +#include <grub/misc.h>
>> +
>> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
>> +static grub_uint8_t grub_tpm_version;
>> +
>> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)
> You can drop from static functions, variables, etc. names "grub_" prefix.
>
>> +{
>> +  static int init_called = 0;
>> +
>> +  if (!init_called) {
>> +      init_called = 1;
>> +      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>> +  }
> Incorrect formatting. It should be
>
> if (!init_called)
>    {
>      init_called = 1;
>      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>    }
>
> You can always refer to grub-core/kern/efi/sb.c to get formatting
> examples for various pieces of the GRUB code.
>
> Please fix similar issues below too.
>
>> +  *tpm_ihandle = grub_tpm_ihandle;
>> +}
>> +
>> +static grub_err_t
>> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
>> +{
>> +  static int version_probed = 0;
>> +  grub_ieee1275_phandle_t vtpm;
>> +  char buffer[20];
>> +  grub_ssize_t buffer_size;
>> +
>> +  if (!version_probed) {
>> +     version_probed = 1;
>> +     if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
>> +         !grub_ieee1275_get_property (vtpm, "compatible", buffer,
>> +                                      sizeof buffer, &buffer_size) &&
>> +         !grub_strcmp (buffer, "IBM,vtpm20")) {
>> +       grub_tpm_version = 2;
>> +    }
>> +  }
>> +  *protocol_version = grub_tpm_version;
>> +
>> +  return 0;
> You ignore this value later.
>
> I think the prototype of this function should be following:
>    grub_uint8_t get_tpm_version (void)
>
> Which means you should return the TPM version.
>
>> +}
>> +
>> +static grub_int8_t
> static grub_err_t
>
>> +grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
>> +		      grub_uint8_t *protocol_version)
>> +{
>> +  grub_ieee1275_tpm_init (tpm_handle);
>> +  if (*tpm_handle == NULL)
>> +      return 0;
> return GRUB_ERR_UNKNOWN_DEVICE;
>
>> +  grub_tpm_get_tpm_version (protocol_version);
>> +
>> +  return 1;
> return GRUB_ERR_NONE;
>
>> +}
>> +
>> +static grub_err_t
>> +grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
>> +		     grub_size_t size, grub_uint8_t pcr,
>> +		     const char *description)
>> +{
>> +  static int error_displayed;
>> +  bool succ;
> s/bool/grub_err_t/
>
>> +
>> +  succ = grub_ieee1275_ibmvtpm_2hash_ext_log (tpm_handle,
>> +                                           pcr, EV_IPL,
>> +                                           description,
>> +                                           grub_strlen(description) + 1,
>> +                                           buf, size);
>> +  if (!succ && !error_displayed) {
>> +    error_displayed = 1;
>> +    grub_printf("2HASH-EXT-LOG failed: Firmware is likely too old.\n");
> s/grub_printf/grub_error/
>
>> +  }
>> +
>> +  return 0;
>> +}
>> +
>> +grub_err_t
>> +grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
>> +		    const char *description)
>> +{
>> +  grub_ieee1275_ihandle_t tpm_handle;
>> +  grub_uint8_t protocol_version = 0;
> It seems to me that you have the same information in the grub_tpm_version
> global variable. So, I think you should you use it and drop all instances
> of protocol_version.
>
>> +  /* Absence of a TPM isn't a failure. */
>> +  if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
>> +    return 0;
>> +
>> +  grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n",
>> +                pcr, size, description);
>> +
>> +  if (protocol_version == 2)
>> +      return grub_tpm2_log_event (tpm_handle, buf, size, pcr, description);
>> +
>> +  return 0;
>> +}
>> diff --git a/grub-core/kern/ieee1275/ibmvtpm.c b/grub-core/kern/ieee1275/ibmvtpm.c
>> new file mode 100644
>> index 000000000..525a792b4
>> --- /dev/null
>> +++ b/grub-core/kern/ieee1275/ibmvtpm.c
>> @@ -0,0 +1,62 @@
>> +/* ibmvtpm.c - Client interface to access the IBM vTPM */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2021  IBM Corporation
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/types.h>
>> +#include <grub/misc.h>
>> +#include <grub/ieee1275/ieee1275.h>
>> +#include <grub/ieee1275/ibmvtpm.h>
>> +
>> +bool
> s/bool/grub_err_t/
>
>> +grub_ieee1275_ibmvtpm_2hash_ext_log (grub_uint32_t ihandle,
>> +                                grub_uint8_t pcrindex,
>> +                                grub_uint32_t eventtype,
>> +                                const char *description,
>> +                                grub_size_t description_size,
>> +                                void *buf, grub_size_t size)
> I cannot see any reason to make this function part of the GRUB kernel.
> I think it should be in grub-core/commands/ieee1275/ibmvtpm.c.
>
>> +{
>> +  struct tpm_2hash_ext_log
> Please define this struct before the function, somewhere after includes.
>
>> +  {
>> +    struct grub_ieee1275_common_hdr common;
>> +    grub_ieee1275_cell_t method;
>> +    grub_ieee1275_cell_t ihandle;
>> +    grub_ieee1275_cell_t size;
>> +    void *buf;
>> +    grub_ieee1275_cell_t description_size;
>> +    const char *description;
>> +    grub_ieee1275_cell_t eventtype;
>> +    grub_ieee1275_cell_t pcrindex;
>> +    grub_ieee1275_cell_t catch_result;
>> +    grub_ieee1275_cell_t rc;
>> +  }
> GRUB_PACKED?
>
>> +  args;
>> +
>> +  INIT_IEEE1275_COMMON (&args.common, "call-method", 8, 2);
>> +  args.method = (grub_ieee1275_cell_t) "2hash-ext-log";
>> +  args.ihandle = ihandle;
>> +  args.pcrindex = pcrindex;
>> +  args.eventtype = eventtype;
>> +  args.description = description;
>> +  args.description_size = description_size;
>> +  args.buf = buf;
>> +  args.size = (grub_ieee1275_cell_t) size;
>> +
>> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
>> +    return false;
>> +  return !!args.rc;
>> +}
> Daniel

Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Daniel Axtens]

Stefan Berger <stefanb@linux.ibm.com> writes:

> On 7/14/21 12:16 PM, Daniel Kiper wrote:
>> CC-ing folks CC-ed in Daniel's patch series and Eric.
>>
>> On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:
>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>
>>> Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
>>> platform. With this patch grub now measures text and binary data
>>> into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
>>> does.
>>>
>>> This patch requires Daniel Axtens's patches for claiming more memory.
>> Would not be it simpler if you take out the memory mgmt changes patches
>> from Daniel's patch series?
> You mean reposting this series with his 3 patches upfront? I can 
> certainly do that.
>>
>> Daniel, are you OK with it?
>>

I don't mind what series the patches go through, as long as they get
merged at some point :)

Kind regards,
Daniel

>> Additionally, please CC Eric when you post IEEE1275 patches next time.
>
>
> Will do.
>
>
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   grub-core/Makefile.core.def           |   8 ++
>>>   grub-core/commands/ieee1275/ibmvtpm.c | 118 ++++++++++++++++++++++++++
>>>   grub-core/kern/ieee1275/ibmvtpm.c     |  62 ++++++++++++++
>>>   include/grub/ieee1275/ibmvtpm.h       |  32 +++++++
>>>   4 files changed, 220 insertions(+)
>>>   create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>>>   create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>>>   create mode 100644 include/grub/ieee1275/ibmvtpm.h
>>>
>>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>>> index 3f3459b2c..e2a64f8ff 100644
>>> --- a/grub-core/Makefile.core.def
>>> +++ b/grub-core/Makefile.core.def
>>> @@ -1166,6 +1166,14 @@ module = {
>>>     enable = powerpc_ieee1275;
>>>   };
>>>
>>> +module = {
>>> +  name = tpm;
>>> +  common = commands/tpm.c;
>>> +  common = kern/ieee1275/ibmvtpm.c;
>>> +  ieee1275 = commands/ieee1275/ibmvtpm.c;
>> s/ieee1275/common/?
>>
>>> +  enable = powerpc_ieee1275;
>>> +};
>>> +
>>>   module = {
>>>     name = terminal;
>>>     common = commands/terminal.c;
>>> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c b/grub-core/commands/ieee1275/ibmvtpm.c
>>> new file mode 100644
>>> index 000000000..9b06c76d9
>>> --- /dev/null
>>> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + *  GRUB  --  GRand Unified Bootloader
>>> + *  Copyright (C) 2021  IBM Corporation
>> Copyright (C) 2021  Free Software Foundation, Inc.
>>
>> Or both if you strongly need IBM...
>>
>>> + *  GRUB is free software: you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation, either version 3 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  GRUB is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + *  You should have received a copy of the GNU General Public License
>>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + *  IBM vTPM support code.
>>> + */
>>> +
>>> +#include <grub/err.h>
>>> +#include <grub/types.h>
>>> +#include <grub/tpm.h>
>>> +#include <grub/ieee1275/ieee1275.h>
>>> +#include <grub/ieee1275/ibmvtpm.h>
>>> +#include <grub/mm.h>
>>> +#include <grub/misc.h>
>>> +
>>> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
>>> +static grub_uint8_t grub_tpm_version;
>>> +
>>> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)
>> You can drop from static functions, variables, etc. names "grub_" prefix.
>>
>>> +{
>>> +  static int init_called = 0;
>>> +
>>> +  if (!init_called) {
>>> +      init_called = 1;
>>> +      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>>> +  }
>> Incorrect formatting. It should be
>>
>> if (!init_called)
>>    {
>>      init_called = 1;
>>      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>>    }
>>
>> You can always refer to grub-core/kern/efi/sb.c to get formatting
>> examples for various pieces of the GRUB code.
>>
>> Please fix similar issues below too.
>>
>>> +  *tpm_ihandle = grub_tpm_ihandle;
>>> +}
>>> +
>>> +static grub_err_t
>>> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
>>> +{
>>> +  static int version_probed = 0;
>>> +  grub_ieee1275_phandle_t vtpm;
>>> +  char buffer[20];
>>> +  grub_ssize_t buffer_size;
>>> +
>>> +  if (!version_probed) {
>>> +     version_probed = 1;
>>> +     if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
>>> +         !grub_ieee1275_get_property (vtpm, "compatible", buffer,
>>> +                                      sizeof buffer, &buffer_size) &&
>>> +         !grub_strcmp (buffer, "IBM,vtpm20")) {
>>> +       grub_tpm_version = 2;
>>> +    }
>>> +  }
>>> +  *protocol_version = grub_tpm_version;
>>> +
>>> +  return 0;
>> You ignore this value later.
>>
>> I think the prototype of this function should be following:
>>    grub_uint8_t get_tpm_version (void)
>>
>> Which means you should return the TPM version.
>>
>>> +}
>>> +
>>> +static grub_int8_t
>> static grub_err_t
>>
>>> +grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
>>> +		      grub_uint8_t *protocol_version)
>>> +{
>>> +  grub_ieee1275_tpm_init (tpm_handle);
>>> +  if (*tpm_handle == NULL)
>>> +      return 0;
>> return GRUB_ERR_UNKNOWN_DEVICE;
>>
>>> +  grub_tpm_get_tpm_version (protocol_version);
>>> +
>>> +  return 1;
>> return GRUB_ERR_NONE;
>>
>>> +}
>>> +
>>> +static grub_err_t
>>> +grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
>>> +		     grub_size_t size, grub_uint8_t pcr,
>>> +		     const char *description)
>>> +{
>>> +  static int error_displayed;
>>> +  bool succ;
>> s/bool/grub_err_t/
>>
>>> +
>>> +  succ = grub_ieee1275_ibmvtpm_2hash_ext_log (tpm_handle,
>>> +                                           pcr, EV_IPL,
>>> +                                           description,
>>> +                                           grub_strlen(description) + 1,
>>> +                                           buf, size);
>>> +  if (!succ && !error_displayed) {
>>> +    error_displayed = 1;
>>> +    grub_printf("2HASH-EXT-LOG failed: Firmware is likely too old.\n");
>> s/grub_printf/grub_error/
>>
>>> +  }
>>> +
>>> +  return 0;
>>> +}
>>> +
>>> +grub_err_t
>>> +grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
>>> +		    const char *description)
>>> +{
>>> +  grub_ieee1275_ihandle_t tpm_handle;
>>> +  grub_uint8_t protocol_version = 0;
>> It seems to me that you have the same information in the grub_tpm_version
>> global variable. So, I think you should you use it and drop all instances
>> of protocol_version.
>>
>>> +  /* Absence of a TPM isn't a failure. */
>>> +  if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
>>> +    return 0;
>>> +
>>> +  grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n",
>>> +                pcr, size, description);
>>> +
>>> +  if (protocol_version == 2)
>>> +      return grub_tpm2_log_event (tpm_handle, buf, size, pcr, description);
>>> +
>>> +  return 0;
>>> +}
>>> diff --git a/grub-core/kern/ieee1275/ibmvtpm.c b/grub-core/kern/ieee1275/ibmvtpm.c
>>> new file mode 100644
>>> index 000000000..525a792b4
>>> --- /dev/null
>>> +++ b/grub-core/kern/ieee1275/ibmvtpm.c
>>> @@ -0,0 +1,62 @@
>>> +/* ibmvtpm.c - Client interface to access the IBM vTPM */
>>> +/*
>>> + *  GRUB  --  GRand Unified Bootloader
>>> + *  Copyright (C) 2021  IBM Corporation
>>> + *
>>> + *  GRUB is free software: you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation, either version 3 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  GRUB is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + *  You should have received a copy of the GNU General Public License
>>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <grub/types.h>
>>> +#include <grub/misc.h>
>>> +#include <grub/ieee1275/ieee1275.h>
>>> +#include <grub/ieee1275/ibmvtpm.h>
>>> +
>>> +bool
>> s/bool/grub_err_t/
>>
>>> +grub_ieee1275_ibmvtpm_2hash_ext_log (grub_uint32_t ihandle,
>>> +                                grub_uint8_t pcrindex,
>>> +                                grub_uint32_t eventtype,
>>> +                                const char *description,
>>> +                                grub_size_t description_size,
>>> +                                void *buf, grub_size_t size)
>> I cannot see any reason to make this function part of the GRUB kernel.
>> I think it should be in grub-core/commands/ieee1275/ibmvtpm.c.
>>
>>> +{
>>> +  struct tpm_2hash_ext_log
>> Please define this struct before the function, somewhere after includes.
>>
>>> +  {
>>> +    struct grub_ieee1275_common_hdr common;
>>> +    grub_ieee1275_cell_t method;
>>> +    grub_ieee1275_cell_t ihandle;
>>> +    grub_ieee1275_cell_t size;
>>> +    void *buf;
>>> +    grub_ieee1275_cell_t description_size;
>>> +    const char *description;
>>> +    grub_ieee1275_cell_t eventtype;
>>> +    grub_ieee1275_cell_t pcrindex;
>>> +    grub_ieee1275_cell_t catch_result;
>>> +    grub_ieee1275_cell_t rc;
>>> +  }
>> GRUB_PACKED?
>>
>>> +  args;
>>> +
>>> +  INIT_IEEE1275_COMMON (&args.common, "call-method", 8, 2);
>>> +  args.method = (grub_ieee1275_cell_t) "2hash-ext-log";
>>> +  args.ihandle = ihandle;
>>> +  args.pcrindex = pcrindex;
>>> +  args.eventtype = eventtype;
>>> +  args.description = description;
>>> +  args.description_size = description_size;
>>> +  args.buf = buf;
>>> +  args.size = (grub_ieee1275_cell_t) size;
>>> +
>>> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
>>> +    return false;
>>> +  return !!args.rc;
>>> +}
>> Daniel

Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

[Stefan Berger]

On 7/14/21 12:16 PM, Daniel Kiper wrote:
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   grub-core/Makefile.core.def           |   8 ++
>>   grub-core/commands/ieee1275/ibmvtpm.c | 118 ++++++++++++++++++++++++++
>>   grub-core/kern/ieee1275/ibmvtpm.c     |  62 ++++++++++++++
>>   include/grub/ieee1275/ibmvtpm.h       |  32 +++++++
>>   4 files changed, 220 insertions(+)
>>   create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>>   create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>>   create mode 100644 include/grub/ieee1275/ibmvtpm.h
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 3f3459b2c..e2a64f8ff 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1166,6 +1166,14 @@ module = {
>>     enable = powerpc_ieee1275;
>>   };
>>
>> +module = {
>> +  name = tpm;
>> +  common = commands/tpm.c;
>> +  common = kern/ieee1275/ibmvtpm.c;
>> +  ieee1275 = commands/ieee1275/ibmvtpm.c;
> s/ieee1275/common/?


It looks like this now:

module = {
   name = tpm;
   common = commands/tpm.c;
   ieee1275 = commands/ieee1275/ibmvtpm.c;
   enable = powerpc_ieee1275;
};


>
>> +  enable = powerpc_ieee1275;
>> +};
>> +
>>   module = {
>>     name = terminal;
>>     common = commands/terminal.c;
>> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c b/grub-core/commands/ieee1275/ibmvtpm.c
>> new file mode 100644
>> index 000000000..9b06c76d9
>> --- /dev/null
>> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2021  IBM Corporation
> Copyright (C) 2021  Free Software Foundation, Inc.
>
> Or both if you strongly need IBM...

Doing both.


>
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + *  IBM vTPM support code.
>> + */
>> +
>> +#include <grub/err.h>
>> +#include <grub/types.h>
>> +#include <grub/tpm.h>
>> +#include <grub/ieee1275/ieee1275.h>
>> +#include <grub/ieee1275/ibmvtpm.h>
>> +#include <grub/mm.h>
>> +#include <grub/misc.h>
>> +
>> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
>> +static grub_uint8_t grub_tpm_version;
>> +
>> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)
> You can drop from static functions, variables, etc. names "grub_" prefix.


Done.


>
>> +{
>> +  static int init_called = 0;
>> +
>> +  if (!init_called) {
>> +      init_called = 1;
>> +      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>> +  }
> Incorrect formatting. It should be
>
> if (!init_called)
>    {
>      init_called = 1;
>      grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>    }
>
> You can always refer to grub-core/kern/efi/sb.c to get formatting
> examples for various pieces of the GRUB code.
>
> Please fix similar issues below too.


Done.


>
>> +  *tpm_ihandle = grub_tpm_ihandle;
>> +}
>> +
>> +static grub_err_t
>> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
>> +{
>> +  static int version_probed = 0;
>> +  grub_ieee1275_phandle_t vtpm;
>> +  char buffer[20];
>> +  grub_ssize_t buffer_size;
>> +
>> +  if (!version_probed) {
>> +     version_probed = 1;
>> +     if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
>> +         !grub_ieee1275_get_property (vtpm, "compatible", buffer,
>> +                                      sizeof buffer, &buffer_size) &&
>> +         !grub_strcmp (buffer, "IBM,vtpm20")) {
>> +       grub_tpm_version = 2;
>> +    }
>> +  }
>> +  *protocol_version = grub_tpm_version;
>> +
>> +  return 0;
> You ignore this value later.
>
> I think the prototype of this function should be following:
>    grub_uint8_t get_tpm_version (void)
>
> Which means you should return the TPM version.

Done


>
>> +}
>> +
>> +static grub_int8_t
> static grub_err_t
Done
>
>> +grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
>> +		      grub_uint8_t *protocol_version)
>> +{
>> +  grub_ieee1275_tpm_init (tpm_handle);
>> +  if (*tpm_handle == NULL)
>> +      return 0;
> return GRUB_ERR_UNKNOWN_DEVICE;
Done
>
>> +  grub_tpm_get_tpm_version (protocol_version);
>> +
>> +  return 1;
> return GRUB_ERR_NONE;

Done


>
>> +}
>> +
>> +static grub_err_t
>> +grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
>> +		     grub_size_t size, grub_uint8_t pcr,
>> +		     const char *description)
>> +{
>> +  static int error_displayed;
>> +  bool succ;
> s/bool/grub_err_t/
>
>> +
>> +  succ = grub_ieee1275_ibmvtpm_2hash_ext_log (tpm_handle,
>> +                                           pcr, EV_IPL,
>> +                                           description,
>> +                                           grub_strlen(description) + 1,
>> +                                           buf, size);
>> +  if (!succ && !error_displayed) {
>> +    error_displayed = 1;
>> +    grub_printf("2HASH-EXT-LOG failed: Firmware is likely too old.\n");
> s/grub_printf/grub_error/
>
>> +  }
>> +
>> +  return 0;
>> +}
>> +
>> +grub_err_t
>> +grub_tpm_measure (unsigned char *buf, grub_size_t size, grub_uint8_t pcr,
>> +		    const char *description)
>> +{
>> +  grub_ieee1275_ihandle_t tpm_handle;
>> +  grub_uint8_t protocol_version = 0;
> It seems to me that you have the same information in the grub_tpm_version
> global variable. So, I think you should you use it and drop all instances
> of protocol_version.
Done
>
>> +  /* Absence of a TPM isn't a failure. */
>> +  if (!grub_tpm_handle_find (&tpm_handle, &protocol_version))
>> +    return 0;
>> +
>> +  grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n",
>> +                pcr, size, description);
>> +
>> +  if (protocol_version == 2)
>> +      return grub_tpm2_log_event (tpm_handle, buf, size, pcr, description);
>> +
>> +  return 0;
>> +}
>> diff --git a/grub-core/kern/ieee1275/ibmvtpm.c b/grub-core/kern/ieee1275/ibmvtpm.c
>> new file mode 100644
>> index 000000000..525a792b4
>> --- /dev/null
>> +++ b/grub-core/kern/ieee1275/ibmvtpm.c
>> @@ -0,0 +1,62 @@
>> +/* ibmvtpm.c - Client interface to access the IBM vTPM */
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2021  IBM Corporation
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/types.h>
>> +#include <grub/misc.h>
>> +#include <grub/ieee1275/ieee1275.h>
>> +#include <grub/ieee1275/ibmvtpm.h>
>> +
>> +bool
> s/bool/grub_err_t/


All the other ones ( grub-core/kern/ieee1275/ieee1275.c ) actually have 
int. Using int now.


>
>> +grub_ieee1275_ibmvtpm_2hash_ext_log (grub_uint32_t ihandle,
>> +                                grub_uint8_t pcrindex,
>> +                                grub_uint32_t eventtype,
>> +                                const char *description,
>> +                                grub_size_t description_size,
>> +                                void *buf, grub_size_t size)
> I cannot see any reason to make this function part of the GRUB kernel.
> I think it should be in grub-core/commands/ieee1275/ibmvtpm.c.
Moved it.
>
>> +{
>> +  struct tpm_2hash_ext_log
> Please define this struct before the function, somewhere after includes.
Done.
>
>> +  {
>> +    struct grub_ieee1275_common_hdr common;
>> +    grub_ieee1275_cell_t method;
>> +    grub_ieee1275_cell_t ihandle;
>> +    grub_ieee1275_cell_t size;
>> +    void *buf;
>> +    grub_ieee1275_cell_t description_size;
>> +    const char *description;
>> +    grub_ieee1275_cell_t eventtype;
>> +    grub_ieee1275_cell_t pcrindex;
>> +    grub_ieee1275_cell_t catch_result;
>> +    grub_ieee1275_cell_t rc;
>> +  }
> GRUB_PACKED?

Per the other examples in the code this doesn't seem to be necessary. I 
also replaced void * and char * with the grub_ieee1275_cell_t.


>
>> +  args;
>> +
>> +  INIT_IEEE1275_COMMON (&args.common, "call-method", 8, 2);
>> +  args.method = (grub_ieee1275_cell_t) "2hash-ext-log";
>> +  args.ihandle = ihandle;
>> +  args.pcrindex = pcrindex;
>> +  args.eventtype = eventtype;
>> +  args.description = description;
>> +  args.description_size = description_size;
>> +  args.buf = buf;
>> +  args.size = (grub_ieee1275_cell_t) size;
>> +
>> +  if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
>> +    return false;
>> +  return !!args.rc;
>> +}
> Daniel


Thanks.