diff mbox series

[v2,08/12] secvar/storage: add secvar storage driver for pnor-based p9 platforms

Message ID 20200120023700.5373-9-erichte@linux.ibm.com
State Superseded
Headers show
Series Add initial secure variable storage and backend drivers | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch fail Failed to apply to any branch
snowpatch_ozlabs/apply_patch warning Failed to apply on branch master (d75e82dbfbb9443efeb3f9a5921ac23605aab469)

Commit Message

Eric Richter Jan. 20, 2020, 2:36 a.m. UTC
This patch implements the platform specific logic for persisting the
secure variable storage banks across reboots via the SECBOOT PNOR
partition.

For POWER 9, all secure variables and updates are stored in the
in the SECBOOT PNOR partition. The partition is split into three
sections: two variable bank sections, and a section for storing
updates. The driver alternates writes between the two variable
sections, so that the final switch from one set of variables to
the next can be as atomic as possible by flipping an "active bit"
stored in TPM NV.

PNOR space provides no lock protection, so prior to writing the
variable bank, a sha256 hash is calculated and stored in TPM NV.
This hash is compared against the hash of the variables loaded from PNOR
to ensure consistency -- otherwise a failure is reported, no keys are
loaded (which should cause skiroot to refuse to boot if secure boot
support is enabled).

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 include/secvar.h                             |   1 +
 libstb/secvar/storage/Makefile.inc           |   4 +-
 libstb/secvar/storage/secboot_tpm.c          | 267 +++++++++++++++++++
 libstb/secvar/storage/secboot_tpm.h          |  26 ++
 libstb/secvar/test/Makefile.check            |   2 +-
 libstb/secvar/test/secvar-test-secboot-tpm.c | 142 ++++++++++
 6 files changed, 439 insertions(+), 3 deletions(-)
 create mode 100644 libstb/secvar/storage/secboot_tpm.c
 create mode 100644 libstb/secvar/storage/secboot_tpm.h
 create mode 100644 libstb/secvar/test/secvar-test-secboot-tpm.c

Comments

Stefan Berger Jan. 22, 2020, 9:51 p.m. UTC | #1
On 1/19/20 9:36 PM, Eric Richter wrote:
> This patch implements the platform specific logic for persisting the
> secure variable storage banks across reboots via the SECBOOT PNOR
> partition.
>
> For POWER 9, all secure variables and updates are stored in the
> in the SECBOOT PNOR partition. The partition is split into three
> sections: two variable bank sections, and a section for storing
> updates. The driver alternates writes between the two variable
> sections, so that the final switch from one set of variables to
> the next can be as atomic as possible by flipping an "active bit"
> stored in TPM NV.
>
> PNOR space provides no lock protection, so prior to writing the
> variable bank, a sha256 hash is calculated and stored in TPM NV.
> This hash is compared against the hash of the variables loaded from PNOR
> to ensure consistency -- otherwise a failure is reported, no keys are
> loaded (which should cause skiroot to refuse to boot if secure boot
> support is enabled).
>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>   include/secvar.h                             |   1 +
>   libstb/secvar/storage/Makefile.inc           |   4 +-
>   libstb/secvar/storage/secboot_tpm.c          | 267 +++++++++++++++++++
>   libstb/secvar/storage/secboot_tpm.h          |  26 ++
>   libstb/secvar/test/Makefile.check            |   2 +-
>   libstb/secvar/test/secvar-test-secboot-tpm.c | 142 ++++++++++
>   6 files changed, 439 insertions(+), 3 deletions(-)
>   create mode 100644 libstb/secvar/storage/secboot_tpm.c
>   create mode 100644 libstb/secvar/storage/secboot_tpm.h
>   create mode 100644 libstb/secvar/test/secvar-test-secboot-tpm.c
>
> diff --git a/include/secvar.h b/include/secvar.h
> index c41fb739..2875c700 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -23,6 +23,7 @@ struct secvar_backend_driver {
>           const char *compatible;			// String to use for compatible in secvar node
>   };
>   
> +extern struct secvar_storage_driver secboot_tpm_driver;
>   
>   int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
>   
> diff --git a/libstb/secvar/storage/Makefile.inc b/libstb/secvar/storage/Makefile.inc
> index b7a821ec..5926e2f5 100644
> --- a/libstb/secvar/storage/Makefile.inc
> +++ b/libstb/secvar/storage/Makefile.inc
> @@ -1,11 +1,11 @@
>   # SPDX-License-Identifier: Apache-2.0
>   # -*-Makefile-*-
>   
> -SECVAR_STORAGE_DIR = libstb/secvar/storage
> +SECVAR_STORAGE_DIR = $(SRC)/libstb/secvar/storage
>   
>   SUBDIRS += $(SECVAR_STORAGE_DIR)
>   
> -SECVAR_STORAGE_SRCS =
> +SECVAR_STORAGE_SRCS = secboot_tpm.c
>   SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
>   SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
>   
> diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
> new file mode 100644
> index 00000000..9a3d228a
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm.c
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: Apache-2.0
> +/* Copyright 2019 IBM Corp. */
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "SECBOOT_TPM: " fmt
> +#endif
> +
> +#include <stdlib.h>
> +#include <skiboot.h>
> +#include <opal.h>
> +#include <mbedtls/sha256.h>
> +#include "../secvar.h"
> +#include "../secvar_tpmnv.h"
> +#include "secboot_tpm.h"
> +
> +//#define CYCLE_BIT(b) (((((b-1)%SECBOOT_VARIABLE_BANK_NUM)+1)%SECBOOT_VARIABLE_BANK_NUM)+1)
> +#define CYCLE_BIT(b) (b^0x1)
> +
> +#define TPMNV_ID_ACTIVE_BIT	0x53414242 // SABB
> +#define TPMNV_ID_HASH_BANK_0	0x53484230 // SHB0
> +#define TPMNV_ID_HASH_BANK_1	0x53484231 // SHB1
> +
> +#define GET_HASH_BANK_ID(bit) ((bit)?TPMNV_ID_HASH_BANK_1:TPMNV_ID_HASH_BANK_0)
> +
> +// Because mbedtls doesn't define this?
> +#define SHA256_DIGEST_LENGTH	32
> +
> +struct secboot *secboot_image;
> +
> +static void calc_bank_hash(char *target_hash, char *source_buf, uint64_t size)
> +{
> +	mbedtls_sha256_context ctx;
> +
> +	mbedtls_sha256_init(&ctx);
> +	mbedtls_sha256_update_ret(&ctx, source_buf, size);
> +	mbedtls_sha256_finish_ret(&ctx, target_hash);
> +}
> +
> +static int secboot_format(void)
> +{
> +	char bank_hash[SHA256_DIGEST_LENGTH];
> +
> +	if (!platform.secboot_write)
> +		return OPAL_UNSUPPORTED;
> +
> +	memset(secboot_image, 0x00, sizeof(struct secboot));
> +
> +	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
> +	secboot_image->header.version = SECBOOT_VERSION;
> +
> +	// Write the empty hash to the tpm so loads work in the future
> +	calc_bank_hash(bank_hash, secboot_image->bank[0], SECBOOT_VARIABLE_BANK_SIZE);
> +	secvar_tpmnv_write(TPMNV_ID_HASH_BANK_0, bank_hash, SHA256_DIGEST_LENGTH, 0);

Check return value?


> +
> +	return platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +}
> +
> +// Flattens a linked-list bank into a contiguous buffer for writing
> +static int secboot_serialize_bank(struct list_head *bank, char *target, size_t target_size, int flags)
> +{
> +	struct secvar_node *node;
> +	char *tmp = target;
> +
> +	if (!bank)
> +		return OPAL_INTERNAL_ERROR;
> +	if (!target)
> +		return OPAL_INTERNAL_ERROR;

if (!bank || !target)


> +
> +	list_for_each(bank, node, link) {
> +		if (node->flags != flags)
> +			continue;
> +
> +		// Bail early if we are out of storage space
> +		if ((target - tmp) + sizeof(struct secvar) + node->var->data_size > target_size) {
> +			return OPAL_EMPTY;
> +		}
> +		
> +		memcpy(target, node->var, sizeof(struct secvar) + node->var->data_size);
> +
> +		target += sizeof(struct secvar) + node->var->data_size;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +
> +static int secboot_load_from_pnor(struct list_head *bank, char *source, size_t max_size)

Source is an array containing flattened secvars? Add description to 
function. And call it source_size so one knows source and source_size go 
together?


> +{
> +	char *src;
> +	struct secvar_node *tmp;
> +	struct secvar *hdr;
> +
> +	src = source;
> +
> +	while (src < (source + max_size)) {
> +		// Load in the header first to get the size, and check if we are at the end
> +		hdr = (struct secvar *) src;
> +		if (hdr->key_len == 0) {
> +			break;
> +		}
> +
> +		tmp = alloc_secvar(hdr->data_size);
> +		if (!tmp) {
> +			prlog(PR_ERR, "Could not allocate memory for loading secvar from image\n");
> +			return OPAL_NO_MEM;
> +		}
> +
> +		memcpy(tmp->var, src, sizeof(struct secvar) + hdr->data_size);
> +
> +		list_add_tail(bank, &tmp->link);
> +		src += sizeof(struct secvar) + hdr->data_size;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +
> +static int secboot_tpm_write_bank(struct list_head *bank, int section)
> +{
> +	int rc;
> +	uint64_t bit;
> +	char bank_hash[SHA256_DIGEST_LENGTH];
> +
> +	switch(section) {
> +		case SECVAR_VARIABLE_BANK:
> +			// Get the current bit and flip it
> +			secvar_tpmnv_read(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);
Check return code ?
> +			bit = CYCLE_BIT(bit);
> +
> +			// Calculate the bank hash, and write to TPM NV
> +			rc = secboot_serialize_bank(bank, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE, 0);
> +			if (rc)
> +				break;
> +
> +			calc_bank_hash(bank_hash, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
> +			rc = secvar_tpmnv_write(GET_HASH_BANK_ID(bit), bank_hash, SHA256_DIGEST_LENGTH, 0);
If project policy is max chars per line is 80, you need to reformat.0
> +			if (rc)
> +				break;
> +
> +			// Write new variable bank to pnor
> +			rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));


If this now failed do we need to undo something from the previous 
secvar_tpmnv_write()?


> +			if (rc)
> +				break;
> +
> +			// Flip the bit, and write to TPM NV
> +			rc = secvar_tpmnv_write(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);

So this activates both pnor and the hash value in TPM NVRAM? And if this 
was to fail what happens to PNOR contents? Are they going to be rejected 
due to non matching hash (I suppose)?


> +			break;
> +		case SECVAR_UPDATE_BANK:
> +			memset(secboot_image->update, 0, SECBOOT_UPDATE_BANK_SIZE);
> +			rc = secboot_serialize_bank(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE, 0);
> +			if (rc)
> +				break;
> +
> +			rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +			break;
> +		default:
> +			rc = OPAL_HARDWARE;
> +	}
> +
> +	return rc;
> +}
> +
> +
> +static int secboot_tpm_load_variable_bank(struct list_head *bank)
> +{
> +	char bank_hash[SHA256_DIGEST_LENGTH];
> +	char tpm_bank_hash[SHA256_DIGEST_LENGTH];
> +	uint64_t bit;
> +
> +	secvar_tpmnv_read(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);
> +	secvar_tpmnv_read(GET_HASH_BANK_ID(bit), tpm_bank_hash, SHA256_DIGEST_LENGTH, 0);
Check return values?
> +
> +	calc_bank_hash(bank_hash, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
> +	if (memcmp(bank_hash, tpm_bank_hash, SHA256_DIGEST_LENGTH))
> +		return OPAL_PERMISSION; // Tampered pnor space detected, abandon ship


Ah, needs to format...


> +
> +	return secboot_load_from_pnor(bank, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
> +}
> +
> +
> +static int secboot_tpm_load_bank(struct list_head *bank, int section)
> +{
> +	switch(section) {
> +		case SECVAR_VARIABLE_BANK:
> +			return secboot_tpm_load_variable_bank(bank);
> +		case SECVAR_UPDATE_BANK:
> +			return secboot_load_from_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
> +		default:
> +			return OPAL_HARDWARE;
> +	}
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +
> +static int secboot_tpm_store_init(void)
> +{
> +	int rc;
> +	unsigned secboot_size;
> +
> +	// Already initialized
> +	if (secboot_image)
> +		return OPAL_SUCCESS;
> +
> +	if (!platform.secboot_info)
> +		return OPAL_UNSUPPORTED;
> +
> +	prlog(PR_DEBUG, "Initializing for pnor+tpm based platform\n");
> +
> +	rc = secvar_tpmnv_alloc(TPMNV_ID_ACTIVE_BIT, sizeof(uint64_t));
> +	rc |= secvar_tpmnv_alloc(TPMNV_ID_HASH_BANK_0, SHA256_DIGEST_LENGTH);
> +	rc |= secvar_tpmnv_alloc(TPMNV_ID_HASH_BANK_1, SHA256_DIGEST_LENGTH);
> +	if (rc) {
> +		prlog(PR_ERR, "unable to alloc or find the tpmnv space, rc = %d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = platform.secboot_info(&secboot_size);
> +	if (rc) {
> +		prlog(PR_ERR, "error %d retrieving keystore info\n", rc);
> +		return rc;
> +	}
> +	if (sizeof(struct secboot) > secboot_size) {
> +		prlog(PR_ERR, "secboot partition %d KB too small. min=%ld\n",
> +		      secboot_size >> 10, sizeof(struct secboot));
> +		return OPAL_RESOURCE;
> +	}
> +
> +	secboot_image = memalign(0x1000, sizeof(struct secboot));
> +	if (!secboot_image) {
> +		prlog(PR_ERR, "Failed to allocate space for the secboot image\n");
> +		return OPAL_NO_MEM;
> +	}
> +
> +	/* Read it in */
> +	rc = platform.secboot_read(secboot_image, 0, sizeof(struct secboot));
> +	if (rc) {
> +		prlog(PR_ERR, "failed to read the secboot partition, rc=%d\n", rc);
> +		goto out_free;
> +	}
> +
> +	if ((secboot_image->header.magic_number != SECBOOT_MAGIC_NUMBER)
> +	     || tpm_first_init ) {
> +		prlog(PR_INFO, "Formatting secboot partition...\n");
> +		rc = secboot_format();
> +		if (rc) {
> +			prlog(PR_ERR, "Failed to format secboot!\n");
> +			goto out_free;
> +		}
> +	}
> +
> +	return OPAL_SUCCESS;
> +
> +out_free:
> +	if (secboot_image) {


no need to check


> +		free(secboot_image);
> +		secboot_image = NULL;
> +	}
> +
> +	return rc;
> +}
> +
> +struct secvar_storage_driver secboot_tpm_driver = {
> +	.load_bank = secboot_tpm_load_bank,
> +	.write_bank = secboot_tpm_write_bank,
> +	.store_init = secboot_tpm_store_init,
> +	.max_var_size = 8192,
> +};
> diff --git a/libstb/secvar/storage/secboot_tpm.h b/libstb/secvar/storage/secboot_tpm.h
> new file mode 100644
> index 00000000..ea48688c
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm.h
> @@ -0,0 +1,26 @@
> +#ifndef _SECBOOT_TPM_H_
> +#define _SECBOOT_TPM_H_
> +
> +// TODO: Determine reasonable values for these, or have platform set it?
> +#define SECBOOT_VARIABLE_BANK_SIZE	32000
> +#define SECBOOT_UPDATE_BANK_SIZE	32000
> +
> +#define SECBOOT_VARIABLE_BANK_NUM	2
> +
> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
> +#define SECBOOT_MAGIC_NUMBER	0x5053424b
> +#define SECBOOT_VERSION		1
> +
> +struct secboot_header {
> +	uint32_t magic_number;
> +	uint8_t version;
> +	uint8_t reserved[3];	// Fix alignment
> +} __packed;
> +
> +struct secboot {
> +	struct secboot_header header;
> +	char bank[SECBOOT_VARIABLE_BANK_NUM][SECBOOT_VARIABLE_BANK_SIZE];
> +	char update[SECBOOT_UPDATE_BANK_SIZE];
> +} __packed;
> +
> +#endif
> diff --git a/libstb/secvar/test/Makefile.check b/libstb/secvar/test/Makefile.check
> index 6dc24f1e..b704a071 100644
> --- a/libstb/secvar/test/Makefile.check
> +++ b/libstb/secvar/test/Makefile.check
> @@ -5,7 +5,7 @@ SECVAR_TEST_DIR = libstb/secvar/test
>   
>   SECVAR_TEST = $(patsubst %.c, %, $(wildcard $(SECVAR_TEST_DIR)/secvar-test-*.c))
>   
> -HOSTCFLAGS+=-I . -I include
> +HOSTCFLAGS+=-I . -I include -I libstb/tss2
>   
>   .PHONY : secvar-check
>   secvar-check: $(SECVAR_TEST:%=%-check) $(SECVAR_TEST:%=%-gcov-run)
> diff --git a/libstb/secvar/test/secvar-test-secboot-tpm.c b/libstb/secvar/test/secvar-test-secboot-tpm.c
> new file mode 100644
> index 00000000..acc2da5b
> --- /dev/null
> +++ b/libstb/secvar/test/secvar-test-secboot-tpm.c
> @@ -0,0 +1,142 @@
> +#include "secvar_common_test.c"
> +#include "../storage/secboot_tpm.c"
> +#include "../../crypto/mbedtls/library/sha256.c"
> +#include "../../crypto/mbedtls/library/platform_util.c"
> +#include "../secvar_util.c"
> +
> +#define TSS_NV_Read NULL
> +#define TSS_NV_Write NULL
> +#define TSS_NV_Define_Space NULL
> +
> +#include "../secvar_tpmnv.c"
> +
> +char *secboot_buffer;
static?
> +
> +#define ARBITRARY_SECBOOT_SIZE 128000
> +
> +const char *secvar_test_name = "secboot_tpm";
> +
> +static int secboot_read(void *dst, uint32_t src, uint32_t len)
> +{
> +	memcpy(dst, secboot_buffer + src, len);
> +	return 0;
> +}
> +
> +static int secboot_write(uint32_t dst, void *src, uint32_t len)
> +{
> +	memcpy(secboot_buffer + dst, src, len);
> +	return 0;
> +}
> +
> +static int secboot_info(uint32_t *total_size)
> +{
> +	*total_size = ARBITRARY_SECBOOT_SIZE;
> +	return 0;
> +}
> +
> +struct platform platform;
> +
> +int run_test(void)
> +{
> +	int rc;
> +	struct secvar_node *tmp;
> +
> +	platform.secboot_read = secboot_read;
> +	platform.secboot_write = secboot_write;
> +	platform.secboot_info = secboot_info;
> +
> +	secboot_buffer = zalloc(ARBITRARY_SECBOOT_SIZE);
> +
> +	// Initialize and format the storage
> +	rc = secboot_tpm_store_init();
> +	ASSERT(OPAL_SUCCESS == rc);
> +
> +	// Load the just-formatted empty section
> +	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(OPAL_SUCCESS == rc);
> +	ASSERT(0 == list_length(&variable_bank));
> +
> +	// Add some test variables
> +	tmp = alloc_secvar(8);
> +	tmp->var->key_len = 5;
> +	memcpy(tmp->var->key, "test", 5);
> +	tmp->var->data_size = 8;
> +	memcpy(tmp->var->data, "testdata", 8);
> +	list_add_tail(&variable_bank, &tmp->link);
> +
> +	tmp = alloc_secvar(8);
> +	tmp->var->key_len = 4;
> +	memcpy(tmp->var->key, "foo", 4);
> +	tmp->var->data_size = 8;
> +	memcpy(tmp->var->data, "moredata", 8);
> +	list_add_tail(&variable_bank, &tmp->link);
> +
> +	// Write the bank
> +	rc = secboot_tpm_write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(OPAL_SUCCESS == rc);
> +	// should write to bank 1 first
> +	ASSERT(secboot_image->bank[1][0] != 0);
> +	ASSERT(secboot_image->bank[0][0] == 0);
> +
> +	// Clear the variable list
> +	clear_bank_list(&variable_bank);
> +	ASSERT(0 == list_length(&variable_bank));
> +
> +	// Load the bank
> +	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(OPAL_SUCCESS == rc);
> +	ASSERT(2 == list_length(&variable_bank));
> +
> +	// Change a variable
> +	tmp = list_top(&variable_bank, struct secvar_node, link);
> +	memcpy(tmp->var->data, "somethin", 8);
> +
> +	// Write the bank
> +	rc = secboot_tpm_write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(OPAL_SUCCESS == rc);
> +	// should have data in both now	
> +	ASSERT(secboot_image->bank[0][0] != 0);
> +	ASSERT(secboot_image->bank[1][0] != 0);
> +
> +	clear_bank_list(&variable_bank);
> +
> +	// Tamper with pnor, hash check should catch this
> +	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
> +
> +	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(rc != OPAL_SUCCESS); // TODO: permission?
> +
> +	// Fix it back...
> +	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
> +
> +	// Should be ok again
> +	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	ASSERT(rc == OPAL_SUCCESS);
> +
> +	clear_bank_list(&variable_bank);
> +	free(secboot_buffer);
> +
> +	return 0;
> +}
> +
> +int main(void)
> +{
> +	int rc = 0;
> +
> +	tpm_fake_nv = 1;
> +	tpm_fake_nv_offset = 0;
> +
> +	list_head_init(&variable_bank);
> +
> +	rc = run_test();
> +
> +	if (rc)
> +		printf(COLOR_RED "FAILED" COLOR_RESET "\n");
> +	else
> +		printf(COLOR_GREEN "OK" COLOR_RESET "\n");
> +
> +	free(tpm_image);
> +	free(secboot_image);
> +
> +	return rc;
> +}
diff mbox series

Patch

diff --git a/include/secvar.h b/include/secvar.h
index c41fb739..2875c700 100644
--- a/include/secvar.h
+++ b/include/secvar.h
@@ -23,6 +23,7 @@  struct secvar_backend_driver {
         const char *compatible;			// String to use for compatible in secvar node
 };
 
+extern struct secvar_storage_driver secboot_tpm_driver;
 
 int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
 
diff --git a/libstb/secvar/storage/Makefile.inc b/libstb/secvar/storage/Makefile.inc
index b7a821ec..5926e2f5 100644
--- a/libstb/secvar/storage/Makefile.inc
+++ b/libstb/secvar/storage/Makefile.inc
@@ -1,11 +1,11 @@ 
 # SPDX-License-Identifier: Apache-2.0
 # -*-Makefile-*-
 
-SECVAR_STORAGE_DIR = libstb/secvar/storage
+SECVAR_STORAGE_DIR = $(SRC)/libstb/secvar/storage
 
 SUBDIRS += $(SECVAR_STORAGE_DIR)
 
-SECVAR_STORAGE_SRCS =
+SECVAR_STORAGE_SRCS = secboot_tpm.c
 SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
 SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
 
diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
new file mode 100644
index 00000000..9a3d228a
--- /dev/null
+++ b/libstb/secvar/storage/secboot_tpm.c
@@ -0,0 +1,267 @@ 
+// SPDX-License-Identifier: Apache-2.0
+/* Copyright 2019 IBM Corp. */
+#ifndef pr_fmt
+#define pr_fmt(fmt) "SECBOOT_TPM: " fmt
+#endif
+
+#include <stdlib.h>
+#include <skiboot.h>
+#include <opal.h>
+#include <mbedtls/sha256.h>
+#include "../secvar.h"
+#include "../secvar_tpmnv.h"
+#include "secboot_tpm.h"
+
+//#define CYCLE_BIT(b) (((((b-1)%SECBOOT_VARIABLE_BANK_NUM)+1)%SECBOOT_VARIABLE_BANK_NUM)+1)
+#define CYCLE_BIT(b) (b^0x1)
+
+#define TPMNV_ID_ACTIVE_BIT	0x53414242 // SABB
+#define TPMNV_ID_HASH_BANK_0	0x53484230 // SHB0
+#define TPMNV_ID_HASH_BANK_1	0x53484231 // SHB1
+
+#define GET_HASH_BANK_ID(bit) ((bit)?TPMNV_ID_HASH_BANK_1:TPMNV_ID_HASH_BANK_0)
+
+// Because mbedtls doesn't define this?
+#define SHA256_DIGEST_LENGTH	32
+
+struct secboot *secboot_image;
+
+static void calc_bank_hash(char *target_hash, char *source_buf, uint64_t size)
+{
+	mbedtls_sha256_context ctx;
+
+	mbedtls_sha256_init(&ctx);
+	mbedtls_sha256_update_ret(&ctx, source_buf, size);
+	mbedtls_sha256_finish_ret(&ctx, target_hash);
+}
+
+static int secboot_format(void)
+{
+	char bank_hash[SHA256_DIGEST_LENGTH];
+
+	if (!platform.secboot_write)
+		return OPAL_UNSUPPORTED;
+
+	memset(secboot_image, 0x00, sizeof(struct secboot));
+
+	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
+	secboot_image->header.version = SECBOOT_VERSION;
+
+	// Write the empty hash to the tpm so loads work in the future
+	calc_bank_hash(bank_hash, secboot_image->bank[0], SECBOOT_VARIABLE_BANK_SIZE);
+	secvar_tpmnv_write(TPMNV_ID_HASH_BANK_0, bank_hash, SHA256_DIGEST_LENGTH, 0);
+
+	return platform.secboot_write(0, secboot_image, sizeof(struct secboot));
+}
+
+// Flattens a linked-list bank into a contiguous buffer for writing
+static int secboot_serialize_bank(struct list_head *bank, char *target, size_t target_size, int flags)
+{
+	struct secvar_node *node;
+	char *tmp = target;
+
+	if (!bank)
+		return OPAL_INTERNAL_ERROR;
+	if (!target)
+		return OPAL_INTERNAL_ERROR;
+
+	list_for_each(bank, node, link) {
+		if (node->flags != flags)
+			continue;
+
+		// Bail early if we are out of storage space
+		if ((target - tmp) + sizeof(struct secvar) + node->var->data_size > target_size) {
+			return OPAL_EMPTY;
+		}
+		
+		memcpy(target, node->var, sizeof(struct secvar) + node->var->data_size);
+
+		target += sizeof(struct secvar) + node->var->data_size;
+	}
+
+	return OPAL_SUCCESS;
+}
+
+
+static int secboot_load_from_pnor(struct list_head *bank, char *source, size_t max_size)
+{
+	char *src;
+	struct secvar_node *tmp;
+	struct secvar *hdr;
+
+	src = source;
+
+	while (src < (source + max_size)) {
+		// Load in the header first to get the size, and check if we are at the end
+		hdr = (struct secvar *) src;
+		if (hdr->key_len == 0) {
+			break;
+		}
+
+		tmp = alloc_secvar(hdr->data_size);
+		if (!tmp) {
+			prlog(PR_ERR, "Could not allocate memory for loading secvar from image\n");
+			return OPAL_NO_MEM;
+		}
+
+		memcpy(tmp->var, src, sizeof(struct secvar) + hdr->data_size);
+
+		list_add_tail(bank, &tmp->link);
+		src += sizeof(struct secvar) + hdr->data_size;
+	}
+
+	return OPAL_SUCCESS;
+}
+
+
+static int secboot_tpm_write_bank(struct list_head *bank, int section)
+{
+	int rc;
+	uint64_t bit;
+	char bank_hash[SHA256_DIGEST_LENGTH];
+
+	switch(section) {
+		case SECVAR_VARIABLE_BANK:
+			// Get the current bit and flip it
+			secvar_tpmnv_read(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);
+			bit = CYCLE_BIT(bit);
+
+			// Calculate the bank hash, and write to TPM NV
+			rc = secboot_serialize_bank(bank, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE, 0);
+			if (rc)
+				break;
+
+			calc_bank_hash(bank_hash, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
+			rc = secvar_tpmnv_write(GET_HASH_BANK_ID(bit), bank_hash, SHA256_DIGEST_LENGTH, 0);
+			if (rc)
+				break;
+
+			// Write new variable bank to pnor
+			rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
+			if (rc)
+				break;
+
+			// Flip the bit, and write to TPM NV
+			rc = secvar_tpmnv_write(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);
+			break;
+		case SECVAR_UPDATE_BANK:
+			memset(secboot_image->update, 0, SECBOOT_UPDATE_BANK_SIZE);
+			rc = secboot_serialize_bank(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE, 0);
+			if (rc)
+				break;
+
+			rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
+			break;
+		default:
+			rc = OPAL_HARDWARE;
+	}
+
+	return rc;
+}
+
+
+static int secboot_tpm_load_variable_bank(struct list_head *bank)
+{
+	char bank_hash[SHA256_DIGEST_LENGTH];
+	char tpm_bank_hash[SHA256_DIGEST_LENGTH];
+	uint64_t bit;
+
+	secvar_tpmnv_read(TPMNV_ID_ACTIVE_BIT, &bit, sizeof(bit), 0);
+	secvar_tpmnv_read(GET_HASH_BANK_ID(bit), tpm_bank_hash, SHA256_DIGEST_LENGTH, 0);
+
+	calc_bank_hash(bank_hash, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
+	if (memcmp(bank_hash, tpm_bank_hash, SHA256_DIGEST_LENGTH))
+		return OPAL_PERMISSION; // Tampered pnor space detected, abandon ship
+
+	return secboot_load_from_pnor(bank, secboot_image->bank[bit], SECBOOT_VARIABLE_BANK_SIZE);
+}
+
+
+static int secboot_tpm_load_bank(struct list_head *bank, int section)
+{
+	switch(section) {
+		case SECVAR_VARIABLE_BANK:
+			return secboot_tpm_load_variable_bank(bank);
+		case SECVAR_UPDATE_BANK:
+			return secboot_load_from_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
+		default:
+			return OPAL_HARDWARE;
+	}
+
+	return OPAL_HARDWARE;
+}
+
+
+static int secboot_tpm_store_init(void)
+{
+	int rc;
+	unsigned secboot_size;
+
+	// Already initialized
+	if (secboot_image)
+		return OPAL_SUCCESS;
+
+	if (!platform.secboot_info)
+		return OPAL_UNSUPPORTED;
+
+	prlog(PR_DEBUG, "Initializing for pnor+tpm based platform\n");
+
+	rc = secvar_tpmnv_alloc(TPMNV_ID_ACTIVE_BIT, sizeof(uint64_t));
+	rc |= secvar_tpmnv_alloc(TPMNV_ID_HASH_BANK_0, SHA256_DIGEST_LENGTH);
+	rc |= secvar_tpmnv_alloc(TPMNV_ID_HASH_BANK_1, SHA256_DIGEST_LENGTH);
+	if (rc) {
+		prlog(PR_ERR, "unable to alloc or find the tpmnv space, rc = %d\n", rc);
+		return rc;
+	}
+
+	rc = platform.secboot_info(&secboot_size);
+	if (rc) {
+		prlog(PR_ERR, "error %d retrieving keystore info\n", rc);
+		return rc;
+	}
+	if (sizeof(struct secboot) > secboot_size) {
+		prlog(PR_ERR, "secboot partition %d KB too small. min=%ld\n",
+		      secboot_size >> 10, sizeof(struct secboot));
+		return OPAL_RESOURCE;
+	}
+
+	secboot_image = memalign(0x1000, sizeof(struct secboot));
+	if (!secboot_image) {
+		prlog(PR_ERR, "Failed to allocate space for the secboot image\n");
+		return OPAL_NO_MEM;
+	}
+
+	/* Read it in */
+	rc = platform.secboot_read(secboot_image, 0, sizeof(struct secboot));
+	if (rc) {
+		prlog(PR_ERR, "failed to read the secboot partition, rc=%d\n", rc);
+		goto out_free;
+	}
+
+	if ((secboot_image->header.magic_number != SECBOOT_MAGIC_NUMBER)
+	     || tpm_first_init ) {
+		prlog(PR_INFO, "Formatting secboot partition...\n");
+		rc = secboot_format();
+		if (rc) {
+			prlog(PR_ERR, "Failed to format secboot!\n");
+			goto out_free;
+		}
+	}
+
+	return OPAL_SUCCESS;
+
+out_free:
+	if (secboot_image) {
+		free(secboot_image);
+		secboot_image = NULL;
+	}
+
+	return rc;
+}
+
+struct secvar_storage_driver secboot_tpm_driver = {
+	.load_bank = secboot_tpm_load_bank,
+	.write_bank = secboot_tpm_write_bank,
+	.store_init = secboot_tpm_store_init,
+	.max_var_size = 8192,
+};
diff --git a/libstb/secvar/storage/secboot_tpm.h b/libstb/secvar/storage/secboot_tpm.h
new file mode 100644
index 00000000..ea48688c
--- /dev/null
+++ b/libstb/secvar/storage/secboot_tpm.h
@@ -0,0 +1,26 @@ 
+#ifndef _SECBOOT_TPM_H_
+#define _SECBOOT_TPM_H_
+
+// TODO: Determine reasonable values for these, or have platform set it?
+#define SECBOOT_VARIABLE_BANK_SIZE	32000
+#define SECBOOT_UPDATE_BANK_SIZE	32000
+
+#define SECBOOT_VARIABLE_BANK_NUM	2
+
+/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
+#define SECBOOT_MAGIC_NUMBER	0x5053424b
+#define SECBOOT_VERSION		1
+
+struct secboot_header {
+	uint32_t magic_number;
+	uint8_t version;
+	uint8_t reserved[3];	// Fix alignment
+} __packed;
+
+struct secboot {
+	struct secboot_header header;
+	char bank[SECBOOT_VARIABLE_BANK_NUM][SECBOOT_VARIABLE_BANK_SIZE];
+	char update[SECBOOT_UPDATE_BANK_SIZE];
+} __packed;
+
+#endif
diff --git a/libstb/secvar/test/Makefile.check b/libstb/secvar/test/Makefile.check
index 6dc24f1e..b704a071 100644
--- a/libstb/secvar/test/Makefile.check
+++ b/libstb/secvar/test/Makefile.check
@@ -5,7 +5,7 @@  SECVAR_TEST_DIR = libstb/secvar/test
 
 SECVAR_TEST = $(patsubst %.c, %, $(wildcard $(SECVAR_TEST_DIR)/secvar-test-*.c))
 
-HOSTCFLAGS+=-I . -I include
+HOSTCFLAGS+=-I . -I include -I libstb/tss2
 
 .PHONY : secvar-check
 secvar-check: $(SECVAR_TEST:%=%-check) $(SECVAR_TEST:%=%-gcov-run)
diff --git a/libstb/secvar/test/secvar-test-secboot-tpm.c b/libstb/secvar/test/secvar-test-secboot-tpm.c
new file mode 100644
index 00000000..acc2da5b
--- /dev/null
+++ b/libstb/secvar/test/secvar-test-secboot-tpm.c
@@ -0,0 +1,142 @@ 
+#include "secvar_common_test.c"
+#include "../storage/secboot_tpm.c"
+#include "../../crypto/mbedtls/library/sha256.c"
+#include "../../crypto/mbedtls/library/platform_util.c"
+#include "../secvar_util.c"
+
+#define TSS_NV_Read NULL
+#define TSS_NV_Write NULL
+#define TSS_NV_Define_Space NULL
+
+#include "../secvar_tpmnv.c"
+
+char *secboot_buffer;
+
+#define ARBITRARY_SECBOOT_SIZE 128000
+
+const char *secvar_test_name = "secboot_tpm";
+
+static int secboot_read(void *dst, uint32_t src, uint32_t len)
+{
+	memcpy(dst, secboot_buffer + src, len);
+	return 0;
+}
+
+static int secboot_write(uint32_t dst, void *src, uint32_t len)
+{
+	memcpy(secboot_buffer + dst, src, len);
+	return 0;
+}
+
+static int secboot_info(uint32_t *total_size)
+{
+	*total_size = ARBITRARY_SECBOOT_SIZE;
+	return 0;
+}
+
+struct platform platform;
+
+int run_test(void)
+{
+	int rc;
+	struct secvar_node *tmp;
+
+	platform.secboot_read = secboot_read;
+	platform.secboot_write = secboot_write;
+	platform.secboot_info = secboot_info;
+
+	secboot_buffer = zalloc(ARBITRARY_SECBOOT_SIZE);
+
+	// Initialize and format the storage
+	rc = secboot_tpm_store_init();
+	ASSERT(OPAL_SUCCESS == rc);
+
+	// Load the just-formatted empty section
+	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(OPAL_SUCCESS == rc);
+	ASSERT(0 == list_length(&variable_bank));
+
+	// Add some test variables
+	tmp = alloc_secvar(8);
+	tmp->var->key_len = 5;
+	memcpy(tmp->var->key, "test", 5);
+	tmp->var->data_size = 8;
+	memcpy(tmp->var->data, "testdata", 8);
+	list_add_tail(&variable_bank, &tmp->link);
+
+	tmp = alloc_secvar(8);
+	tmp->var->key_len = 4;
+	memcpy(tmp->var->key, "foo", 4);
+	tmp->var->data_size = 8;
+	memcpy(tmp->var->data, "moredata", 8);
+	list_add_tail(&variable_bank, &tmp->link);
+
+	// Write the bank
+	rc = secboot_tpm_write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(OPAL_SUCCESS == rc);
+	// should write to bank 1 first
+	ASSERT(secboot_image->bank[1][0] != 0); 
+	ASSERT(secboot_image->bank[0][0] == 0);
+
+	// Clear the variable list
+	clear_bank_list(&variable_bank);
+	ASSERT(0 == list_length(&variable_bank));
+
+	// Load the bank
+	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(OPAL_SUCCESS == rc);
+	ASSERT(2 == list_length(&variable_bank));
+
+	// Change a variable
+	tmp = list_top(&variable_bank, struct secvar_node, link);
+	memcpy(tmp->var->data, "somethin", 8);
+
+	// Write the bank
+	rc = secboot_tpm_write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(OPAL_SUCCESS == rc);
+	// should have data in both now	
+	ASSERT(secboot_image->bank[0][0] != 0); 
+	ASSERT(secboot_image->bank[1][0] != 0);
+
+	clear_bank_list(&variable_bank);
+
+	// Tamper with pnor, hash check should catch this
+	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
+
+	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(rc != OPAL_SUCCESS); // TODO: permission?
+
+	// Fix it back...
+	secboot_image->bank[0][0] = ~secboot_image->bank[0][0];
+
+	// Should be ok again
+	rc = secboot_tpm_load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	ASSERT(rc == OPAL_SUCCESS);
+
+	clear_bank_list(&variable_bank);
+	free(secboot_buffer);
+
+	return 0;
+}
+
+int main(void)
+{
+	int rc = 0;
+
+	tpm_fake_nv = 1;
+	tpm_fake_nv_offset = 0;
+
+	list_head_init(&variable_bank);
+
+	rc = run_test();
+
+	if (rc)
+		printf(COLOR_RED "FAILED" COLOR_RESET "\n");
+	else
+		printf(COLOR_GREEN "OK" COLOR_RESET "\n");
+
+	free(tpm_image);
+	free(secboot_image);
+
+	return rc;
+}