[SLOF] [PATCH v2 02/20] Add TPM initialization support
Stefan Berger
stefanb at linux.vnet.ibm.com
Thu Nov 19 23:15:44 AEDT 2015
On 11/19/2015 04:04 AM, Thomas Huth wrote:
> 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>
> [...]
+
+: 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?
Per firmware spec only one device is supported. QEMU also only allows
one device.
>
> 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.
Stored Measurement Log.
>
>> +" /" find-device
>> +
>> +new-device
>> +
>> +false VALUE vtpm-debug?
> Again, yet another vtpm-debug? variable that shadows the global one?
What is preferable ? One global one or local ones?
>
>
>> +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.
Will do.
>
>> +\ 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" ?
Fine by me.
>
>> + 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.
... is this really necessary?
>
>> +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.
Thanks for pointing out.
>
>> + 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?
internal
>
> [...]
>> +
>> +/* 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.
This one has a const void * for data, which may not be what others want.
So under what name should that go?
Stefan
More information about the SLOF
mailing list