[SLOF] [PATCH v2 02/20] Add TPM initialization support
Thomas Huth
thuth at redhat.com
Thu Nov 19 20:04:26 AEDT 2015
On 17/11/15 18:02, Stefan Berger wrote:
> From: Stefan Berger <stefanb at linux.vnet.ibm.com>
>
> This patch implements the main part of the fimrmware extensions. It provides
> the following functionality:
>
> - initialization of the TPM by sending a sequence of commands to it
> - proper setup of the TPM before the firmware hands over control to the bootloader
>
> Structures that are needed in subsequent patches are also included in the
> private header file tcgbios_int.h at this point.
>
> Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
[...]
> diff --git a/board-qemu/slof/OF.fs b/board-qemu/slof/OF.fs
> index 561d892..80bc011 100644
> --- a/board-qemu/slof/OF.fs
> +++ b/board-qemu/slof/OF.fs
> @@ -113,6 +113,10 @@ d# 512000000 VALUE tb-frequency \ default value - needed for "ms" to work
>
> #include "fdt.fs"
>
> +350 cp
> +
> +#include <tpm/tpm-static.fs>
> +
> 360 cp
>
> #include <root.fs>
> @@ -300,6 +304,8 @@ cr
> #include "copyright-oss.fs"
> cr cr
>
> +vtpm-unassert-physical-presence
> +
> \ this CATCH is to ensure the code bellow always executes: boot may ABORT!
> ' start-it CATCH drop
>
> diff --git a/board-qemu/slof/tree.fs b/board-qemu/slof/tree.fs
> index 4aba4c5..b71009d 100644
> --- a/board-qemu/slof/tree.fs
> +++ b/board-qemu/slof/tree.fs
> @@ -83,6 +83,9 @@ include fbuffer.fs
> 2dup " qemu,spapr-nvram" strequal IF
> " rtas-nvram.fs" included
> THEN
> + 2dup " IBM,vtpm" strequal IF
> + " vio-vtpm-cdriver.fs" included
> + THEN
> 2drop
> THEN
> peer
> diff --git a/board-qemu/slof/vio-vtpm-cdriver.fs b/board-qemu/slof/vio-vtpm-cdriver.fs
> new file mode 100644
> index 0000000..86e8bd6
> --- /dev/null
> +++ b/board-qemu/slof/vio-vtpm-cdriver.fs
> @@ -0,0 +1,71 @@
> +\ *****************************************************************************
> +\ * Copyright (c) 2015 IBM Corporation
> +\ * All rights reserved.
> +\ * This program and the accompanying materials
> +\ * are made available under the terms of the BSD License
> +\ * which accompanies this distribution, and is available at
> +\ * http://www.opensource.org/licenses/bsd-license.php
> +\ *
> +\ * Contributors:
> +\ * IBM Corporation - initial implementation
> +\ ****************************************************************************/
> +
> +." Populating " pwd
> +
> +false VALUE vtpm-debug?
You already defined a global "vtpm-debug?" variable in vtpm-static.fs
... are you sure that you want yet another one here with the same name
that shadows the global variable? Sounds cumbersome to me.
> +0 VALUE vtpm-unit
> +
> +: setup-alias
> + " ibm,vtpm" find-alias 0= IF
> + " ibm,vtpm" get-node node>path set-alias
> + ELSE
> + drop
> + THEN
> +;
> +
> +: vtpm-cleanup ( )
> + vtpm-debug? IF ." VTPM: Disabling RTAS bypass" cr THEN
> + vtpm-unit 0 rtas-set-tce-bypass
> +;
> +
> +: vtpm-init ( -- true | false )
> + 0 0 get-node open-node ?dup 0= IF EXIT THEN
> + my-self >r
> + dup to my-self
> +
> + vtpm-debug? IF ." VTPM: Initializing for c-driver" cr THEN
> +
> + my-unit to vtpm-unit
> +
> + \ Enable TCE bypass special qemu feature
> + vtpm-unit 1 rtas-set-tce-bypass
> +
> + \ Have TCE bypass cleaned up
> + ['] vtpm-cleanup add-quiesce-xt
> +
> + tpm-start dup 0= IF
> + vtpm-debug? IF ." VTPM: Success from tpm-start" cr THEN
> + drop
> + setup-alias
> + ELSE
> + IF ." VTPM: Error code from tpm-start: " . cr THEN
> + THEN
> +
> + close-node
> + r> to my-self
> +;
> +
> +: open ( )
> + vtpm-debug? IF ." VTPM: vTPM open()" cr THEN
> + true
> +;
> +
> +: close ( )
> + vtpm-debug? IF ." VTPM: vTPM close()" cr THEN
> +;
> +
> +\ setup alias and the RTAS bypass
> +vtpm-init
> +
> +\ setup the log
> +include vtpm-sml.fs
Is there always only max. one TPM chip in the system or could there also
be multiple TPM chips?
vio-vtpm-cdriver.fs could theoretically be included multiple times (in
case there are multiple TPM chips in the system), while vtpm-sml.fs
looks like it should only be included once (since it creates a new node
under the root node).
So maybe it would be better to include vtpm-sml.fs from tree.fs instead,
when the root node is touched anyway (after checking "vtpm-available?"
of course).
> diff --git a/board-qemu/slof/vtpm-sml.fs b/board-qemu/slof/vtpm-sml.fs
> new file mode 100644
> index 0000000..40f1b7e
> --- /dev/null
> +++ b/board-qemu/slof/vtpm-sml.fs
> @@ -0,0 +1,49 @@
> +\ *****************************************************************************
> +\ * Copyright (c) 2015 IBM Corporation
> +\ * All rights reserved.
> +\ * This program and the accompanying materials
> +\ * are made available under the terms of the BSD License
> +\ * which accompanies this distribution, and is available at
> +\ * http://www.opensource.org/licenses/bsd-license.php
> +\ *
> +\ * Contributors:
> +\ * IBM Corporation - initial implementation
> +\ ****************************************************************************/
> +
> +\ KVM/qemu TPM SML entries in /ibm,vtpm
What does SML mean? ... being a little bit more verbose the first time
you use TLAs (Three Letter Acronyms) would be nice.
> +" /" find-device
> +
> +new-device
> +
> +false VALUE vtpm-debug?
Again, yet another vtpm-debug? variable that shadows the global one?
> +0 VALUE log-base
> +40000 CONSTANT LOG-SIZE \ 256k per VTPM FW spec.
> +
> +LOG-SIZE alloc-mem to log-base
If you never free the buffer, please use the "BUFFER:" keyword instead.
The pool of memory for alloc-mem is quite limited, so it should be used
for memory only that is allocated and freed dynamically.
> +\ create /ibm,vtpm
> +s" ibm,vtpm" 2dup device-name device-type
> +
> +: sml-get-allocated-size ( -- buffer-size)
> + vtpm-debug? IF
> + ." Call to sml-get-allocated-size; size = " LOG-SIZE . cr
> + THEN
> + LOG-SIZE
> +;
> +
> +: sml-handover ( dest size -- )
> + vtpm-debug? IF
> + 2dup
> + ." Call to sml-handover; size = " . ." dest = " . cr
> + THEN
> + log-base ( dest size src )
> + -rot ( src dest size )
> + move
> +;
> +
> +: open true ;
> +: close ;
> +
> +finish-device
> +device-end
[...]
> diff --git a/lib/libtpm/tcgbios.c b/lib/libtpm/tcgbios.c
> new file mode 100644
> index 0000000..0dae810
> --- /dev/null
> +++ b/lib/libtpm/tcgbios.c
> @@ -0,0 +1,406 @@
> +
> +/*****************************************************************************
> + * Copyright (c) 2015 IBM Corporation
> + * All rights reserved.
> + * This program and the accompanying materials
> + * are made available under the terms of the BSD License
> + * which accompanies this distribution, and is available at
> + * http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + * IBM Corporation - initial implementation
> + *****************************************************************************/
> +
> +/*
> + * Implementation of the TPM BIOS extension according to the specification
> + * described in the IBM VTPM Firmware document and the TCG Specification
> + * that can be found here under the following link:
> + * http://www.trustedcomputinggroup.org/resources/pc_client_work_group_specific_implementation_specification_for_conventional_bios
> + */
> +
> +#include "types.h"
> +#include "byteorder.h"
> +#include "tpm_drivers.h"
> +#include "string.h"
> +#include "tcgbios.h"
> +#include "tcgbios_int.h"
> +#include "stdio.h"
> +
> +#undef TCGBIOS_DEBUG
> +//#define TCGBIOS_DEBUG
> +#ifdef TCGBIOS_DEBUG
> +#define dprintf(_x ...) do { printf("TCGBIOS: " _x); } while(0)
> +#else
> +#define dprintf(_x ...)
> +#endif
> +
> +
> +static const uint8_t startup_st_clear[] = { 0x00, TPM_ST_CLEAR };
> +static const uint8_t startup_st_state[] = { 0x00, TPM_ST_STATE };
> +
> +static const uint8_t physical_presence_cmd_enable[] = { 0x00, 0x20 };
> +static const uint8_t physical_presence_cmd_disable[] = { 0x01, 0x00 };
> +static const uint8_t physical_presence_present[] = { 0x00, 0x08 };
> +static const uint8_t physical_presence_not_present_lock[] = { 0x00, 0x14 };
> +
> +static const uint8_t command_flag_false[] = { 0x00 };
> +static const uint8_t command_flag_true[] = { 0x01 };
> +
> +static const uint8_t get_capability_permanent_flags[] = {
> + 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x04,
> + 0x00, 0x00, 0x01, 0x08
> +};
> +
> +static const uint8_t get_capability_owner_auth[] = {
> + 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x04,
> + 0x00, 0x00, 0x01, 0x11
> +};
> +
> +static const uint8_t get_capability_timeouts[] = {
> + 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x04,
> + 0x00, 0x00, 0x01, 0x15
> +};
> +
> +static const uint8_t get_capability_durations[] = {
> + 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x04,
> + 0x00, 0x00, 0x01, 0x20
> +};
> +
> +struct tpm_state {
> + uint8_t tpm_probed:1;
> + uint8_t tpm_found:1;
> + uint8_t tpm_working:1;
> + uint8_t if_shutdown:1;
I have to say that I'm not a fan of using a "uintXX_t" type for
bitfields unless it's really necessary. Here it seems not to be
necessary since it is not a packed struct and the compiler likely will
align the following field anyway. So could you simply use "unsigned"
here instead of "uint8_t" ?
> + struct tpm_driver *tpm_drv;
> +};
> +
> +static struct tpm_state tpm_state = {
> + .tpm_drv = NULL,
> +};
> +
> +extern struct tpm_driver tpm_drivers[];
> +
> +/* prototypes */
> +static uint32_t build_and_send_cmd(uint32_t ordinal, const uint8_t *append,
> + uint32_t append_size, uint8_t *resbuffer,
> + uint32_t return_size, uint32_t *return_code,
> + enum tpm_duration_type to_t);
> +
> +/********************************************************
> + Extensions for TCG-enabled BIOS
> + *******************************************************/
> +
> +static bool is_tpm_present(void)
> +{
> + bool rc = false;
> + unsigned int i;
> +
> + for (i = 0; i < TPM_NUM_DRIVERS; i++) {
> + struct tpm_driver *td = &tpm_drivers[i];
> + if (td->probe()) {
> + td->init();
> + tpm_state.tpm_drv = td;
> + rc = true;
> + break;
> + }
> + }
> +
> + return rc;
> +}
Uh, unless you really really want to support multiple TPM drivers in the
near future, I really would prefer if you could keep this simple instead
and get rid of that indirect "struct tpm_driver *" function pointer
magic. That really looks over-engineered to me right now.
> +static void probe_tpm(void)
> +{
> + tpm_state.tpm_probed = 1;
> + tpm_state.tpm_found = is_tpm_present();
> + tpm_state.tpm_working = tpm_state.tpm_found;
> +}
> +
> +static bool has_working_tpm(void)
> +{
> + if (!tpm_state.tpm_probed)
> + probe_tpm();
> +
> + return tpm_state.tpm_working;
> +}
> +
> +static void tpm_set_failure(void)
> +{
> + uint32_t return_code;
> +
> + /* we will try to deactivate the TPM now - ignoring all errors */
> + build_and_send_cmd(TPM_ORD_PHYSICAL_PRESENCE,
> + physical_presence_cmd_enable,
> + sizeof(physical_presence_cmd_enable),
> + NULL, 0, &return_code,
> + TPM_DURATION_TYPE_SHORT);
> +
> + build_and_send_cmd(TPM_ORD_PHYSICAL_PRESENCE,
> + physical_presence_present,
> + sizeof(physical_presence_present),
> + NULL, 0, &return_code,
> + TPM_DURATION_TYPE_SHORT);
> +
> + build_and_send_cmd(TPM_ORD_SET_TEMP_DEACTIVATED,
> + NULL, 0, NULL, 0, &return_code,
> + TPM_DURATION_TYPE_SHORT);
> +
> + tpm_state.tpm_working = 0;
> +}
> +
> +static uint32_t transmit(uint8_t locty, const struct iovec iovec[],
> + uint8_t *respbuffer, uint32_t *respbufferlen,
> + enum tpm_duration_type to_t)
> +{
> + struct tpm_driver *td = tpm_state.tpm_drv;
> + unsigned int i;
> +
> + if (td == NULL)
> + goto err_exit;
So if td == NULL, you goto err_exit - which will then do
tpm_set_failure() ... which then calls build_and_send_cmd() ... which
will finally call transmit() again, which will fail again since td ==
NULL ... and you end up in a nice recursive endless loop. That error
handling seems to be broken.
> + if (!td->activate(locty))
> + goto err_exit;
> +
> + for (i = 0; iovec[i].length; i++) {
> + if (!td->senddata(iovec[i].data,
> + iovec[i].length))
> + goto err_exit;
> + }
> +
> + if (!td->transfer() ||
> + !td->waitresponseready(to_t) ||
> + !td->readresponse(respbuffer, respbufferlen))
> + goto err_exit;
> +
> + td->ready();
> +
> + return 0;
> +
> +err_exit:
> + tpm_set_failure();
I think you should rather only set tpm_state.tpm_working = 0 here,
without trying to send another command which will trigger a new
transmit() function.
> + return TCGBIOS_FATAL_COM_ERROR;
> +}
[...]
> +uint32_t tpm_start(void)
> +{
> + tpm_state.if_shutdown = 0;
> + tpm_state.tpm_probed = 0;
> + tpm_state.tpm_found = 0;
> + tpm_state.tpm_working = 0;
> +
> + if (!has_working_tpm()) {
> + dprintf("%s: Machine does not have a working TPM\n",
> + __func__);
> + tpm_state.if_shutdown = 1;
> + return TCGBIOS_FATAL_COM_ERROR;
> + }
> +
> + return tpm_startup();
> +}
I'm having a hard time to imagine what "unassert physical presence"
means ... could you please add a comment here to explain what this
function does?
> +uint32_t tpm_unassert_physical_presence(void)
> +{
> + uint32_t rc;
> + uint32_t return_code;
> +
> + if (!has_working_tpm())
> + return TCGBIOS_GENERAL_ERROR;
> +
> + rc = build_and_send_cmd(TPM_ORD_PHYSICAL_PRESENCE,
> + physical_presence_cmd_enable,
> + sizeof(physical_presence_cmd_enable),
> + NULL, 0, &return_code,
> + TPM_DURATION_TYPE_SHORT);
> +
> + dprintf("Return code from TPM_PhysicalPresence(CMD_ENABLE) = 0x%08x\n",
> + return_code);
> +
> + if (rc || return_code)
> + goto err_exit;
> +
> + rc = build_and_send_cmd(TPM_ORD_PHYSICAL_PRESENCE,
> + physical_presence_not_present_lock,
> + sizeof(physical_presence_not_present_lock),
> + NULL, 0, &return_code,
> + TPM_DURATION_TYPE_SHORT);
> +
> + dprintf("Return code from TPM_PhysicalPresence(NOT_PRESENT_LOCK) = 0x%08x\n",
> + return_code);
> +
> + if (rc || return_code)
> + goto err_exit;
> +
> + return 0;
> +
> +err_exit:
> + dprintf("TPM malfunctioning (line %d).\n", __LINE__);
> +
> + tpm_set_failure();
> + if (rc)
> + return rc;
> + return TCGBIOS_COMMAND_ERROR;
> +}
> diff --git a/lib/libtpm/tcgbios.h b/lib/libtpm/tcgbios.h
> new file mode 100644
> index 0000000..594a089
> --- /dev/null
> +++ b/lib/libtpm/tcgbios.h
> @@ -0,0 +1,21 @@
> +/*****************************************************************************
> + * Copyright (c) 2015 IBM Corporation
> + * All rights reserved.
> + * This program and the accompanying materials
> + * are made available under the terms of the BSD License
> + * which accompanies this distribution, and is available at
> + * http://www.opensource.org/licenses/bsd-license.php
> + *
> + * Contributors:
> + * IBM Corporation - initial implementation
> + *****************************************************************************/
> +
> +#ifndef TCGBIOS_H
> +#define TCGBIOS_H
> +
> +#include <stdint.h>
> +
> +uint32_t tpm_start(void);
> +uint32_t tpm_unassert_physical_presence(void);
> +
> +#endif /* TCGBIOS_H */
> diff --git a/lib/libtpm/tcgbios_int.h b/lib/libtpm/tcgbios_int.h
> new file mode 100644
> index 0000000..a6a0d8f
> --- /dev/null
> +++ b/lib/libtpm/tcgbios_int.h
What does "_int" mean in the file name here?
[...]
> +
> +/* event types */
> +#define EV_POST_CODE 1
> +#define EV_SEPARATOR 4
> +#define EV_ACTION 5
> +#define EV_EVENT_TAG 6
> +#define EV_COMPACT_HASH 12
> +#define EV_IPL 13
> +#define EV_IPL_PARTITION_DATA 14
> +
> +#define STATUS_FLAG_SHUTDOWN (1 << 0)
> +
> +#define SHA1_BUFSIZE 20
> +
> +struct iovec {
> + size_t length;
> + const void *data;
> +};
That "struct iovec" should rather be put into the right header file of
SLOF's libc instead - otherwise we'll end up again with multiple
definitions of this struct type as soon as some other code needs it, too.
Thomas
More information about the SLOF
mailing list