[Skiboot] [PATCH v2 06/12] secvar_tpmnv: add high-level tpm nv index abstraction for secvar
Eric Richter
erichte at linux.ibm.com
Sat Jan 25 12:43:14 AEDT 2020
On 1/23/20 8:39 AM, Stefan Berger wrote:
> On 1/19/20 9:36 PM, Eric Richter wrote:
>> Multiple components, like the storage driver or backend driver, may need
>> to store information in the one TPM NV index assigned for secure boot.
>> This abstraction provides a method for these components to share the
>> index space without stomping on each other's data, and without them
>> needing to understand anything about the other.
>>
>> This is probably an overengineered solution to the problem, but the
>> intent is to keep the drivers as independent from one another as possible.
>>
>> NOTE: This version of the patch now includes the ability to switch between
>> a "Fake TPMNV" mode simulated using PNOR, and use of an actual TPM. The
>> simulated mode exists for unit test and review purposes on machines without
>> a TPM. It is not intended for production use.
>>
>> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
>> ---
>> libstb/secvar/Makefile.inc | 3 +-
>> libstb/secvar/secvar_tpmnv.c | 265 +++++++++++++++++++++++++++++++++++
>> libstb/secvar/secvar_tpmnv.h | 16 +++
>> 3 files changed, 282 insertions(+), 2 deletions(-)
>> create mode 100644 libstb/secvar/secvar_tpmnv.c
>> create mode 100644 libstb/secvar/secvar_tpmnv.h
>>
>> diff --git a/libstb/secvar/Makefile.inc b/libstb/secvar/Makefile.inc
>> index f4b196d9..1d68941e 100644
>> --- a/libstb/secvar/Makefile.inc
>> +++ b/libstb/secvar/Makefile.inc
>> @@ -8,8 +8,7 @@ SUBDIRS += $(SECVAR_DIR)
>> include $(SECVAR_DIR)/storage/Makefile.inc
>> include $(SECVAR_DIR)/backend/Makefile.inc
>> -SECVAR_SRCS = secvar_main.c secvar_util.c secvar_devtree.c
>> -SECVAR_SRCS = secvar_main.c secvar_util.c secvar_devtree.c secvar_api.c
>> +SECVAR_SRCS = secvar_main.c secvar_util.c secvar_devtree.c secvar_api.c secvar_tpmnv.c
>> SECVAR_OBJS = $(SECVAR_SRCS:%.c=%.o)
>> SECVAR = $(SECVAR_DIR)/built-in.a
>> diff --git a/libstb/secvar/secvar_tpmnv.c b/libstb/secvar/secvar_tpmnv.c
>> new file mode 100644
>> index 00000000..2944ece4
>> --- /dev/null
>> +++ b/libstb/secvar/secvar_tpmnv.c
>> @@ -0,0 +1,265 @@
>> +// SPDX-License-Identifier: Apache-2.0
>> +/* Copyright 2019 IBM Corp. */
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "SECVAR_TPMNV: " fmt
>> +#endif
>> +
>> +#include <opal.h>
>> +#include <skiboot.h>
>> +#include <string.h>
>> +#include <tssskiboot.h>
>> +#include "secvar_tpmnv.h"
>> +
>> +#define TPM_SECVAR_NV_INDEX 0x01c10191
>> +#define TPM_SECVAR_MAGIC_NUM 0x53544e56
>> +
>> +struct tpm_nv_id {
>> + uint32_t id;
>> + uint32_t size;
>> + char data[0];
>> +} __packed;
>> +
>> +struct tpm_nv {
>> + uint32_t magic_num;
>> + uint32_t version;
>> + struct tpm_nv_id vars[0];
>> +} __packed;
>> +
>> +int tpm_ready = 0;
>> +int tpm_error = 0;
>> +int tpm_first_init = 0;
>> +struct tpm_nv *tpm_image;
>> +size_t tpm_nv_size = 0;
>> +
>> +// Values set by a platform to enable TPMNV simulation mode
>> +// NOT INTENDED FOR PRODUCTION USE
>> +int tpm_fake_nv = 0; // Use fake NV mode using pnor
>> +uint64_t tpm_fake_nv_offset = 0; // Offset into SECBOOT pnor to use
>> +uint64_t tpm_fake_nv_max_size = 0;
>> +
>> +static int TSS_Fake_Read(uint32_t nvIndex, void *buf, size_t bufsize, uint64_t off)
>> +{
>> + (void) nvIndex;
>> + (void) off;
>
> Same comment as for other patch: use __attribute__((unused))
>
>
>> + return platform.secboot_read(buf, tpm_fake_nv_offset, bufsize);
>> +}
>> +
>> +static int TSS_Fake_Write(uint32_t nvIndex, void *buf, size_t bufsize, uint64_t off)
>> +{
>> + (void) nvIndex;
>> + (void) off;
>> + return platform.secboot_write(tpm_fake_nv_offset, buf, bufsize);
>> +}
>> +
>> +static int TSS_Fake_Define_Space(uint32_t nvIndex, const char hierarchy,
>> + const char hierarchy_authorization,
>> + uint16_t dataSize)
>> +{
>> + (void) nvIndex;
>> + (void) hierarchy;
>> + (void) hierarchy_authorization;
>> + (void) dataSize;
>> + return 0;
>> +}
>> +
>> +struct tpmnv_ops_s {
>> + int (*tss_nv_read)(uint32_t, void*, size_t, uint64_t);
>> + int (*tss_nv_write)(uint32_t, void*, size_t, uint64_t);
>> + int (*tss_nv_define_space)(uint32_t, const char, const char, uint16_t);
>> +};
>> +
>> +struct tpmnv_ops_s TSS_tpmnv_ops = {
>> + .tss_nv_read = TSS_NV_Read,
>> + .tss_nv_write = TSS_NV_Write,
>> + .tss_nv_define_space = TSS_NV_Define_Space,
>> +};
>> +
>> +struct tpmnv_ops_s Fake_tpmnv_ops = {
>> + .tss_nv_read = TSS_Fake_Read,
>> + .tss_nv_write = TSS_Fake_Write,
>> + .tss_nv_define_space = TSS_Fake_Define_Space,
>> +};
>> +
>> +struct tpmnv_ops_s *tpmnv_ops = &TSS_tpmnv_ops;
>> +
>> +// This function should be replaced with logic that performs the initial
>> +// TPM NV Index definition, and any first-write logic
>> +static int secvar_tpmnv_format(void)
>> +{
>> + int rc;
>> +
>> + memset(tpm_image, 0, sizeof(tpm_nv_size));
>
> +size_t tpm_nv_size = 0;
>
> sizeof() doesn't seem right...
>
It is indeed very wrong, thanks for the catch!
>
>> +
>> + // TODO: Determine the proper auth
>
> right. I doubt it will always work with physical presence protection. What all environments do you intend to run this in? Will SLOF be there before you run this ?
>
This whole file is largely only intended for secure boot of the host OS, specifically on p9 witherspoon machines. Guest secure boot should (hopefully) not be requiring the TPM NV space as part of key storage.
>
>> + rc = tpmnv_ops->tss_nv_define_space(TPM_SECVAR_NV_INDEX, 'p', 'p', tpm_nv_size);
>> + if (rc) {
>> + prlog(PR_INFO, "Failed to define NV index, rc = %d\n", rc);
>> + return rc;
>> + }
>> +
>> + tpm_image->magic_num = TPM_SECVAR_MAGIC_NUM;
>> + tpm_image->version = 1;
>> +
>> + tpm_first_init = 1;
>> +
>> + return tpmnv_ops->tss_nv_write(TPM_SECVAR_NV_INDEX, tpm_image, tpm_nv_size, 0);
>> +}
>> +
>> +
>> +static int secvar_tpmnv_init(void)
>> +{
>> + int rc;
>> +
>> + if (tpm_ready)
>> + return OPAL_SUCCESS;
>> + if (tpm_error)
>> + return OPAL_HARDWARE;
>> +
>> + prlog(PR_INFO, "Initializing TPMNV space...\n");
>> +
>> + // Check here if TPM NV Index is defined
>> + // if not, call secvar_tpmnv_format() here
>> +
>> + // Using the minimum defined by the spec for now
>> + // This value should probably be determined by tss_get_capatibility
>> + tpm_nv_size = 1024;
>> +
>> + tpm_image = malloc(tpm_nv_size);
>> + if (!tpm_image) {
>> + tpm_error = 1;
>> + return OPAL_NO_MEM;
>> + }
>> +
>> + if (tpm_fake_nv) {
>> + prlog(PR_INFO, "Enabling fake TPM NV mode\n");
>> + tpmnv_ops = &Fake_tpmnv_ops;
>> + }
>> +
>> + prlog(PR_INFO, "Reading in from TPM NV...\n");
>> + rc = tpmnv_ops->tss_nv_read(TPM_SECVAR_NV_INDEX, tpm_image, tpm_nv_size, 0);
>> + if (rc) {
>> + prlog(PR_INFO, "Failed to read from NV index, rc = %d\n", rc);
>> + tpm_error = 1;
>> + return OPAL_HARDWARE;
>> + }
>> +
>
> Hm, will it ever get here in case TPM_SECVAR_NV_INDEX doesn't exist yet and you need to 'format'?
>
The actual code detecting if the index was defined was omitted. The read call should check if the index is defined, and call format (which tries to define it) if not. Next version will contain this logic.
>
>> + if (tpm_image->magic_num != TPM_SECVAR_MAGIC_NUM) {
>> + prlog(PR_INFO, "Magic num mismatch, reformatting NV space...\n");
>> + rc = secvar_tpmnv_format();
>> + if (rc) {
>> + prlog(PR_INFO, "Failed to format tpmnv space, rc = %d\n", rc);
>> + tpm_error = 1;
>> + return OPAL_HARDWARE;
>> + }
>> + }
>> + prlog(PR_INFO, "TPMNV space initialized successfully\n");
>> + tpm_ready = 1;
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +
>> +static struct tpm_nv_id *find_tpmnv_id(uint32_t id)
> A bit description for this function would help what this id is that you are searching for.
>> +{
>> + struct tpm_nv_id *tmp;
>> + char *cur, *end;
>> +
>> + cur = (char *) tpm_image->vars;
>> + end = ((char *) tpm_image) + tpm_nv_size;
>> + while (cur < end) {
>> + tmp = (struct tpm_nv_id *) cur;
>> + if (tmp->id == 0)
>> + return NULL;
>> + if (tmp->id == id)
>> + return tmp;
>> + cur += sizeof(struct tpm_nv_id) + tmp->size;
>
> whitespace error.
>
> Do you know that cur will be fitting into the rest of the space?
Hmm... yes, a lot of this was written under the assumption that the driver code is never wrong. I've updated it now with more bounds checks, so even if bad data is written, they should still be handled gracefully.
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +
>> +// "Allocate" space within the secvar tpm
>> +int secvar_tpmnv_alloc(uint32_t id, int32_t size)
>> +{
>> + struct tpm_nv_id *tmp;
>> + char *cur;
>> + char *end;
>> +
>> + if (secvar_tpmnv_init())
>> + return OPAL_RESOURCE;
>> +
>> + cur = (char *) tpm_image->vars;
>> + end = ((char *) tpm_image) + tpm_nv_size;
>> + while (cur < end) {
>> + tmp = (struct tpm_nv_id *) cur;
>> + if (tmp->id == 0)
>> + goto allocate;
>> + if (tmp->id == id)
>> + return OPAL_SUCCESS; // Already allocated
>> +
>> + cur += sizeof(struct tpm_nv_id) + tmp->size;
>> + }
>> + // We ran out of space...
>> + return OPAL_EMPTY;
>> +
>> +allocate:
>> + tmp->id = id;
>> +
>> + // Special case: size of -1 gives remaining space
>> + if (size == -1)
>> + tmp->size = end - tmp->data;
>> + else
>> + tmp->size = size;
> Are you sure you have size bytes left?
>> +
>> + return OPAL_SUCCESS;
>> +}
>> +
>> +
>> +int secvar_tpmnv_read(uint32_t id, void *buf, size_t size, size_t off)
>> +{
>> + struct tpm_nv_id *var;
>> +
>> + if (secvar_tpmnv_init())
>> + return OPAL_RESOURCE;
>
> There should be an 'earlier' location where you can call the init function.
>
This is intended to be "lazily evaluated", so the first use of the TPM NV is what prompts the initialization. Not all platforms may use TPM NV, so if these functions are never called, we never have to initialize it.
Although, there shouldn't be a _read() call before an _alloc() call, so we shouldn't be expecting to _init() here, only that it was _init()'d.
>
>> +
>> + var = find_tpmnv_id(id);
>> + if (!var)
>> + return OPAL_EMPTY;
>> +
>> + size = MIN(size, var->size);
> You should check whether off > var->size and then also do MIN(size, var->size - off).
>> + memcpy(buf, var->data + off, size);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +int secvar_tpmnv_write(uint32_t id, void *buf, size_t size, size_t off)
>> +{
>> + struct tpm_nv_id *var;
>> +
>> + if (secvar_tpmnv_init())
>> + return OPAL_RESOURCE;
> Same comment as above.
>> +
>> + var = find_tpmnv_id(id);
>> + if (!var)
>> + return OPAL_EMPTY;
>> +
>> + size = MIN(size, var->size);
> similar comment as above but off > size check and MIN(size - off, var->size).
>> + memcpy(var->data, buf + off, size);
> size - off ?
>> +
>> + return tpmnv_ops->tss_nv_write(TPM_SECVAR_NV_INDEX, tpm_image, tpm_nv_size, 0);
>> +}
>> +
>> +int secvar_tpmnv_size(uint32_t id)
>> +{
>> + struct tpm_nv_id *var;
>> +
>> + if (secvar_tpmnv_init())
>> + return OPAL_RESOURCE;
>> +
> same comment
>> + var = find_tpmnv_id(id);
>> + if (!var)
>> + return 0;
>> + return var->size;
>
> +struct tpm_nv_id {
> + uint32_t id;
> + uint32_t size;
> + char data[0];
> +} __packed;
>
>
> function should return uint32_t ?
>
>
>> +}
>> diff --git a/libstb/secvar/secvar_tpmnv.h b/libstb/secvar/secvar_tpmnv.h
>> new file mode 100644
>> index 00000000..697a52c2
>> --- /dev/null
>> +++ b/libstb/secvar/secvar_tpmnv.h
>> @@ -0,0 +1,16 @@
>> +#ifndef _SECVAR_TPMNV_H_
>> +#define _SECVAR_TPMNV_H_
>> +#include <stdint.h>
> License header needed ?
>> +
>> +extern int tpm_first_init;
>> +
>> +int secvar_tpmnv_alloc(uint32_t id, int32_t size);
>> +int secvar_tpmnv_read(uint32_t id, void *buf, size_t size, size_t off);
>> +int secvar_tpmnv_write(uint32_t id, void *buf, size_t size, size_t off);
>> +int secvar_tpmnv_size(uint32_t id);
>> +
>> +extern int tpm_fake_nv;
>> +extern uint64_t tpm_fake_nv_offset;
>> +
>> +#endif
>> +
>
>
Thanks for the feedback!
More information about the Skiboot
mailing list