[Skiboot] [PATCH v2 06/12] secvar_tpmnv: add high-level tpm nv index abstraction for secvar
Stewart Smith
stewart at flamingspork.com
Mon Jan 27 06:04:19 AEDT 2020
On Sun, Jan 19, 2020, at 6: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.
ack. I like it. How's the code coverage with the simulation?
> 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
Are these special apart from invented here? My guess is they should make their way to the skiboot docs if they're on-"disk" formats.
> +
> +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
Skiboot uses C style comments
> +int tpm_fake_nv = 0; // Use fake NV mode using pnor
Should this be a #define instead? Maybe linked to DEBUG builds?
> +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;
> + 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);
> +}
sanity checking? It could be good to see unit tests where a lot of the odd/negative paths are taken.
> +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));
> +
> + // TODO: Determine the proper auths
> + 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
this seems like a pretty big TODO? How does one provision the TPM?
> + // Using the minimum defined by the spec for now
> + // This value should probably be determined by tss_get_capatibility
Which spec?
C style comments.
> + tpm_nv_size = 1024;
Should probably be documented somewhere in the skiboot docs of the minimum size being this (is there a maximum?)
> +
> + tpm_image = malloc(tpm_nv_size);
> + if (!tpm_image) {
> + tpm_error = 1;
> + return OPAL_NO_MEM;
> + }
> +
> + if (tpm_fake_nv) {
It's not immediately obvious that this is a in-sim-only or debug-only thing. Could we locate the enabling of this somewhere closer to ensuring it's not accidentally running on a production/real machine?
> + 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;
> + }
> +
> + if (tpm_image->magic_num != TPM_SECVAR_MAGIC_NUM) {
> + prlog(PR_INFO, "Magic num mismatch, reformatting NV space...\n");
These semantics should likely be clearly documented in doc/
> + 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)
> +{
> + 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;
> + }
> +
> + return NULL;
> +}
> +
> +
> +// "Allocate" space within the secvar tpm
the quotation marks make me think there needs to be decent docs about it.
C style comments (I also think this comment is redundant and doesn't need to exist at all)
> +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...
comment is redundant
> + 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;
> +
> + 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;
> +
> + var = find_tpmnv_id(id);
> + if (!var)
> + return OPAL_EMPTY;
> +
> + size = MIN(size, var->size);
> + memcpy(buf, var->data + off, size);
how does the calller know how much data is in buf?
> +
> + 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;
> +
> + var = find_tpmnv_id(id);
> + if (!var)
> + return OPAL_EMPTY;
> +
> + size = MIN(size, var->size);
> + memcpy(var->data, buf + off, size);
> +
> + 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;
> +
> + var = find_tpmnv_id(id);
> + if (!var)
> + return 0;
> + return var->size;
> +}
> 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>
> +
> +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
> +
> --
> 2.21.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
More information about the Skiboot
mailing list