[Skiboot] [PATCH v2 06/12] secvar_tpmnv: add high-level tpm nv index abstraction for secvar
Eric Richter
erichte at linux.ibm.com
Mon Jan 27 08:08:20 AEDT 2020
On 1/26/20 1:04 PM, Stewart Smith wrote:
> 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?
>
The storage driver test exercises all the bits to be stored in the TPM, e.g. bank flipping, hash calculation/verification using the simulation. At the time of writing this response, this file is 72% covered by tests, the majority of the code not covered are edge case errors that still need tests, and the TPM Define Space logic (not pictured in this current version). Will be improved in the next posting.
Unfortunately this mode does mean we don't exercise the TSS wrappers, they still need their own unit tests.
>> 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.
>
The NV Index value is the one assigned for us to use for secure variable storage. The magic number is invented here.
Both should and will be documented on their origin.
>> +
>> +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
>
Eventually I will learn...
>> +int tpm_fake_nv = 0; // Use fake NV mode using pnor
>
> Should this be a #define instead? Maybe linked to DEBUG builds?
>
I considered that, but I wasn't sure if tying to DEBUG made sense. I can see this being useful for evaluation on platforms without a TPM, but not needing the full debug output.
Though, it probably should be a completely compile time option, rather than bloating a normal skiboot build with this unused (and potentially dangerous) feature.
>> +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?
>
Resolved in the next set, this version had this code omitted.
Short answer is, the NV space is defined here on first boot. It shouldn't be able to be undefined henceforth without the proper auth.
>> + // 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?)
>
The TPM doesn't have a large amount of NV space to work with, so we are using the least amount feasibly possible. The PK is about ~900 bytes, plus some space for the bank hashes.
I will update the comment (and docs) with the exact spec reference when I can find it, but I ran into issues allocating much higher than 1K on the platform I was testing on.
>> +
>> + 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?
>
Would making it a compile time option with #ifdefs help this?
Otherwise, where would you suggest this placement? Setting tpm_fake_nv=1 should be set in the platform's secvar_init() function, I could put the whole ops replacement there instead.
>> + 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
>>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
More information about the Skiboot
mailing list