From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1m8jZd-0007AZ-W9 for mharc-grub-devel@gnu.org; Wed, 28 Jul 2021 09:26:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46984) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m8jZZ-000743-1l for grub-devel@gnu.org; Wed, 28 Jul 2021 09:26:09 -0400 Received: from dibed.net-space.pl ([84.10.22.86]:42561) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:192) (Exim 4.90_1) (envelope-from ) id 1m8jZW-0007VY-Qr for grub-devel@gnu.org; Wed, 28 Jul 2021 09:26:08 -0400 Received: from router-fw.i.net-space.pl ([192.168.52.1]:45662 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S2106079AbhG1NZ6 (ORCPT ); Wed, 28 Jul 2021 15:25:58 +0200 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Wed, 28 Jul 2021 15:25:56 +0200 From: Daniel Kiper To: Stefan Berger Cc: grub-devel@gnu.org, Stefan Berger , Eric Snowberg Subject: Re: [PATCH v2 4/4] ibmvtpm: Add support for trusted boot using a vTPM 2.0 Message-ID: <20210728132556.xxcu5ua3w6jekcro@tomti.i.net-space.pl> References: <20210720211449.572356-1-stefanb@linux.vnet.ibm.com> <20210720211449.572356-5-stefanb@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210720211449.572356-5-stefanb@linux.vnet.ibm.com> User-Agent: NeoMutt/20170113 (1.7.2) Received-SPF: pass client-ip=84.10.22.86; envelope-from=dkiper@net-space.pl; helo=dibed.net-space.pl X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jul 2021 13:26:09 -0000 On Tue, Jul 20, 2021 at 05:14:49PM -0400, Stefan Berger wrote: > From: Stefan Berger > > Add support for trusted boot using a vTPM 2.0 on the IBM IEEE1275 > PowerPC 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. > > Cc: Eric Snowberg > Signed-off-by: Stefan Berger > --- > docs/grub.texi | 3 +- > grub-core/Makefile.core.def | 7 ++ > grub-core/commands/ieee1275/ibmvtpm.c | 169 ++++++++++++++++++++++++++ > 3 files changed, 178 insertions(+), 1 deletion(-) > create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c > > diff --git a/docs/grub.texi b/docs/grub.texi > index f8b4b3b21..23d2f2d90 100644 > --- a/docs/grub.texi > +++ b/docs/grub.texi > @@ -6023,7 +6023,8 @@ tpm module is loaded. As such it is recommended that the tpm module be built > into @file{core.img} in order to avoid a potential gap in measurement between > @file{core.img} being loaded and the tpm module being loaded. > > -Measured boot is currently only supported on EFI platforms. > +Measured boot is currently only supported on EFI and IBM IEEE1275 PowerPC > +platforms. > > @node Lockdown > @section Lockdown when booting on a secure setup > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def > index 8022e1c0a..e6ac50704 100644 > --- a/grub-core/Makefile.core.def > +++ b/grub-core/Makefile.core.def > @@ -1118,6 +1118,13 @@ module = { > enable = powerpc_ieee1275; > }; > > +module = { > + name = tpm; > + common = commands/tpm.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..d61cb111a > --- /dev/null > +++ b/grub-core/commands/ieee1275/ibmvtpm.c > @@ -0,0 +1,169 @@ > +/* > + * GRUB -- GRand Unified Bootloader > + * Copyright (C) 2021 Free Software Foundation, Inc. > + * 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 . > + * > + * IBM vTPM support code. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +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; > + grub_ieee1275_cell_t buf; > + grub_ieee1275_cell_t description_size; > + grub_ieee1275_cell_t description; > + grub_ieee1275_cell_t eventtype; > + grub_ieee1275_cell_t pcrindex; > + grub_ieee1275_cell_t catch_result; > + grub_ieee1275_cell_t rc; > +}; Huh! It looks that the struct should be defined in ibmvtpm_2hash_ext_log() as it is done in the similar GRUB IEEE1275 code. Please move it back there. Sorry for the confusion. > +#define IEEE1275_CELL_TRUE ((grub_ieee1275_cell_t) -1) This smells like global constant. Does not it? If yes could you define it in a global header and use it? Maybe even replace existing comparisons in the IEEE1275 code with IEEE1275_CELL_TRUE. But probably then s/IEEE1275_CELL_TRUE/GRUB_IEEE1275_CELL_TRUE/... > +static grub_ieee1275_ihandle_t tpm_ihandle; > +static grub_uint8_t tpm_version; > + > +static grub_ieee1275_ihandle_t > +tpm_init (void) > +{ > + static int init_called = 0; > + > + if (!init_called) > + { > + init_called = 1; I would move this behind grub_ieee1275_open() call. > + grub_ieee1275_open ("/vdevice/vtpm", &tpm_ihandle); Why do you ignore status returned by grub_ieee1275_open()? > + } > + > + return tpm_ihandle; > +} > + > +static void > +tpm_get_tpm_version (void) > +{ > + static int version_probed = 0; > + grub_ieee1275_phandle_t vtpm; > + char buffer[20]; > + grub_ssize_t buffer_size; > + > + if (!version_probed) > + { > + version_probed = 1; I would move version_probed behind this if. > + if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) && > + !grub_ieee1275_get_property (vtpm, "compatible", buffer, > + sizeof buffer, &buffer_size) && s/sizeof buffer/sizeof (buffer)/ And you can use NULL instead of buffer_size. > + !grub_strcmp (buffer, "IBM,vtpm20")) > + { > + tpm_version = 2; > + } Please drop these curly brackets. > + } > +} > + > +static grub_err_t > +tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle) > +{ > + *tpm_handle = tpm_init (); > + if (*tpm_handle == 0) > + return GRUB_ERR_UNKNOWN_DEVICE; > + > + tpm_get_tpm_version (); > + > + return GRUB_ERR_NONE; > +} > + > +static int > +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 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 = (grub_ieee1275_cell_t) description; > + args.description_size = description_size; > + args.buf = (grub_ieee1275_cell_t) buf; > + args.size = (grub_ieee1275_cell_t) size; > + > + if (IEEE1275_CALL_ENTRY_FN (&args) == -1) > + return -1; Incorrect alignment of return. > + /* > + * catch_result is set if firmware does not support 2hash-ext-log > + * rc is TRUE (-1) on success > + */ > + if ((args.catch_result) || args.rc != IEEE1275_CELL_TRUE) > + return -1; Ditto. > + return 0; > +} > + > +static grub_err_t > +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; static int error_displayed = 0; > + int err; > + > + err = ibmvtpm_2hash_ext_log (tpm_handle, > + pcr, EV_IPL, > + description, > + grub_strlen(description) + 1, > + buf, size); > + if (err && error_displayed < 1) s/error_displayed < 1/!error_displayed/ > + { > + error_displayed++; > + grub_error (GRUB_ERR_BAD_DEVICE, s/grub_error/return grub_error/ > + "2HASH-EXT-LOG failed: Firmware is likely too old.\n"); > + } > + > + return GRUB_ERR_NONE; > +} > + > +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; > + > + /* Absence of a TPM isn't a failure. */ > + if (tpm_handle_find (&tpm_handle) != GRUB_ERR_NONE) > + return GRUB_ERR_NONE; Incorrect alignment of return. > + grub_dprintf ("tpm", "log_event, pcr = %d, size = 0x%" PRIxGRUB_SIZE ", %s\n", > + pcr, size, description); > + > + if (tpm_version == 2) > + return tpm2_log_event (tpm_handle, buf, size, pcr, description); Ditto. Daniel