[v2,06/12] secvar_tpmnv: add high-level tpm nv index abstraction for secvar

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@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

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@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...
  

> +
> +	// 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 ?


> +	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'?


> +	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?

> +	}
> +
> +	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.


> +
> +	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
> +

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@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!

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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>

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@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>

On 1/19/20 11: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@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
> +

Each NV index in Nuvoton TPM 2.0 can't be larger than 1K bytes. IIRC, you
will need more than one NV index. Shouldn't you define all of them here and
init them below?


> +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;
> +

Down below you allocate multiple of these tpm_nv_id structures. Layout
description would be helpful.


> +int tpm_ready = 0;

tpm_ready seems critical. This may deserve a description.

Claudio


> +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;
> +	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));
> +
> +	// 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
> +
> +	// 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;
> +	}
> +
> +	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)
> +{
> +	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
> +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;
> +
> +	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);
> +
> +	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
> +

On 1/19/20 11: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@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;
> +	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;

If you put the fake and the real implementation of this interface you can
use the Makefile to compile in only the one you want.

Claudio

> +
> +// 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
> +
> +	// 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;
> +	}
> +
> +	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)
> +{
> +	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
> +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;
> +
> +	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);
> +
> +	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
> +

On 1/27/20 11:25 AM, Claudio Carvalho wrote:
> On 1/19/20 11: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@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
>> +
> Each NV index in Nuvoton TPM 2.0 can't be larger than 1K bytes.

Errata.

"./getcapability -cap 6" returns that TPM_PT_NV_INDEX_MAX=2048 for a
Nuvoton TPM 2.0.

Claudio


>  IIRC, you
> will need more than one NV index. Shouldn't you define all of them here and
> init them below?
>
>
>> +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;
>> +
> Down below you allocate multiple of these tpm_nv_id structures. Layout
> description would be helpful.
>
>
>> +int tpm_ready = 0;
> tpm_ready seems critical. This may deserve a description.
>
> Claudio
>
>
>> +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;
>> +	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));
>> +
>> +	// 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
>> +
>> +	// 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;
>> +	}
>> +
>> +	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)
>> +{
>> +	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
>> +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;
>> +
>> +	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);
>> +
>> +	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
>> +
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

On 1/27/20 11:40 AM, Claudio Carvalho wrote:
> On 1/19/20 11: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@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;
>> +	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;
> If you put the fake and the real implementation of this interface you can
> use the Makefile to compile in only the one you want.

If you put the fake and real implementation of this interface in separated
files you can use the Make file to compile in only the one you want.

Claudio.


>
> Claudio
>
>> +
>> +// 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
>> +
>> +	// 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;
>> +	}
>> +
>> +	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)
>> +{
>> +	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
>> +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;
>> +
>> +	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);
>> +
>> +	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
>> +
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

On 1/24/20 8:43 PM, Eric Richter wrote:
> On 1/23/20 8:39 AM, Stefan Berger wrote:
>> On 1/19/20 9:36 PM, Eric Richter wrote:
>>
>>> +
>>> +    // 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.


To support this kind of scenario you'll probably need a driver 
abstraction / interface for pnor+tpm so that one can accommodate some 
other storage driver underneath. Probably some sort of probing would be 
needed so that one executable can work for both environments then.