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.