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