[v4,11/18] secvar/storage: add secvar storage driver for pnor-based p9

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>

---
V4:
 - add documentation for this driver
 - removed unused SECURE_STORAGE flag, renamed PRIORITY to PROTECTED
     This is still open to suggestions, PROTECTED sounded more clear)
 - fixed missing pointer advancement when writing protected variables
 - updated the NV public name hashes to match the current set of
   attributes used to define the indices
 - moved the expected NV public name hashes to seperate header
 - cleaned up most line lengths to under 80 columns, with the exception
     of prlog statements and possibly a few others.

 doc/secvar/secboot_tpm.rst                    | 175 +++++
 include/secvar.h                              |   1 +
 libstb/secvar/secvar.h                        |   4 +-
 libstb/secvar/storage/Makefile.inc            |   5 +-
 libstb/secvar/storage/secboot_tpm.c           | 678 ++++++++++++++++++
 libstb/secvar/storage/secboot_tpm.h           |  61 ++
 .../secvar/storage/secboot_tpm_public_name.h  |  18 +
 libstb/secvar/storage/tpmnv_ops.c             |  15 +
 8 files changed, 953 insertions(+), 4 deletions(-)
 create mode 100644 doc/secvar/secboot_tpm.rst
 create mode 100644 libstb/secvar/storage/secboot_tpm.c
 create mode 100644 libstb/secvar/storage/secboot_tpm.h
 create mode 100644 libstb/secvar/storage/secboot_tpm_public_name.h
 create mode 100644 libstb/secvar/storage/tpmnv_ops.c

On Mon, 2020-05-11 at 16:31 -0500, 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>
> 
> ---
> V4:
>  - add documentation for this driver
>  - removed unused SECURE_STORAGE flag, renamed PRIORITY to PROTECTED
>      This is still open to suggestions, PROTECTED sounded more clear)
>  - fixed missing pointer advancement when writing protected variables
>  - updated the NV public name hashes to match the current set of
>    attributes used to define the indices
>  - moved the expected NV public name hashes to seperate header
>  - cleaned up most line lengths to under 80 columns, with the exception
>      of prlog statements and possibly a few others.
> 
>  doc/secvar/secboot_tpm.rst                    | 175 +++++
>  include/secvar.h                              |   1 +
>  libstb/secvar/secvar.h                        |   4 +-
>  libstb/secvar/storage/Makefile.inc            |   5 +-
>  libstb/secvar/storage/secboot_tpm.c           | 678 ++++++++++++++++++
>  libstb/secvar/storage/secboot_tpm.h           |  61 ++
>  .../secvar/storage/secboot_tpm_public_name.h  |  18 +
>  libstb/secvar/storage/tpmnv_ops.c             |  15 +
>  8 files changed, 953 insertions(+), 4 deletions(-)
>  create mode 100644 doc/secvar/secboot_tpm.rst
>  create mode 100644 libstb/secvar/storage/secboot_tpm.c
>  create mode 100644 libstb/secvar/storage/secboot_tpm.h
>  create mode 100644 libstb/secvar/storage/secboot_tpm_public_name.h
>  create mode 100644 libstb/secvar/storage/tpmnv_ops.c
> 
> diff --git a/doc/secvar/secboot_tpm.rst b/doc/secvar/secboot_tpm.rst
> new file mode 100644
> index 00000000..8da0c2f0
> --- /dev/null
> +++ b/doc/secvar/secboot_tpm.rst
> @@ -0,0 +1,175 @@
> +.. _secvar/secboot_tpm:
> +
> +secboot_tpm secvar storage driver for P9 platforms
> +==================================================
> +
> +Overview
> +--------
> +
> +This storage driver utilizes the SECBOOT PNOR partition and TPM NV space to
> +persist secure variables across reboots in a tamper-resistant manner. While
> +writes to PNOR cannot be completely prevented, writes CAN be prevented to TPM
> +NV. On the other hand, there is limited available space in TPM NV.
> +
> +Therefore, this driver uses both in conjunction: large variable data is written
> +to SECBOOT, and a hash of the variable data is stored in TPM NV. When the
> +variables are loaded from SECBOOT, this hash is recalculated and compared
> +against the value stored in the TPM. If they do not match, then the variables
> +must have been altered and are not loaded.
> +
> +See the following sections for more information on the internals of the driver.
> +
> +
> +Storage Layouts
> +---------------
> +
> +At a high-level, there are a few major logical components:
> +
> + - (PNOR) Variable storage (split in half, active/staging)
> + - (PNOR) Update storage
> + - (TPM)  Protected variable storage
> + - (TPM)  Bank hashes & active bit
> +
> +Variable storage consists of two smaller banks, variable bank 0 and variable
> +bank 1. Either of the banks may be designated "active" by setting the active
> +bank bit to either 0 or 1, indicating that the corresponding bank is now
> +"active". The other bank is then considered "staging". See the "Persisting
> +Variable Bank Updates" for more on the active/staging bank logic.
> +
> +Protected variable storage is stored in ``VARS`` TPM NV index. Unlike the other
> +variable storage, there is only one bank due to limited storage space. See the
> +TPM NV Indices section for more.
> +
> +
> +Persisting the Variable Bank
> +----------------------------
> +
> +When writing a new variable bank to storage, this is (roughly) the procedure the
> +driver will follow:
> +
> +1. write variables to the staging bank
> +2. calculate hash of the staging bank
> +3. store the staging bank hash in the TPM NV
> +4. flip the active bank bit
> +
> +This procedure ensures that the switch-over from the old variables to the
> +new variables is as atomic as possible. This should prevent any possible
> +issues caused by an interruption during the writing process, such as power loss.
> +
> +The bank hashes are a SHA256 hash calculated over the whole region of
> +storage space allocated to the bank, including unused storage. For consistency,
> +unused space is always written as zeroes. Like the active/staging variable
> +banks, there are also two corresponding active/staging bank hashes stored in
> +the TPM.
> +
> +
> +TPM NV Indices
> +--------------
> +
> +The driver utilizes two TPM NV indices:
> +
> +.. code-block:: c
> +
> +  # size). datadefine SECBOOT_TPMNV_VARS_INDEX	0x01c10190
> +  #define SECBOOT_TPMNV_CONTROL_INDEX	0x01c10191
> +
> +The ``VARS`` index stores variables flagged with ``SECVAR_FLAG_PROTECTED``.
> +These variables are critical to the state of OS secure boot, and therefore
> +cannot be safely stored in the SECBOOT partition. This index is defined to be
> +1024 bytes in size, which is enough for the current implementation on P9. It
> +is kept small by default to preserve the very limited NV index space.
> +
> +The ``CONTROL`` index stores the bank hashes, and the bit to determine which
> +bank is active. See the Active/Staging Bank Swapping section for more.
> +
> +Both indices are defined on first boot with the same set of attributes. If the
> +indices are already defined but not in the expected state, (different
> +attributes, size, etc), then the driver will halt the boot. Asserting physical
> +presence will redefine the indices in the correct state.
> +
> +
> +Locking
> +-------
> +
> +PNOR cannot be locked, however the TPM can be. The TPM NV indices are double
> +protected via two locking mechanisms:
> +
> + - The driver's ``.lock()`` hook sends the ``TSS_NV_WriteLock`` TPM command.
> +This sets the ``WRITELOCKED`` attribute, which is cleared on the next
> +TPM reset.
> +
> + - The TPM NV indices are defined under the platform hierarchy. Skiboot will add
> +a global lock to all the NV indices under this hierarchy prior to loading a
> +kernel. This is also reset on the next TPM reset.
> +
> +NOTE: The TPM is only reset during a cold reboot. Fast reboots or kexecs will
> +NOT unlock the TPM.
> +
> +
> +Resetting Storage / Physical Presence
> +-------------------------------------
> +
> +In the case that secure boot/secvar has been rendered unusable, (for example:
> +corrupted data, lost/compromised private key, improperly defined NV indices, etc)
> +this storage driver responds to physical presence assertion as a last-resort
> +method to recover the system.
> +
> +Asserting physical presence undefines, and immediately redefines the TPM NV
> +indices. Defining the NV indices then causes a cascading set of reformats for
> +the remaining components of storage, similar to a first-boot scenario.
> +
> +This driver considers physical presence to be asserted if any of the following
> +device tree nodes are present in ``ibm,secureboot``:
> + - ``clear-os-keys``
> + - ``clear-all-keys``
> + - ``clear-mfg-keys``

what does clear-mfg-keys do?


> +Storage Formats/Layouts
> +=======================
> +
> +SECBOOT (PNOR)
> +--------------
> +
> +Partition Format:
> + - 8b secboot header
> +   - 4b: u32. magic number, always 0x5053424b
> +   - 1b: u8. version, always 1
> +   - 3b: unused padding

This is slightly awkward since the 8 byte header bumps us past the
flash sector alignment boundary (usually 4k) so you end up with some
wasted space.

> + - 32k: secvars. variable bank 0
> + - 32k: secvars. variable bank 1
> + - 32k: secvars. update bank

Why are the bank sizes fixed?

> +Variable Format (secvar):
> + - 8b: u64. key length
> + - 8b: u64. data size
> + - 1k: string. key
> + - (data size). data

1) Why is the key size hardcoded to 1k?  It seems like a waste of our
limited storage space. Especially when you consider that most of the
key names that we define are only a few bytes long. (NB: there's more
on this further down).

2) Any multi-byte integers written to storage needs to have a defined
endianness. We do plan to switch skiboot to LE at some point so this
isn't an academic problem either.

> +TPM VARS (NV)
> +-------------
> +
> +NV Index Format:
> + - 8b secboot header
> +   - 4b: u32. magic number, always 0x5053424b
> +   - 1b: u8. version, always 1
> +   - 3b: unused padding
> + - 1016b: packed secvars. protected variable storage

Where does the 1K size limit come from?

> +Variable Format (packed secvar):
> + - 8b: u64. key length
> + - 8b: u64. data size
> + - (key length): string. key
> + - (data size). data
> +
> +TPM CONTROL (NV)
> +----------------
> +
> + - 8b secboot header
> +   - 4b: u32. magic number, always 0x5053424b
> +   - 1b: u8. version, always 1
> +   - 3b: unused padding
> + - 1b: u8. active bit, 0 or 1
> + - 32b: sha256 hash of variable bank 0
> + - 32b: sha256 hash of variable bank 1

As a general comment these structures being completely fixed means we
have a flag day if they need to change for any reason at all. In
general customers expect to be able to do a switch between version N
and version N-1 without issues since that is sometimes required to work
around problems. This makes me a bit nervous.

> diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
> index b141b705..7e23dcc3 100644
> --- a/libstb/secvar/secvar.h
> +++ b/libstb/secvar/secvar.h
> @@ -23,8 +23,8 @@ struct secvar_node {
>  	uint64_t size;		// How much space was allocated for data
>  };
>  
> -#define SECVAR_FLAG_VOLATILE		0x1 // Instructs storage driver to ignore variable on writes
> -#define SECVAR_FLAG_SECURE_STORAGE	0x2 // Hint for storage driver to select storage location
> +#define SECVAR_FLAG_VOLATILE	0x1 // Instructs storage driver to ignore variable on writes
> +#define SECVAR_FLAG_PROTECTED	0x2 // Hint for storage driver to store in lockable flash

Hint or requirement? Also, comment style.

 
> diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
> new file mode 100644
> index 00000000..012da294
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm.c
> @@ -0,0 +1,678 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2020 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_devtree.h"
> +#include "secboot_tpm.h"
> +#include <tssskiboot.h>
> +#include <ibmtss/TPM_Types.h>
> +
> +#define CYCLE_BIT(b) (b^0x1)
> +
> +#define SECBOOT_TPM_MAX_VAR_SIZE	8192
> +
> +struct secboot *secboot_image = NULL;
> +struct tpmnv_vars *tpmnv_vars_image = NULL;
> +struct tpmnv_control *tpmnv_control_image = NULL;
> +
> +const size_t tpmnv_vars_size = 1024;
> +
> +/* Expected TPM NV index name field from NV_ReadPublic given our known
> + * set of attributes.
> + * See Part 1 Section 16, and Part 2 Section 13.5 of the TPM Specification
> + * for how this is calculated
> + */
> +#include "secboot_tpm_public_name.h"
> +
> +/* Calculate a SHA256 hash over the supplied buffer */
> +static int calc_bank_hash(char *target_hash, char *source_buf, uint64_t size)
> +{
> +	mbedtls_sha256_context ctx;
> +	int rc;
> +
> +	mbedtls_sha256_init(&ctx);
> +
> +	rc = mbedtls_sha256_update_ret(&ctx, source_buf, size);
> +	if (rc)
> +		goto out;
> +
> +	mbedtls_sha256_finish_ret(&ctx, target_hash);
> +	if (rc)
> +		goto out;
> +
> +out:
> +	mbedtls_sha256_free(&ctx);
> +	return rc;
> +}
> +
> +/* Reformat the TPMNV space */
> +static int tpmnv_format(void)
> +{
> +	int rc;
> +
> +	memset(tpmnv_vars_image, 0x00, tpmnv_vars_size);
> +	memset(tpmnv_control_image, 0x00, sizeof(struct tpmnv_control));
> +
> +	tpmnv_vars_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
> +	tpmnv_vars_image->header.version = SECBOOT_VERSION;
> +	tpmnv_control_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
> +	tpmnv_control_image->header.version = SECBOOT_VERSION;
> +
> +	/* Counts as first write to the TPM NV, which sets the
> +	 *  TPMA_NVA_WRITTEN attribute */
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_VARS_INDEX,
> +			     tpmnv_vars_image,
> +			     tpmnv_vars_size, 0);
> +	if (rc) {
> +		prlog(PR_ERR, "Could not write new formatted data to VARS index, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
> +			     tpmnv_control_image,
> +			     sizeof(struct tpmnv_control), 0);
> +	if (rc)
> +		prlog(PR_ERR, "Could not write new formatted data to CONTROL index, rc=%d\n", rc);
> +
> +	return rc;
> +}
> +
> +/* Reformat the secboot PNOR space */
> +static int secboot_format(void)
> +{
> +	int rc;
> +
> +	memset(secboot_image, 0x00, sizeof(struct secboot));
> +
> +	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
> +	secboot_image->header.version = SECBOOT_VERSION;
> +
> +	/* Write the hash of the empty bank to the tpm so future loads work */
> +	rc = calc_bank_hash(tpmnv_control_image->bank_hash[0],
> +			    secboot_image->bank[0],
> +			    SECBOOT_VARIABLE_BANK_SIZE);
> +	if (rc) {
> +		prlog(PR_ERR, "Bank hash failed to calculate somehow\n");
> +		return rc;
> +	}
> +
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
> +			     tpmnv_control_image->bank_hash[0],
> +			     SHA256_DIGEST_SIZE,
> +			     offsetof(struct tpmnv_control,
> +			     bank_hash[0]));
> +	if (rc) {
> +		prlog(PR_ERR, "Could not write fresh formatted bank hashes to CONTROL index, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +	if (rc)
> +		prlog(PR_ERR, "Could not write formatted data to PNOR, rc=%d\n", rc);
> +
> +	return rc;
> +}
> +
> +
> +/* Serialize one priority variable using a tighter packing scheme
> + * Returns the advanced target pointer */
> +static char *secboot_serialize_priority(char *target, struct secvar_node *node, char *end)
> +{
> +	if ((target +
> +	     node->var->key_len +
> +	     node->var->data_size +
> +	     offsetof(struct secvar, key)) > end)
> +		return NULL;
> +
> +	memcpy(target, &node->var->key_len, sizeof(node->var->key_len));
> +	target += sizeof(node->var->key_len);
> +	memcpy(target, &node->var->data_size, sizeof(node->var->data_size));
> +	target += sizeof(node->var->data_size);
> +	memcpy(target, node->var->key, node->var->key_len);
> +	target += node->var->key_len;
> +	memcpy(target, node->var->data, node->var->data_size);
> +	target += node->var->data_size;
> +
> +	return target;
> +}
> +
> +
> +/* 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;
> +	char *end = target + target_size;
> +
> +	if (!bank)
> +		return OPAL_INTERNAL_ERROR;
> +	if (!target)
> +		return OPAL_INTERNAL_ERROR;
> +
> +	memset(target, 0x00, target_size);
> +
> +	list_for_each(bank, node, link) {
> +		if (node->flags != flags)
> +			continue;
> +
> +		/* Priority variable has a different packing scheme */
> +		if (flags & SECVAR_FLAG_PROTECTED) {
> +			target = secboot_serialize_priority(target, node, end);
> +			if (!target)
> +				return OPAL_EMPTY;
> +			continue;
> +		}
> +
> +		/* Bail early if we are out of storage space */
> +		if ((target - tmp) +
> +		    sizeof(struct secvar) +
> +		    node->var->data_size > target_size) {
> +			prlog(PR_ERR, "Ran out of PNOR space, giving up!\n");
> +			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;
> +}
> +
> +/* Loads in a flattened list of variables from a buffer into a linked list */
> +static int secboot_load_from_pnor(struct list_head *bank, char *source, size_t max_size)
source should be const, and probably void.

> +{
> +	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.
> +		 *
> +		 * Banks are zeroized after each write, thus key_len == 0
> +		 *  indicates end of the list */
> +		hdr = (struct secvar *) src;
> +		if (hdr->key_len == 0) {
> +			break;
> +		} else if (hdr->key_len > SECVAR_MAX_KEY_LEN) {
> +			prlog(PR_ERR, "Attempted to load a key larger than max, len = %llu\n", hdr->key_len);
> +			return OPAL_INTERNAL_ERROR;
> +		}
> +
> +		if (hdr->data_size > SECBOOT_TPM_MAX_VAR_SIZE) {
> +			prlog(PR_ERR, "Attempted to load a data payload larger than max, "
> +				      "size = %llu\n", hdr->data_size);
> +			return OPAL_INTERNAL_ERROR;
> +		}
> +
> +		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;
Shouldn't this free all the variables that we did manage to allocate?

> +		}
> +
> +		memcpy(tmp->var, src, sizeof(struct secvar) + hdr->data_size);

As a general rule I don't like this sort of "deserialistaion" since: a)
It conflates the in-memory and on-disk representations, and b) It
usually results in questionable design decisions since you want to keep
the struct a fixed length to avoid having to parse the internal
fields. And I'm guessing that's why we ended up with a 1024 byte key
field in the secvar structure. From libstb/secvar/secvar.h:

	#define SECVAR_MAX_KEY_LEN              1024 

	...

	struct secvar {
        	uint64_t key_len;
	        uint64_t data_size;
        	char key[SECVAR_MAX_KEY_LEN];
	        char data[0];
	};

Not great.

There's a few other problems with this as well the biggest issue for me
is that since we plan to change skiboot to LE we're either going to
have to put in a pile of after-the-fact fix-up code to handle endian
conversion of the struct fields. I'd prefer we parsed out each field,
then did the fixup as we went since it's easier to verify that we're
doing the right thing when parsing the source data. You get similar
issues with changing the struct too.

In short, this sort of thing is almost always a maintenence headache.
The oddest thing about it is that you've already implemented a
serialised format (the one used for the PK in the TPM) that handles
this properly, so... just use that everywhere?

> +		list_add_tail(bank, &tmp->link);
> +		src += sizeof(struct secvar) + hdr->data_size;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +
> +/* Helper for the variable-bank specific writing logic */
> +static int secboot_tpm_write_variable_bank(struct list_head *bank)
> +{
> +	int rc;
> +	uint64_t bit;
> +
> +	bit = CYCLE_BIT(tpmnv_control_image->active_bit);
> +	rc = secboot_serialize_bank(bank, tpmnv_vars_image->vars,
> +				    tpmnv_vars_size - sizeof(struct tpmnv_vars),
> +				    SECVAR_FLAG_PROTECTED);
> +	if (rc)
> +		goto out;
> +
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_VARS_INDEX,
> +			     tpmnv_vars_image,
> +			     tpmnv_vars_size, 0);
> +	if (rc)
> +		goto out;
> +
> +	/* 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)
> +		goto out;
> +
> +	rc = calc_bank_hash(tpmnv_control_image->bank_hash[bit],
> +			    secboot_image->bank[bit],
> +			    SECBOOT_VARIABLE_BANK_SIZE);
> +	if (rc)
> +		goto out;
> +
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
> +			     tpmnv_control_image->bank_hash[bit],
> +			     SHA256_DIGEST_LENGTH,
> +			     offsetof(struct tpmnv_control, bank_hash[bit]));
> +	if (rc)
> +		goto out;
> +
> +	/* Write new variable bank to pnor */
> +	rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +	if (rc)
> +		goto out;
> +
> +	/* Flip the bit, and write to TPM NV */
> +	tpmnv_control_image->active_bit = bit;
> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
> +			     &tpmnv_control_image->active_bit,
> +			     sizeof(tpmnv_control_image->active_bit),
> +			     offsetof(struct tpmnv_control, active_bit));
> +out:
> +
> +	return rc;
> +}
> +
> +static int secboot_tpm_write_bank(struct list_head *bank, int section)
> +{
> +	int rc;
> +
> +	switch (section) {
> +	case SECVAR_VARIABLE_BANK:
> +		rc = secboot_tpm_write_variable_bank(bank);
> +		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;
> +}
> +
> +/* Priority variables stored in TPMNV have to be packed tighter to make the
> + *  most out of the small amount of space available */
> +static int secboot_tpm_load_from_tpmnv(struct list_head *bank)
> +{
> +	struct secvar *hdr;
> +	struct secvar_node *node;
> +	char *cur;
> +	char *end;
> +
> +	cur = tpmnv_vars_image->vars;
> +	end = ((char *) tpmnv_vars_image) + tpmnv_vars_size;
> +
> +	while (cur < end) {
> +		/* Ensure there is enough space to even check for another
> +		 *  var header */
> +		if ((end - cur) < offsetof(struct secvar, key))
> +			break;
> +
> +		/* Temporary cast to check sizes in the header */
> +		hdr = (struct secvar *) cur;
> +
> +		/* Check if we have a priority variable to load
> +		 * Should be zeroes if nonexistent */
> +		if ((hdr->key_len == 0) && (hdr->data_size == 0))
> +			break;
> +
> +		/* Sanity check our potential priority variables */
> +		if ((hdr->key_len > SECVAR_MAX_KEY_LEN)
> +		     || (hdr->data_size > SECBOOT_TPM_MAX_VAR_SIZE)) {
> +			prlog(PR_ERR, "TPM NV Priority variable has impossible sizes, probably internal bug. "
> +				      "len = %llu, size = %llu\n",
> +				      hdr->key_len, hdr->data_size);
> +			return OPAL_INTERNAL_ERROR;
> +		}
> +
> +		/* Advance cur over the two size values */
> +		cur += sizeof(hdr->key_len);
> +		cur += sizeof(hdr->data_size);
> +
> +		/* Ensure the expected key/data size doesn't exceed
> +		 *  the remaining buffer */
> +		if ((end - cur) < (hdr->data_size + hdr->key_len))
> +			return OPAL_INTERNAL_ERROR;
> +
> +		node = alloc_secvar(hdr->data_size);
> +		if (!node)
> +			return OPAL_NO_MEM;
> +
> +		node->var->key_len = hdr->key_len;
> +		node->var->data_size = hdr->data_size;
> +		node->flags |= SECVAR_FLAG_PROTECTED;
> +
> +		memcpy(node->var->key, cur, hdr->key_len);
> +		cur += hdr->key_len;
> +		memcpy(node->var->data, cur, hdr->data_size);
> +		cur += hdr->data_size;
> +
> +		list_add_tail(bank, &node->link);
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int secboot_tpm_load_variable_bank(struct list_head *bank)
> +{
> +	char bank_hash[SHA256_DIGEST_LENGTH];
> +	uint64_t bit = tpmnv_control_image->active_bit;
> +	int rc;
> +
> +	/* Check the hash of the bank we loaded from PNOR
> +	 *  versus the expected hash in TPM NV */
> +	rc = calc_bank_hash(bank_hash,
> +			    secboot_image->bank[bit],
> +			    SECBOOT_VARIABLE_BANK_SIZE);
> +	if (rc)
> +		return rc;
> +
> +	if (memcmp(bank_hash,
> +		   tpmnv_control_image->bank_hash[bit],
> +		   SHA256_DIGEST_LENGTH))
> +		/* Tampered pnor space detected, abandon ship */
> +		return OPAL_PERMISSION;
> +
> +	rc = secboot_tpm_load_from_tpmnv(bank);
> +	if (rc)
> +		return rc;
> +
> +	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);
> +	}
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +
> +/* Ensure the NV indices were defined with the correct set of attributes */
> +static int secboot_tpm_check_tpmnv_attrs(void)
> +{
> +	TPMS_NV_PUBLIC nv_public; /* Throwaway, we only want the name field */
> +	TPM2B_NAME nv_vars_name;
> +	TPM2B_NAME nv_control_name;
> +	int rc;
> +
> +	rc = tpmnv_ops.readpublic(SECBOOT_TPMNV_VARS_INDEX,
> +				  &nv_public,
> +				  &nv_vars_name);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to readpublic from the VARS index, rc=%d\n", rc);
> +		return rc;
> +	}
> +	rc = tpmnv_ops.readpublic(SECBOOT_TPMNV_CONTROL_INDEX,
> +				  &nv_public,
> +				  &nv_control_name);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to readpublic from the CONTROL index, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	if (memcmp(tpmnv_vars_name,
> +		   nv_vars_name.t.name,
> +		   sizeof(tpmnv_vars_name))) {
> +		prlog(PR_ERR, "VARS index not defined with the correct attributes\n");
> +		return OPAL_RESOURCE;
> +	}
> +	if (memcmp(tpmnv_control_name,
> +		   nv_control_name.t.name,
> +		   sizeof(tpmnv_control_name))) {
> +		prlog(PR_ERR, "CONTROL index not defined with the correct attributes\n");
> +		return OPAL_RESOURCE;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +
> +static int secboot_tpm_define_indices(void)
> +{
> +	int rc = OPAL_SUCCESS;
> +
> +	rc = tpmnv_ops.definespace(SECBOOT_TPMNV_VARS_INDEX, tpmnv_vars_size);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to define the VARS index, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = tpmnv_ops.definespace(SECBOOT_TPMNV_CONTROL_INDEX, sizeof(struct tpmnv_control));
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to define the CONTROL index, rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	rc = tpmnv_format();
> +	if (rc)
> +		return rc;
> +
> +	/* TPM NV just got redefined, so unconditionally format the SECBOOT partition */
> +	return secboot_format();
> +}
> +
> +static int secboot_tpm_store_init(void)
> +{
> +	int rc;
> +	unsigned int secboot_size;
> +
> +	TPMI_RH_NV_INDEX *indices = NULL;
> +	size_t count = 0;
> +	bool control_defined = false;
> +	bool vars_defined = false;
> +	int i;
> +
> +	if (secboot_image)
> +		return OPAL_SUCCESS;
> +
> +	if (!platform.secboot_info)
> +		return OPAL_UNSUPPORTED;
> +
> +	prlog(PR_DEBUG, "Initializing for pnor+tpm based platform\n");
> +
> +	/* Initialize SECBOOT first, we may need to format this later */
> +	rc = platform.secboot_info(&secboot_size);
> +	if (rc) {
> +		prlog(PR_ERR, "error %d retrieving keystore info\n", rc);
> +		goto error;
> +	}
> +	if (sizeof(struct secboot) > secboot_size) {
> +		prlog(PR_ERR, "secboot partition %d KB too small. min=%ld\n",
> +		      secboot_size >> 10, sizeof(struct secboot));
> +		rc = OPAL_RESOURCE;
> +		goto error;
> +	}
> +
> +	secboot_image = memalign(0x1000, sizeof(struct secboot));
> +	if (!secboot_image) {
> +		prlog(PR_ERR, "Failed to allocate space for the secboot image\n");
> +		rc = OPAL_NO_MEM;
> +		goto error;
> +	}
> +
> +	/* Read in the PNOR data, bank hash is checked on call to .load_bank() */
> +	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 error;
> +	}
> +
> +	/* Allocate the tpmnv data buffers */
> +	tpmnv_vars_image = zalloc(tpmnv_vars_size);
> +	if (!tpmnv_vars_image)
> +		return OPAL_NO_MEM;
> +	tpmnv_control_image = zalloc(sizeof(struct tpmnv_control));
> +	if (!tpmnv_control_image)
> +		return OPAL_NO_MEM;
> +
> +	/* Check if the NV indices have been defined already */
> +	rc = tpmnv_ops.getindices(&indices, &count);
> +	if (rc) {
> +		prlog(PR_ERR, "Could not load defined indicies from TPM, rc=%d\n", rc);
> +		goto error;
> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		if (indices[i] == SECBOOT_TPMNV_VARS_INDEX)
> +			vars_defined = true;
> +		else if (indices[i] == SECBOOT_TPMNV_CONTROL_INDEX)
> +			control_defined = true;
> +	}
> +	free(indices);
> +
> +	/* Undefine the NV indices if physical presence has been asserted */
> +	if (secvar_check_physical_presence()) {
> +		prlog(PR_INFO, "Physical presence asserted, redefining NV indices, and resetting keystore\n");
> +
> +		if (vars_defined) {
> +			rc = tpmnv_ops.undefinespace(SECBOOT_TPMNV_VARS_INDEX);
> +			if (rc) {
> +				prlog(PR_ERR, "Physical presence failed to undefine VARS, something is seriously wrong\n");
> +				goto error;
> +			}
> +		}
> +
> +		if (control_defined) {
> +			rc = tpmnv_ops.undefinespace(SECBOOT_TPMNV_CONTROL_INDEX);
> +			if (rc) {
> +				prlog(PR_ERR, "Physical presence failed to undefine CONTROL, something is seriously wrong\n");
> +				goto error;
> +			}
> +		}
> +
> +		vars_defined = control_defined = false;
> +	}
> +
> +	/* Determine if we need to define the indices. These should BOTH be false or true */
> +	if (!vars_defined && !control_defined) {
> +		rc = secboot_tpm_define_indices();
> +		if (rc)
> +			goto error;
> +
> +		/* Indicies got defined and formatted, we're done here */
> +		goto done;
> +	} else if (vars_defined ^ control_defined) {
> +		/* This should never happen. Both indices should be defined at the same
> +		 * time. Otherwise something seriously went wrong. */
> +		prlog(PR_ERR, "NV indices defined with unexpected attributes. Assert physical presence to clear\n");
> +		goto error;
> +	}
> +
> +	/* Ensure the NV indices were defined with the correct set of attributes */
> +	rc = secboot_tpm_check_tpmnv_attrs();
> +	if (rc)
> +		goto error;
> +
> +	/* TPMNV indices exist, are correct, and weren't just formatted, so read them in */
> +	rc = tpmnv_ops.read(SECBOOT_TPMNV_VARS_INDEX,
> +			    tpmnv_vars_image,
> +			    tpmnv_vars_size, 0);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to read from the VARS index\n");
> +		goto error;
> +	}
> +
> +	rc = tpmnv_ops.read(SECBOOT_TPMNV_CONTROL_INDEX,
> +			    tpmnv_control_image,
> +			    sizeof(struct tpmnv_control), 0);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to read from the CONTROL index\n");
> +		goto error;
> +	}
> +
> +	/* Verify the header information is correct */
> +	if (tpmnv_vars_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
> +	    tpmnv_control_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
> +	    tpmnv_vars_image->header.version != SECBOOT_VERSION ||
> +	    tpmnv_control_image->header.version != SECBOOT_VERSION) {
> +		prlog(PR_ERR, "TPMNV indices defined, but contain bad data. Assert physical presence to clear\n");
> +		goto error;
> +	}
> +
> +	/* Verify the secboot partition header information,
> +	 *  reformat if incorrect
> +	 * Note: Future variants should attempt to handle older versions safely
> +	 */
> +	if (secboot_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
> +	    secboot_image->header.version != SECBOOT_VERSION) {
> +		rc = secboot_format();
> +		if (rc)
> +			goto error;
> +	}
> +
> +done:
> +	return OPAL_SUCCESS;
> +
> +error:
> +	free(secboot_image);
> +	secboot_image = NULL;
> +	free(tpmnv_vars_image);
> +	tpmnv_vars_image = NULL;
> +	free(tpmnv_control_image);
> +	tpmnv_control_image = NULL;
> +
> +	return rc;
> +}
> +
> +
> +static void secboot_tpm_lock(void)
> +{
> +	/* Note: While write lock is called here on the two NV indices,
> +	 * both indices are also defined on the platform hierarchy.
> +	 * The platform hierarchy auth is set later in the skiboot
> +	 * initialization process, and not by any secvar-related code.
> +	 */
> +	int rc;
> +
> +	rc = tpmnv_ops.writelock(SECBOOT_TPMNV_VARS_INDEX);
> +	if (rc) {
> +		prlog(PR_EMERG, "TSS Write Lock failed on VARS index, halting.\n");
> +		abort();
> +	}
> +
> +	rc = tpmnv_ops.writelock(SECBOOT_TPMNV_CONTROL_INDEX);
> +	if (rc) {
> +		prlog(PR_EMERG, "TSS Write Lock failed on CONTROL index, halting.\n");
> +		abort();
> +	}
> +}
> +
> +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,
> +	.lock = secboot_tpm_lock,
> +	.max_var_size = SECBOOT_TPM_MAX_VAR_SIZE,
> +};
> diff --git a/libstb/secvar/storage/secboot_tpm.h b/libstb/secvar/storage/secboot_tpm.h
> new file mode 100644
> index 00000000..30a747a7
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm.h
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2020 IBM Corp. */
> +#ifndef _SECBOOT_TPM_H_
> +#define _SECBOOT_TPM_H_
> +
> +#include <ibmtss/tss.h>
> +
> +#define SECBOOT_VARIABLE_BANK_SIZE	32000
> +#define SECBOOT_UPDATE_BANK_SIZE	32000
> +
> +#define SECBOOT_VARIABLE_BANK_NUM	2
> +
> +/* Because mbedtls doesn't define this? */
> +#define SHA256_DIGEST_LENGTH	32
> +
> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
> +#define SECBOOT_MAGIC_NUMBER	0x5053424b
> +#define SECBOOT_VERSION		1
> +
> +#define SECBOOT_TPMNV_VARS_INDEX	0x01c10190
> +#define SECBOOT_TPMNV_CONTROL_INDEX	0x01c10191
> +
> +struct secboot_header {
> +	uint32_t magic_number;
> +	uint8_t version;
> +	uint8_t reserved[3];	/* Fix alignment */
> +} __attribute__((packed));
> +
> +struct secboot {
> +	struct secboot_header header;
> +	char bank[SECBOOT_VARIABLE_BANK_NUM][SECBOOT_VARIABLE_BANK_SIZE];
> +	char update[SECBOOT_UPDATE_BANK_SIZE];
> +} __attribute__((packed));
> +
> +struct tpmnv_vars {
> +	struct secboot_header header;
> +	char vars[0];
> +} __attribute__((packed));
> +
> +struct tpmnv_control {
> +	struct secboot_header header;
> +	uint8_t active_bit;
> +	char bank_hash[SECBOOT_VARIABLE_BANK_NUM][SHA256_DIGEST_LENGTH];
> +} __attribute__((packed));
> +
> +struct tpmnv_ops_s {
> +	int (*read)(TPMI_RH_NV_INDEX nv, void*, size_t, uint16_t);
> +	int (*write)(TPMI_RH_NV_INDEX nv, void*, size_t, uint16_t);
> +	int (*writelock)(TPMI_RH_NV_INDEX);
> +	int (*definespace)(TPMI_RH_NV_INDEX, uint16_t);
> +	int (*getindices)(TPMI_RH_NV_INDEX**, size_t*);
> +	int (*undefinespace)(TPMI_RH_NV_INDEX);
> +	int (*readpublic)(TPMI_RH_NV_INDEX, TPMS_NV_PUBLIC*, TPM2B_NAME*);
> +};
> +
> +extern struct tpmnv_ops_s tpmnv_ops;
> +
> +extern const uint8_t tpmnv_vars_name[];
> +extern const uint8_t tpmnv_control_name[];
> +
> +#endif
> diff --git a/libstb/secvar/storage/secboot_tpm_public_name.h b/libstb/secvar/storage/secboot_tpm_public_name.h
> new file mode 100644
> index 00000000..d0c7fdc1
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm_public_name.h
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
> +/* Copyright 2020 IBM Corp. */
> +#ifndef _SECBOOT_TPM_PUBLIC_NAME_H_
> +#define _SECBOOT_TPM_PUBLIC_NAME_H_
> +
> +const uint8_t tpmnv_vars_name[] = {
> +	0x00, 0x0b, 0x94, 0x64, 0x36, 0x25, 0xfc, 0xc1, 0x1d, 0xc1, 0x0e, 0x28, 0xe7,
> +	0xac, 0xaf, 0xc6, 0x08, 0x8e, 0xda, 0x21, 0xd6, 0x43, 0xd2, 0x77, 0xe7, 0x2d,
> +	0x83, 0x39, 0x0f, 0xa6, 0xdf, 0xc0, 0x59, 0x37,
> +};
> +
> +const uint8_t tpmnv_control_name[] = {
> +	0x00, 0x0b, 0xad, 0x47, 0x6b, 0xa5, 0xdf, 0xb1, 0xe2, 0x18, 0x50, 0xf6, 0x05,
> +	0x67, 0xe8, 0x8b, 0xa9, 0x0f, 0x86, 0x1f, 0x06, 0xab, 0x43, 0x96, 0x7f, 0x6e,
> +	0x85, 0x33, 0x5b, 0xa6, 0xf0, 0x63, 0x73, 0xd0,
> +};

This should be a .c file or the definitions moved into the file that
that includes this one.

On 5/14/20 10:21 PM, Oliver O'Halloran wrote:
> On Mon, 2020-05-11 at 16:31 -0500, 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>
>>
>> ---
>> V4:
>>  - add documentation for this driver
>>  - removed unused SECURE_STORAGE flag, renamed PRIORITY to PROTECTED
>>      This is still open to suggestions, PROTECTED sounded more clear)
>>  - fixed missing pointer advancement when writing protected variables
>>  - updated the NV public name hashes to match the current set of
>>    attributes used to define the indices
>>  - moved the expected NV public name hashes to seperate header
>>  - cleaned up most line lengths to under 80 columns, with the exception
>>      of prlog statements and possibly a few others.
>>
>>  doc/secvar/secboot_tpm.rst                    | 175 +++++
>>  include/secvar.h                              |   1 +
>>  libstb/secvar/secvar.h                        |   4 +-
>>  libstb/secvar/storage/Makefile.inc            |   5 +-
>>  libstb/secvar/storage/secboot_tpm.c           | 678 ++++++++++++++++++
>>  libstb/secvar/storage/secboot_tpm.h           |  61 ++
>>  .../secvar/storage/secboot_tpm_public_name.h  |  18 +
>>  libstb/secvar/storage/tpmnv_ops.c             |  15 +
>>  8 files changed, 953 insertions(+), 4 deletions(-)
>>  create mode 100644 doc/secvar/secboot_tpm.rst
>>  create mode 100644 libstb/secvar/storage/secboot_tpm.c
>>  create mode 100644 libstb/secvar/storage/secboot_tpm.h
>>  create mode 100644 libstb/secvar/storage/secboot_tpm_public_name.h
>>  create mode 100644 libstb/secvar/storage/tpmnv_ops.c
>>
>> diff --git a/doc/secvar/secboot_tpm.rst b/doc/secvar/secboot_tpm.rst
>> new file mode 100644
>> index 00000000..8da0c2f0
>> --- /dev/null
>> +++ b/doc/secvar/secboot_tpm.rst
>> @@ -0,0 +1,175 @@
>> +.. _secvar/secboot_tpm:
>> +
>> +secboot_tpm secvar storage driver for P9 platforms
>> +==================================================
>> +
>> +Overview
>> +--------
>> +
>> +This storage driver utilizes the SECBOOT PNOR partition and TPM NV space to
>> +persist secure variables across reboots in a tamper-resistant manner. While
>> +writes to PNOR cannot be completely prevented, writes CAN be prevented to TPM
>> +NV. On the other hand, there is limited available space in TPM NV.
>> +
>> +Therefore, this driver uses both in conjunction: large variable data is written
>> +to SECBOOT, and a hash of the variable data is stored in TPM NV. When the
>> +variables are loaded from SECBOOT, this hash is recalculated and compared
>> +against the value stored in the TPM. If they do not match, then the variables
>> +must have been altered and are not loaded.
>> +
>> +See the following sections for more information on the internals of the driver.
>> +
>> +
>> +Storage Layouts
>> +---------------
>> +
>> +At a high-level, there are a few major logical components:
>> +
>> + - (PNOR) Variable storage (split in half, active/staging)
>> + - (PNOR) Update storage
>> + - (TPM)  Protected variable storage
>> + - (TPM)  Bank hashes & active bit
>> +
>> +Variable storage consists of two smaller banks, variable bank 0 and variable
>> +bank 1. Either of the banks may be designated "active" by setting the active
>> +bank bit to either 0 or 1, indicating that the corresponding bank is now
>> +"active". The other bank is then considered "staging". See the "Persisting
>> +Variable Bank Updates" for more on the active/staging bank logic.
>> +
>> +Protected variable storage is stored in ``VARS`` TPM NV index. Unlike the other
>> +variable storage, there is only one bank due to limited storage space. See the
>> +TPM NV Indices section for more.
>> +
>> +
>> +Persisting the Variable Bank
>> +----------------------------
>> +
>> +When writing a new variable bank to storage, this is (roughly) the procedure the
>> +driver will follow:
>> +
>> +1. write variables to the staging bank
>> +2. calculate hash of the staging bank
>> +3. store the staging bank hash in the TPM NV
>> +4. flip the active bank bit
>> +
>> +This procedure ensures that the switch-over from the old variables to the
>> +new variables is as atomic as possible. This should prevent any possible
>> +issues caused by an interruption during the writing process, such as power loss.
>> +
>> +The bank hashes are a SHA256 hash calculated over the whole region of
>> +storage space allocated to the bank, including unused storage. For consistency,
>> +unused space is always written as zeroes. Like the active/staging variable
>> +banks, there are also two corresponding active/staging bank hashes stored in
>> +the TPM.
>> +
>> +
>> +TPM NV Indices
>> +--------------
>> +
>> +The driver utilizes two TPM NV indices:
>> +
>> +.. code-block:: c
>> +
>> +  # size). datadefine SECBOOT_TPMNV_VARS_INDEX	0x01c10190
>> +  #define SECBOOT_TPMNV_CONTROL_INDEX	0x01c10191
>> +
>> +The ``VARS`` index stores variables flagged with ``SECVAR_FLAG_PROTECTED``.
>> +These variables are critical to the state of OS secure boot, and therefore
>> +cannot be safely stored in the SECBOOT partition. This index is defined to be
>> +1024 bytes in size, which is enough for the current implementation on P9. It
>> +is kept small by default to preserve the very limited NV index space.
>> +
>> +The ``CONTROL`` index stores the bank hashes, and the bit to determine which
>> +bank is active. See the Active/Staging Bank Swapping section for more.
>> +
>> +Both indices are defined on first boot with the same set of attributes. If the
>> +indices are already defined but not in the expected state, (different
>> +attributes, size, etc), then the driver will halt the boot. Asserting physical
>> +presence will redefine the indices in the correct state.
>> +
>> +
>> +Locking
>> +-------
>> +
>> +PNOR cannot be locked, however the TPM can be. The TPM NV indices are double
>> +protected via two locking mechanisms:
>> +
>> + - The driver's ``.lock()`` hook sends the ``TSS_NV_WriteLock`` TPM command.
>> +This sets the ``WRITELOCKED`` attribute, which is cleared on the next
>> +TPM reset.
>> +
>> + - The TPM NV indices are defined under the platform hierarchy. Skiboot will add
>> +a global lock to all the NV indices under this hierarchy prior to loading a
>> +kernel. This is also reset on the next TPM reset.
>> +
>> +NOTE: The TPM is only reset during a cold reboot. Fast reboots or kexecs will
>> +NOT unlock the TPM.
>> +
>> +
>> +Resetting Storage / Physical Presence
>> +-------------------------------------
>> +
>> +In the case that secure boot/secvar has been rendered unusable, (for example:
>> +corrupted data, lost/compromised private key, improperly defined NV indices, etc)
>> +this storage driver responds to physical presence assertion as a last-resort
>> +method to recover the system.
>> +
>> +Asserting physical presence undefines, and immediately redefines the TPM NV
>> +indices. Defining the NV indices then causes a cascading set of reformats for
>> +the remaining components of storage, similar to a first-boot scenario.
>> +
>> +This driver considers physical presence to be asserted if any of the following
>> +device tree nodes are present in ``ibm,secureboot``:
>> + - ``clear-os-keys``
>> + - ``clear-all-keys``
>> + - ``clear-mfg-keys``
> 
> what does clear-mfg-keys do?
> 
> 

Set by manufacturing to clear data, functionally the same as clear-all-keys.

We'll add a seperate document explaining the physical presence device tree nodes.

>> +Storage Formats/Layouts
>> +=======================
>> +
>> +SECBOOT (PNOR)
>> +--------------
>> +
>> +Partition Format:
>> + - 8b secboot header
>> +   - 4b: u32. magic number, always 0x5053424b
>> +   - 1b: u8. version, always 1
>> +   - 3b: unused padding
> 
> This is slightly awkward since the 8 byte header bumps us past the
> flash sector alignment boundary (usually 4k) so you end up with some
> wasted space.

Would it be better to be add padding to align the header to a whole 4k sector?

> 
>> + - 32k: secvars. variable bank 0
>> + - 32k: secvars. variable bank 1
>> + - 32k: secvars. update bank
> 
> Why are the bank sizes fixed?
> 

Largely simplicity in design. The sections are located at at known fixed offsets from the beginning of the partition. We don't expect banks to be resized, on the other hand it could potentially be set by the platform if a future platform happens to have more space.

>> +Variable Format (secvar):
>> + - 8b: u64. key length
>> + - 8b: u64. data size
>> + - 1k: string. key
>> + - (data size). data
> 
> 1) Why is the key size hardcoded to 1k?  It seems like a waste of our
> limited storage space. Especially when you consider that most of the
> key names that we define are only a few bytes long. (NB: there's more
> on this further down).

It is a waste, will address this in the next set which will also rework the secvar serialize/deserialize logic.

> > 2) Any multi-byte integers written to storage needs to have a defined
> endianness. We do plan to switch skiboot to LE at some point so this
> isn't an academic problem either.
> 

Will switch the on-disk format to be always BE, with swaps to native where appropriate.

>> +TPM VARS (NV)
>> +-------------
>> +
>> +NV Index Format:
>> + - 8b secboot header
>> +   - 4b: u32. magic number, always 0x5053424b
>> +   - 1b: u8. version, always 1
>> +   - 3b: unused padding
>> + - 1016b: packed secvars. protected variable storage
> 
> Where does the 1K size limit come from?
> 

The NV index is defined to be 1K bytes. According to the TPM spec, there is a minimum of ~7K bytes of NV space available, some/most of should still be made available to the end user. The PK is only about 800~900 bytes, so we're using the largest minimum we determined to be safe.

I'll update the document to include this reasoning in the NV section.

>> +Variable Format (packed secvar):
>> + - 8b: u64. key length
>> + - 8b: u64. data size
>> + - (key length): string. key
>> + - (data size). data
>> +
>> +TPM CONTROL (NV)
>> +----------------
>> +
>> + - 8b secboot header
>> +   - 4b: u32. magic number, always 0x5053424b
>> +   - 1b: u8. version, always 1
>> +   - 3b: unused padding
>> + - 1b: u8. active bit, 0 or 1
>> + - 32b: sha256 hash of variable bank 0
>> + - 32b: sha256 hash of variable bank 1
> 
> As a general comment these structures being completely fixed means we
> have a flag day if they need to change for any reason at all. In
> general customers expect to be able to do a switch between version N
> and version N-1 without issues since that is sometimes required to work
> around problems. This makes me a bit nervous.
> 

I agree that the fixed structs are restrictive regarding upgrade/downgrade potential, would it make sense to include offsets as values in the header?

As a side point however, how freely should an end-user be able to downgrade when in secure mode? If we allow it without manual intervention, it could lead to downgrade attacks.

>> diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
>> index b141b705..7e23dcc3 100644
>> --- a/libstb/secvar/secvar.h
>> +++ b/libstb/secvar/secvar.h
>> @@ -23,8 +23,8 @@ struct secvar_node {
>>  	uint64_t size;		// How much space was allocated for data
>>  };
>>  
>> -#define SECVAR_FLAG_VOLATILE		0x1 // Instructs storage driver to ignore variable on writes
>> -#define SECVAR_FLAG_SECURE_STORAGE	0x2 // Hint for storage driver to select storage location
>> +#define SECVAR_FLAG_VOLATILE	0x1 // Instructs storage driver to ignore variable on writes
>> +#define SECVAR_FLAG_PROTECTED	0x2 // Hint for storage driver to store in lockable flash
> 
> Hint or requirement? Also, comment style.
> 

Suggestion, more like. This is a special case for P9, where we use the TPM for secure storage, but have a limited amount. A future platform with larger amounts of lockable flash may choose to ignore this, as all variables are stored in a protected manner.

Will tweak the wording to make that more clear.

>  
>> diff --git a/libstb/secvar/storage/secboot_tpm.c b/libstb/secvar/storage/secboot_tpm.c
>> new file mode 100644
>> index 00000000..012da294
>> --- /dev/null
>> +++ b/libstb/secvar/storage/secboot_tpm.c
>> @@ -0,0 +1,678 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +/* Copyright 2020 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_devtree.h"
>> +#include "secboot_tpm.h"
>> +#include <tssskiboot.h>
>> +#include <ibmtss/TPM_Types.h>
>> +
>> +#define CYCLE_BIT(b) (b^0x1)
>> +
>> +#define SECBOOT_TPM_MAX_VAR_SIZE	8192
>> +
>> +struct secboot *secboot_image = NULL;
>> +struct tpmnv_vars *tpmnv_vars_image = NULL;
>> +struct tpmnv_control *tpmnv_control_image = NULL;
>> +
>> +const size_t tpmnv_vars_size = 1024;
>> +
>> +/* Expected TPM NV index name field from NV_ReadPublic given our known
>> + * set of attributes.
>> + * See Part 1 Section 16, and Part 2 Section 13.5 of the TPM Specification
>> + * for how this is calculated
>> + */
>> +#include "secboot_tpm_public_name.h"
>> +
>> +/* Calculate a SHA256 hash over the supplied buffer */
>> +static int calc_bank_hash(char *target_hash, char *source_buf, uint64_t size)
>> +{
>> +	mbedtls_sha256_context ctx;
>> +	int rc;
>> +
>> +	mbedtls_sha256_init(&ctx);
>> +
>> +	rc = mbedtls_sha256_update_ret(&ctx, source_buf, size);
>> +	if (rc)
>> +		goto out;
>> +
>> +	mbedtls_sha256_finish_ret(&ctx, target_hash);
>> +	if (rc)
>> +		goto out;
>> +
>> +out:
>> +	mbedtls_sha256_free(&ctx);
>> +	return rc;
>> +}
>> +
>> +/* Reformat the TPMNV space */
>> +static int tpmnv_format(void)
>> +{
>> +	int rc;
>> +
>> +	memset(tpmnv_vars_image, 0x00, tpmnv_vars_size);
>> +	memset(tpmnv_control_image, 0x00, sizeof(struct tpmnv_control));
>> +
>> +	tpmnv_vars_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
>> +	tpmnv_vars_image->header.version = SECBOOT_VERSION;
>> +	tpmnv_control_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
>> +	tpmnv_control_image->header.version = SECBOOT_VERSION;
>> +
>> +	/* Counts as first write to the TPM NV, which sets the
>> +	 *  TPMA_NVA_WRITTEN attribute */
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_VARS_INDEX,
>> +			     tpmnv_vars_image,
>> +			     tpmnv_vars_size, 0);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Could not write new formatted data to VARS index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
>> +			     tpmnv_control_image,
>> +			     sizeof(struct tpmnv_control), 0);
>> +	if (rc)
>> +		prlog(PR_ERR, "Could not write new formatted data to CONTROL index, rc=%d\n", rc);
>> +
>> +	return rc;
>> +}
>> +
>> +/* Reformat the secboot PNOR space */
>> +static int secboot_format(void)
>> +{
>> +	int rc;
>> +
>> +	memset(secboot_image, 0x00, sizeof(struct secboot));
>> +
>> +	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
>> +	secboot_image->header.version = SECBOOT_VERSION;
>> +
>> +	/* Write the hash of the empty bank to the tpm so future loads work */
>> +	rc = calc_bank_hash(tpmnv_control_image->bank_hash[0],
>> +			    secboot_image->bank[0],
>> +			    SECBOOT_VARIABLE_BANK_SIZE);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Bank hash failed to calculate somehow\n");
>> +		return rc;
>> +	}
>> +
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
>> +			     tpmnv_control_image->bank_hash[0],
>> +			     SHA256_DIGEST_SIZE,
>> +			     offsetof(struct tpmnv_control,
>> +			     bank_hash[0]));
>> +	if (rc) {
>> +		prlog(PR_ERR, "Could not write fresh formatted bank hashes to CONTROL index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
>> +	if (rc)
>> +		prlog(PR_ERR, "Could not write formatted data to PNOR, rc=%d\n", rc);
>> +
>> +	return rc;
>> +}
>> +
>> +
>> +/* Serialize one priority variable using a tighter packing scheme
>> + * Returns the advanced target pointer */
>> +static char *secboot_serialize_priority(char *target, struct secvar_node *node, char *end)
>> +{
>> +	if ((target +
>> +	     node->var->key_len +
>> +	     node->var->data_size +
>> +	     offsetof(struct secvar, key)) > end)
>> +		return NULL;
>> +
>> +	memcpy(target, &node->var->key_len, sizeof(node->var->key_len));
>> +	target += sizeof(node->var->key_len);
>> +	memcpy(target, &node->var->data_size, sizeof(node->var->data_size));
>> +	target += sizeof(node->var->data_size);
>> +	memcpy(target, node->var->key, node->var->key_len);
>> +	target += node->var->key_len;
>> +	memcpy(target, node->var->data, node->var->data_size);
>> +	target += node->var->data_size;
>> +
>> +	return target;
>> +}
>> +
>> +
>> +/* 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;
>> +	char *end = target + target_size;
>> +
>> +	if (!bank)
>> +		return OPAL_INTERNAL_ERROR;
>> +	if (!target)
>> +		return OPAL_INTERNAL_ERROR;
>> +
>> +	memset(target, 0x00, target_size);
>> +
>> +	list_for_each(bank, node, link) {
>> +		if (node->flags != flags)
>> +			continue;
>> +
>> +		/* Priority variable has a different packing scheme */
>> +		if (flags & SECVAR_FLAG_PROTECTED) {
>> +			target = secboot_serialize_priority(target, node, end);
>> +			if (!target)
>> +				return OPAL_EMPTY;
>> +			continue;
>> +		}
>> +
>> +		/* Bail early if we are out of storage space */
>> +		if ((target - tmp) +
>> +		    sizeof(struct secvar) +
>> +		    node->var->data_size > target_size) {
>> +			prlog(PR_ERR, "Ran out of PNOR space, giving up!\n");
>> +			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;
>> +}
>> +
>> +/* Loads in a flattened list of variables from a buffer into a linked list */
>> +static int secboot_load_from_pnor(struct list_head *bank, char *source, size_t max_size)
> source should be const, and probably void.
> 
>> +{
>> +	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.
>> +		 *
>> +		 * Banks are zeroized after each write, thus key_len == 0
>> +		 *  indicates end of the list */
>> +		hdr = (struct secvar *) src;
>> +		if (hdr->key_len == 0) {
>> +			break;
>> +		} else if (hdr->key_len > SECVAR_MAX_KEY_LEN) {
>> +			prlog(PR_ERR, "Attempted to load a key larger than max, len = %llu\n", hdr->key_len);
>> +			return OPAL_INTERNAL_ERROR;
>> +		}
>> +
>> +		if (hdr->data_size > SECBOOT_TPM_MAX_VAR_SIZE) {
>> +			prlog(PR_ERR, "Attempted to load a data payload larger than max, "
>> +				      "size = %llu\n", hdr->data_size);
>> +			return OPAL_INTERNAL_ERROR;
>> +		}
>> +
>> +		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;
> Shouldn't this free all the variables that we did manage to allocate?
> 
>> +		}
>> +
>> +		memcpy(tmp->var, src, sizeof(struct secvar) + hdr->data_size);
> 
> As a general rule I don't like this sort of "deserialistaion" since: a)
> It conflates the in-memory and on-disk representations, and b) It
> usually results in questionable design decisions since you want to keep
> the struct a fixed length to avoid having to parse the internal
> fields. And I'm guessing that's why we ended up with a 1024 byte key
> field in the secvar structure. From libstb/secvar/secvar.h:
> 
> 	#define SECVAR_MAX_KEY_LEN              1024 
> 
> 	...
> 
> 	struct secvar {
>         	uint64_t key_len;
> 	        uint64_t data_size;
>         	char key[SECVAR_MAX_KEY_LEN];
> 	        char data[0];
> 	};
> 
> Not great.
> 
> There's a few other problems with this as well the biggest issue for me
> is that since we plan to change skiboot to LE we're either going to
> have to put in a pile of after-the-fact fix-up code to handle endian
> conversion of the struct fields. I'd prefer we parsed out each field,
> then did the fixup as we went since it's easier to verify that we're
> doing the right thing when parsing the source data. You get similar
> issues with changing the struct too.
> 
> In short, this sort of thing is almost always a maintenence headache.
> The oddest thing about it is that you've already implemented a
> serialised format (the one used for the PK in the TPM) that handles
> this properly, so... just use that everywhere?
> 

Makes sense, I will change this, and also add in endian conversions in the serialize/deserialize logic while I'm at it.

>> +		list_add_tail(bank, &tmp->link);
>> +		src += sizeof(struct secvar) + hdr->data_size;
>> +	}
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +
>> +/* Helper for the variable-bank specific writing logic */
>> +static int secboot_tpm_write_variable_bank(struct list_head *bank)
>> +{
>> +	int rc;
>> +	uint64_t bit;
>> +
>> +	bit = CYCLE_BIT(tpmnv_control_image->active_bit);
>> +	rc = secboot_serialize_bank(bank, tpmnv_vars_image->vars,
>> +				    tpmnv_vars_size - sizeof(struct tpmnv_vars),
>> +				    SECVAR_FLAG_PROTECTED);
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_VARS_INDEX,
>> +			     tpmnv_vars_image,
>> +			     tpmnv_vars_size, 0);
>> +	if (rc)
>> +		goto out;
>> +
>> +	/* 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)
>> +		goto out;
>> +
>> +	rc = calc_bank_hash(tpmnv_control_image->bank_hash[bit],
>> +			    secboot_image->bank[bit],
>> +			    SECBOOT_VARIABLE_BANK_SIZE);
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
>> +			     tpmnv_control_image->bank_hash[bit],
>> +			     SHA256_DIGEST_LENGTH,
>> +			     offsetof(struct tpmnv_control, bank_hash[bit]));
>> +	if (rc)
>> +		goto out;
>> +
>> +	/* Write new variable bank to pnor */
>> +	rc = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
>> +	if (rc)
>> +		goto out;
>> +
>> +	/* Flip the bit, and write to TPM NV */
>> +	tpmnv_control_image->active_bit = bit;
>> +	rc = tpmnv_ops.write(SECBOOT_TPMNV_CONTROL_INDEX,
>> +			     &tpmnv_control_image->active_bit,
>> +			     sizeof(tpmnv_control_image->active_bit),
>> +			     offsetof(struct tpmnv_control, active_bit));
>> +out:
>> +
>> +	return rc;
>> +}
>> +
>> +static int secboot_tpm_write_bank(struct list_head *bank, int section)
>> +{
>> +	int rc;
>> +
>> +	switch (section) {
>> +	case SECVAR_VARIABLE_BANK:
>> +		rc = secboot_tpm_write_variable_bank(bank);
>> +		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;
>> +}
>> +
>> +/* Priority variables stored in TPMNV have to be packed tighter to make the
>> + *  most out of the small amount of space available */
>> +static int secboot_tpm_load_from_tpmnv(struct list_head *bank)
>> +{
>> +	struct secvar *hdr;
>> +	struct secvar_node *node;
>> +	char *cur;
>> +	char *end;
>> +
>> +	cur = tpmnv_vars_image->vars;
>> +	end = ((char *) tpmnv_vars_image) + tpmnv_vars_size;
>> +
>> +	while (cur < end) {
>> +		/* Ensure there is enough space to even check for another
>> +		 *  var header */
>> +		if ((end - cur) < offsetof(struct secvar, key))
>> +			break;
>> +
>> +		/* Temporary cast to check sizes in the header */
>> +		hdr = (struct secvar *) cur;
>> +
>> +		/* Check if we have a priority variable to load
>> +		 * Should be zeroes if nonexistent */
>> +		if ((hdr->key_len == 0) && (hdr->data_size == 0))
>> +			break;
>> +
>> +		/* Sanity check our potential priority variables */
>> +		if ((hdr->key_len > SECVAR_MAX_KEY_LEN)
>> +		     || (hdr->data_size > SECBOOT_TPM_MAX_VAR_SIZE)) {
>> +			prlog(PR_ERR, "TPM NV Priority variable has impossible sizes, probably internal bug. "
>> +				      "len = %llu, size = %llu\n",
>> +				      hdr->key_len, hdr->data_size);
>> +			return OPAL_INTERNAL_ERROR;
>> +		}
>> +
>> +		/* Advance cur over the two size values */
>> +		cur += sizeof(hdr->key_len);
>> +		cur += sizeof(hdr->data_size);
>> +
>> +		/* Ensure the expected key/data size doesn't exceed
>> +		 *  the remaining buffer */
>> +		if ((end - cur) < (hdr->data_size + hdr->key_len))
>> +			return OPAL_INTERNAL_ERROR;
>> +
>> +		node = alloc_secvar(hdr->data_size);
>> +		if (!node)
>> +			return OPAL_NO_MEM;
>> +
>> +		node->var->key_len = hdr->key_len;
>> +		node->var->data_size = hdr->data_size;
>> +		node->flags |= SECVAR_FLAG_PROTECTED;
>> +
>> +		memcpy(node->var->key, cur, hdr->key_len);
>> +		cur += hdr->key_len;
>> +		memcpy(node->var->data, cur, hdr->data_size);
>> +		cur += hdr->data_size;
>> +
>> +		list_add_tail(bank, &node->link);
>> +	}
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +static int secboot_tpm_load_variable_bank(struct list_head *bank)
>> +{
>> +	char bank_hash[SHA256_DIGEST_LENGTH];
>> +	uint64_t bit = tpmnv_control_image->active_bit;
>> +	int rc;
>> +
>> +	/* Check the hash of the bank we loaded from PNOR
>> +	 *  versus the expected hash in TPM NV */
>> +	rc = calc_bank_hash(bank_hash,
>> +			    secboot_image->bank[bit],
>> +			    SECBOOT_VARIABLE_BANK_SIZE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (memcmp(bank_hash,
>> +		   tpmnv_control_image->bank_hash[bit],
>> +		   SHA256_DIGEST_LENGTH))
>> +		/* Tampered pnor space detected, abandon ship */
>> +		return OPAL_PERMISSION;
>> +
>> +	rc = secboot_tpm_load_from_tpmnv(bank);
>> +	if (rc)
>> +		return rc;
>> +
>> +	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);
>> +	}
>> +
>> +	return OPAL_HARDWARE;
>> +}
>> +
>> +
>> +/* Ensure the NV indices were defined with the correct set of attributes */
>> +static int secboot_tpm_check_tpmnv_attrs(void)
>> +{
>> +	TPMS_NV_PUBLIC nv_public; /* Throwaway, we only want the name field */
>> +	TPM2B_NAME nv_vars_name;
>> +	TPM2B_NAME nv_control_name;
>> +	int rc;
>> +
>> +	rc = tpmnv_ops.readpublic(SECBOOT_TPMNV_VARS_INDEX,
>> +				  &nv_public,
>> +				  &nv_vars_name);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to readpublic from the VARS index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +	rc = tpmnv_ops.readpublic(SECBOOT_TPMNV_CONTROL_INDEX,
>> +				  &nv_public,
>> +				  &nv_control_name);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to readpublic from the CONTROL index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	if (memcmp(tpmnv_vars_name,
>> +		   nv_vars_name.t.name,
>> +		   sizeof(tpmnv_vars_name))) {
>> +		prlog(PR_ERR, "VARS index not defined with the correct attributes\n");
>> +		return OPAL_RESOURCE;
>> +	}
>> +	if (memcmp(tpmnv_control_name,
>> +		   nv_control_name.t.name,
>> +		   sizeof(tpmnv_control_name))) {
>> +		prlog(PR_ERR, "CONTROL index not defined with the correct attributes\n");
>> +		return OPAL_RESOURCE;
>> +	}
>> +
>> +	return OPAL_SUCCESS;
>> +}
>> +
>> +
>> +static int secboot_tpm_define_indices(void)
>> +{
>> +	int rc = OPAL_SUCCESS;
>> +
>> +	rc = tpmnv_ops.definespace(SECBOOT_TPMNV_VARS_INDEX, tpmnv_vars_size);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to define the VARS index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = tpmnv_ops.definespace(SECBOOT_TPMNV_CONTROL_INDEX, sizeof(struct tpmnv_control));
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to define the CONTROL index, rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	rc = tpmnv_format();
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* TPM NV just got redefined, so unconditionally format the SECBOOT partition */
>> +	return secboot_format();
>> +}
>> +
>> +static int secboot_tpm_store_init(void)
>> +{
>> +	int rc;
>> +	unsigned int secboot_size;
>> +
>> +	TPMI_RH_NV_INDEX *indices = NULL;
>> +	size_t count = 0;
>> +	bool control_defined = false;
>> +	bool vars_defined = false;
>> +	int i;
>> +
>> +	if (secboot_image)
>> +		return OPAL_SUCCESS;
>> +
>> +	if (!platform.secboot_info)
>> +		return OPAL_UNSUPPORTED;
>> +
>> +	prlog(PR_DEBUG, "Initializing for pnor+tpm based platform\n");
>> +
>> +	/* Initialize SECBOOT first, we may need to format this later */
>> +	rc = platform.secboot_info(&secboot_size);
>> +	if (rc) {
>> +		prlog(PR_ERR, "error %d retrieving keystore info\n", rc);
>> +		goto error;
>> +	}
>> +	if (sizeof(struct secboot) > secboot_size) {
>> +		prlog(PR_ERR, "secboot partition %d KB too small. min=%ld\n",
>> +		      secboot_size >> 10, sizeof(struct secboot));
>> +		rc = OPAL_RESOURCE;
>> +		goto error;
>> +	}
>> +
>> +	secboot_image = memalign(0x1000, sizeof(struct secboot));
>> +	if (!secboot_image) {
>> +		prlog(PR_ERR, "Failed to allocate space for the secboot image\n");
>> +		rc = OPAL_NO_MEM;
>> +		goto error;
>> +	}
>> +
>> +	/* Read in the PNOR data, bank hash is checked on call to .load_bank() */
>> +	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 error;
>> +	}
>> +
>> +	/* Allocate the tpmnv data buffers */
>> +	tpmnv_vars_image = zalloc(tpmnv_vars_size);
>> +	if (!tpmnv_vars_image)
>> +		return OPAL_NO_MEM;
>> +	tpmnv_control_image = zalloc(sizeof(struct tpmnv_control));
>> +	if (!tpmnv_control_image)
>> +		return OPAL_NO_MEM;
>> +
>> +	/* Check if the NV indices have been defined already */
>> +	rc = tpmnv_ops.getindices(&indices, &count);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Could not load defined indicies from TPM, rc=%d\n", rc);
>> +		goto error;
>> +	}
>> +
>> +	for (i = 0; i < count; i++) {
>> +		if (indices[i] == SECBOOT_TPMNV_VARS_INDEX)
>> +			vars_defined = true;
>> +		else if (indices[i] == SECBOOT_TPMNV_CONTROL_INDEX)
>> +			control_defined = true;
>> +	}
>> +	free(indices);
>> +
>> +	/* Undefine the NV indices if physical presence has been asserted */
>> +	if (secvar_check_physical_presence()) {
>> +		prlog(PR_INFO, "Physical presence asserted, redefining NV indices, and resetting keystore\n");
>> +
>> +		if (vars_defined) {
>> +			rc = tpmnv_ops.undefinespace(SECBOOT_TPMNV_VARS_INDEX);
>> +			if (rc) {
>> +				prlog(PR_ERR, "Physical presence failed to undefine VARS, something is seriously wrong\n");
>> +				goto error;
>> +			}
>> +		}
>> +
>> +		if (control_defined) {
>> +			rc = tpmnv_ops.undefinespace(SECBOOT_TPMNV_CONTROL_INDEX);
>> +			if (rc) {
>> +				prlog(PR_ERR, "Physical presence failed to undefine CONTROL, something is seriously wrong\n");
>> +				goto error;
>> +			}
>> +		}
>> +
>> +		vars_defined = control_defined = false;
>> +	}
>> +
>> +	/* Determine if we need to define the indices. These should BOTH be false or true */
>> +	if (!vars_defined && !control_defined) {
>> +		rc = secboot_tpm_define_indices();
>> +		if (rc)
>> +			goto error;
>> +
>> +		/* Indicies got defined and formatted, we're done here */
>> +		goto done;
>> +	} else if (vars_defined ^ control_defined) {
>> +		/* This should never happen. Both indices should be defined at the same
>> +		 * time. Otherwise something seriously went wrong. */
>> +		prlog(PR_ERR, "NV indices defined with unexpected attributes. Assert physical presence to clear\n");
>> +		goto error;
>> +	}
>> +
>> +	/* Ensure the NV indices were defined with the correct set of attributes */
>> +	rc = secboot_tpm_check_tpmnv_attrs();
>> +	if (rc)
>> +		goto error;
>> +
>> +	/* TPMNV indices exist, are correct, and weren't just formatted, so read them in */
>> +	rc = tpmnv_ops.read(SECBOOT_TPMNV_VARS_INDEX,
>> +			    tpmnv_vars_image,
>> +			    tpmnv_vars_size, 0);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to read from the VARS index\n");
>> +		goto error;
>> +	}
>> +
>> +	rc = tpmnv_ops.read(SECBOOT_TPMNV_CONTROL_INDEX,
>> +			    tpmnv_control_image,
>> +			    sizeof(struct tpmnv_control), 0);
>> +	if (rc) {
>> +		prlog(PR_ERR, "Failed to read from the CONTROL index\n");
>> +		goto error;
>> +	}
>> +
>> +	/* Verify the header information is correct */
>> +	if (tpmnv_vars_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
>> +	    tpmnv_control_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
>> +	    tpmnv_vars_image->header.version != SECBOOT_VERSION ||
>> +	    tpmnv_control_image->header.version != SECBOOT_VERSION) {
>> +		prlog(PR_ERR, "TPMNV indices defined, but contain bad data. Assert physical presence to clear\n");
>> +		goto error;
>> +	}
>> +
>> +	/* Verify the secboot partition header information,
>> +	 *  reformat if incorrect
>> +	 * Note: Future variants should attempt to handle older versions safely
>> +	 */
>> +	if (secboot_image->header.magic_number != SECBOOT_MAGIC_NUMBER ||
>> +	    secboot_image->header.version != SECBOOT_VERSION) {
>> +		rc = secboot_format();
>> +		if (rc)
>> +			goto error;
>> +	}
>> +
>> +done:
>> +	return OPAL_SUCCESS;
>> +
>> +error:
>> +	free(secboot_image);
>> +	secboot_image = NULL;
>> +	free(tpmnv_vars_image);
>> +	tpmnv_vars_image = NULL;
>> +	free(tpmnv_control_image);
>> +	tpmnv_control_image = NULL;
>> +
>> +	return rc;
>> +}
>> +
>> +
>> +static void secboot_tpm_lock(void)
>> +{
>> +	/* Note: While write lock is called here on the two NV indices,
>> +	 * both indices are also defined on the platform hierarchy.
>> +	 * The platform hierarchy auth is set later in the skiboot
>> +	 * initialization process, and not by any secvar-related code.
>> +	 */
>> +	int rc;
>> +
>> +	rc = tpmnv_ops.writelock(SECBOOT_TPMNV_VARS_INDEX);
>> +	if (rc) {
>> +		prlog(PR_EMERG, "TSS Write Lock failed on VARS index, halting.\n");
>> +		abort();
>> +	}
>> +
>> +	rc = tpmnv_ops.writelock(SECBOOT_TPMNV_CONTROL_INDEX);
>> +	if (rc) {
>> +		prlog(PR_EMERG, "TSS Write Lock failed on CONTROL index, halting.\n");
>> +		abort();
>> +	}
>> +}
>> +
>> +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,
>> +	.lock = secboot_tpm_lock,
>> +	.max_var_size = SECBOOT_TPM_MAX_VAR_SIZE,
>> +};
>> diff --git a/libstb/secvar/storage/secboot_tpm.h b/libstb/secvar/storage/secboot_tpm.h
>> new file mode 100644
>> index 00000000..30a747a7
>> --- /dev/null
>> +++ b/libstb/secvar/storage/secboot_tpm.h
>> @@ -0,0 +1,61 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +/* Copyright 2020 IBM Corp. */
>> +#ifndef _SECBOOT_TPM_H_
>> +#define _SECBOOT_TPM_H_
>> +
>> +#include <ibmtss/tss.h>
>> +
>> +#define SECBOOT_VARIABLE_BANK_SIZE	32000
>> +#define SECBOOT_UPDATE_BANK_SIZE	32000
>> +
>> +#define SECBOOT_VARIABLE_BANK_NUM	2
>> +
>> +/* Because mbedtls doesn't define this? */
>> +#define SHA256_DIGEST_LENGTH	32
>> +
>> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
>> +#define SECBOOT_MAGIC_NUMBER	0x5053424b
>> +#define SECBOOT_VERSION		1
>> +
>> +#define SECBOOT_TPMNV_VARS_INDEX	0x01c10190
>> +#define SECBOOT_TPMNV_CONTROL_INDEX	0x01c10191
>> +
>> +struct secboot_header {
>> +	uint32_t magic_number;
>> +	uint8_t version;
>> +	uint8_t reserved[3];	/* Fix alignment */
>> +} __attribute__((packed));
>> +
>> +struct secboot {
>> +	struct secboot_header header;
>> +	char bank[SECBOOT_VARIABLE_BANK_NUM][SECBOOT_VARIABLE_BANK_SIZE];
>> +	char update[SECBOOT_UPDATE_BANK_SIZE];
>> +} __attribute__((packed));
>> +
>> +struct tpmnv_vars {
>> +	struct secboot_header header;
>> +	char vars[0];
>> +} __attribute__((packed));
>> +
>> +struct tpmnv_control {
>> +	struct secboot_header header;
>> +	uint8_t active_bit;
>> +	char bank_hash[SECBOOT_VARIABLE_BANK_NUM][SHA256_DIGEST_LENGTH];
>> +} __attribute__((packed));
>> +
>> +struct tpmnv_ops_s {
>> +	int (*read)(TPMI_RH_NV_INDEX nv, void*, size_t, uint16_t);
>> +	int (*write)(TPMI_RH_NV_INDEX nv, void*, size_t, uint16_t);
>> +	int (*writelock)(TPMI_RH_NV_INDEX);
>> +	int (*definespace)(TPMI_RH_NV_INDEX, uint16_t);
>> +	int (*getindices)(TPMI_RH_NV_INDEX**, size_t*);
>> +	int (*undefinespace)(TPMI_RH_NV_INDEX);
>> +	int (*readpublic)(TPMI_RH_NV_INDEX, TPMS_NV_PUBLIC*, TPM2B_NAME*);
>> +};
>> +
>> +extern struct tpmnv_ops_s tpmnv_ops;
>> +
>> +extern const uint8_t tpmnv_vars_name[];
>> +extern const uint8_t tpmnv_control_name[];
>> +
>> +#endif
>> diff --git a/libstb/secvar/storage/secboot_tpm_public_name.h b/libstb/secvar/storage/secboot_tpm_public_name.h
>> new file mode 100644
>> index 00000000..d0c7fdc1
>> --- /dev/null
>> +++ b/libstb/secvar/storage/secboot_tpm_public_name.h
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
>> +/* Copyright 2020 IBM Corp. */
>> +#ifndef _SECBOOT_TPM_PUBLIC_NAME_H_
>> +#define _SECBOOT_TPM_PUBLIC_NAME_H_
>> +
>> +const uint8_t tpmnv_vars_name[] = {
>> +	0x00, 0x0b, 0x94, 0x64, 0x36, 0x25, 0xfc, 0xc1, 0x1d, 0xc1, 0x0e, 0x28, 0xe7,
>> +	0xac, 0xaf, 0xc6, 0x08, 0x8e, 0xda, 0x21, 0xd6, 0x43, 0xd2, 0x77, 0xe7, 0x2d,
>> +	0x83, 0x39, 0x0f, 0xa6, 0xdf, 0xc0, 0x59, 0x37,
>> +};
>> +
>> +const uint8_t tpmnv_control_name[] = {
>> +	0x00, 0x0b, 0xad, 0x47, 0x6b, 0xa5, 0xdf, 0xb1, 0xe2, 0x18, 0x50, 0xf6, 0x05,
>> +	0x67, 0xe8, 0x8b, 0xa9, 0x0f, 0x86, 0x1f, 0x06, 0xab, 0x43, 0x96, 0x7f, 0x6e,
>> +	0x85, 0x33, 0x5b, 0xa6, 0xf0, 0x63, 0x73, 0xd0,
>> +};
> 
> This should be a .c file or the definitions moved into the file that
> that includes this one.
>