diff mbox series

[RFC,4/7] secvar/storage: add draft secvar storage driver for pnor-based p9 platforms

Message ID 20190610122649.16618-5-erichte@linux.ibm.com
State RFC
Headers show
Series Add Secure Variable Support | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (db3929ee4f0a98596938f05da2789686908ebfd4)

Commit Message

Eric Richter June 10, 2019, 12:26 p.m. UTC
This patch implements the platform specific logic for persisting the
secure variable storage banks across reboots via the SECBOOT PNOR
partition. This is a base implementation meant to provide the minimal
functionality required, and is a work-in-progress.

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. For this initial implementation, the second variable bank
section is ignored, but later patches will alternate writes
between them to provide a back up in the event of a failure.

Forthcoming patches will extend this implementation to utilize the
TPM NV storage space to protect the PNOR storage from external
tampering.

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 include/secvar.h                   |   1 +
 libstb/secvar/storage/Makefile.inc |   2 +-
 libstb/secvar/storage/secboot_p9.c | 243 +++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+), 1 deletion(-)
 create mode 100644 libstb/secvar/storage/secboot_p9.c

Comments

Stewart Smith June 14, 2019, 6:05 a.m. UTC | #1
Eric Richter <erichte@linux.ibm.com> writes:
> This patch implements the platform specific logic for persisting the
> secure variable storage banks across reboots via the SECBOOT PNOR
> partition. This is a base implementation meant to provide the minimal
> functionality required, and is a work-in-progress.

This seems to be a little bit more of a serialisation layer? But not
quite a full one...

>
> 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. For this initial implementation, the second variable bank
> section is ignored, but later patches will alternate writes
> between them to provide a back up in the event of a failure.
>
> Forthcoming patches will extend this implementation to utilize the
> TPM NV storage space to protect the PNOR storage from external
> tampering.
>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>  include/secvar.h                   |   1 +
>  libstb/secvar/storage/Makefile.inc |   2 +-
>  libstb/secvar/storage/secboot_p9.c | 243 +++++++++++++++++++++++++++++
>  3 files changed, 245 insertions(+), 1 deletion(-)
>  create mode 100644 libstb/secvar/storage/secboot_p9.c
>
> diff --git a/include/secvar.h b/include/secvar.h
> index 3a64e8ae..beb2097b 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -33,6 +33,7 @@ struct secvar_backend_driver {
>          int version;                            // Which backend version in use
>  };
>  
> +extern struct secvar_storage_driver secboot_p9_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 c107736e..1ade3ae7 100644
> --- a/libstb/secvar/storage/Makefile.inc
> +++ b/libstb/secvar/storage/Makefile.inc
> @@ -4,7 +4,7 @@ SECVAR_STORAGE_DIR = libstb/secvar/storage
>  
>  SUBDIRS += $(SECVAR_STORAGE_DIR)
>  
> -SECVAR_STORAGE_SRCS =
> +SECVAR_STORAGE_SRCS = secboot_p9.c
>  SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
>  SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
>  
> diff --git a/libstb/secvar/storage/secboot_p9.c b/libstb/secvar/storage/secboot_p9.c
> new file mode 100644
> index 00000000..bb4242c4
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_p9.c
> @@ -0,0 +1,243 @@
> +/* Copyright 2019 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef pr_fmt
> +#define pr_fmt(fmt) "SECBOOT_P9: " fmt
> +#endif
> +
> +#include <stdlib.h>
> +#include <skiboot.h>
> +#include <opal.h>
> +#include "../secvar.h" // TODO: fix relative imports
> +
> +// TODO: Determine reasonable values for these
> +#define SECBOOT_VARIABLE_BANK_SIZE	32000
> +#define SECBOOT_UPDATE_BANK_SIZE	32000

I would think these would be dictated by the size of the underlaying
PNOR partition.

I think we should set a *minimum* size that they *must* be that we can
live with, and then split the pnor partition up on format to be whatever
sizes it needs to be

> +
> +
> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */

I almost want to suggest using the rot13 of PSBR just for lols :)

> +#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 bank0[SECBOOT_VARIABLE_BANK_SIZE];
> +	char bank1[SECBOOT_VARIABLE_BANK_SIZE];
> +	char update[SECBOOT_UPDATE_BANK_SIZE];
> +} __attribute__((packed));

what format are these banks in?

> +struct secboot *secboot_image;
> +
> +
> +static int secboot_format(void)
> +{
> +	if (!platform.secboot_write)
> +		return -1;
> +
> +	memset(secboot_image, 0x00, sizeof(struct secboot));
> +
> +	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
> +	secboot_image->header.version = SECBOOT_VERSION;
> +
> +	return platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +}
> +
> +
> +static int secboot_serialize_bank(struct list_head *bank, char *target, size_t target_size)
> +{
> +	struct secvar_node *node;
> +	char *tmp = target;
> +
> +	if (!bank)
> +		return -1;
> +	if (!target)
> +		return -1;
> +
> +	list_for_each(bank, node, link) {
> +		// Don't persist volatile variables to flash
> +		if (node->var->flags & SECVAR_FLAG_VOLATILE)
> +			continue;
> +
> +		// TODO: skip over writing PK, write this to TPM in the future
> +		if (node->var->flags & SECVAR_FLAG_SECURE_STORAGE)
> +			continue;
> +
> +		// Bail early if we are out of storage space
> +		if ((tmp - target) + sizeof(struct secvar) > target_size) {
> +			return -1;
> +		}
> +
> +		memcpy(target, node->var, sizeof(struct secvar));
> +		((struct secvar *) target)->flags = 0; // TODO: use constant to remove problem flags
> +
> +		target += sizeof(struct secvar);
> +	}

Okay, so I see you're using 'struct secvar' as something that is
*directly* serialised and is a firmware data structure.

This was defined back in patch 2 as:
 +#define SECVAR_FLAG_VOLATILE	0x1
 +struct secvar {
 +	char key[SECVAR_MAX_KEY_LEN];
 +	char metadata[SECVAR_MAX_METADATA_SIZE];
 +	char data[SECVAR_MAX_DATA_SIZE];
 +	uint64_t key_len;
 +	uint64_t metadata_size;
 +	uint64_t data_size;
 +	uint64_t flags;
 +};
 +
 +#define SECVAR_FLAG_VOLATILE		0x1 // Hint for storage driver to ignore variable on writes
 +#define SECVAR_FLAG_DYN_ALLOC		0x2 // Set when metadata and/or data needs to be freed
 +#define SECVAR_FLAG_SECURE_STORAGE	0x4 // Hint for storage driver to select storage location

Which at least has the problem of no endianness specified, nor behaviour
for unknown flags that may be set (i.e. how do we ensure
forward/backwards compatibility of firmware... we want people to be able
to upgrade and possibly downgrade firmware and not lose all their keys)

Since the sizing is this:

 +#define SECVAR_MAX_KEY_LEN		1024
 +#define SECVAR_MAX_METADATA_SIZE	1024
 +#define SECVAR_MAX_DATA_SIZE		2048

This would mean we have each var take up 1024+1024+2048+8+8+8+8=4128
space, as well as the above code *enforces* the 2048 limit on data size.

For the 32000 bytes for a bank specified before, we can only have 7
secure variables.

Say we have PK, KEK, DB, DBX, MOKLIST - we're already nearly out.

> +
> +	return 0;
> +}
> +
> +
> +static int secboot_write_to_pnor(struct list_head *bank, char *target, size_t max_size)
> +{
> +	int ret = 0;
> +
> +	memset(target, 0, max_size);
> +
> +	ret = secboot_serialize_bank(bank, target, max_size);
> +	if (ret)
> +		return ret;
> +
> +	if (!platform.secboot_write) {
> +		prlog(PR_ERR, "Failed to write: platform.secboot_write not set\n");
> +		return -1;
> +	}
> +
> +	ret = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
> +
> +	return ret;
> +}
> +
> +
> +static int secboot_load_from_pnor(struct list_head *bank, char
> *source, size_t max_size)

This doesn't seem to be actually loading anything. is it instead
*parsing* the serialised structure?

> +{
> +	char *src;
> +	struct secvar_node *tmp;
> +	uint64_t empty = 0;
> +
> +	src = source;
> +
> +	while (src < (source + max_size)) {
> +		// Peek to see if we are at the end of the bank
> +		if (!memcmp(src, &empty, sizeof(empty))) {
> +			break;
> +		}
> +
> +		tmp = malloc(sizeof(struct secvar_node));
> +		if (!tmp) {
> +			prlog(PR_ERR, "Could not allocate memory for loading secvar from image\n");
> +			return -1;
> +		}
> +
> +		// TODO: reuse the memory here instead?
> +//		tmp->var = (struct secvar *) src;
> +		tmp->var = zalloc(sizeof(struct secvar));
> +		tmp->var->flags = SECVAR_FLAG_DYN_ALLOC;
> +		memcpy(tmp->var, src, sizeof(struct secvar));

I assume that prior to this  function we've verified the hash of the bank?

> +		list_add_tail(bank, &tmp->link);
> +		src += sizeof(struct secvar);
> +	}
> +
> +	return 0;
> +}
> +
> +// TODO: support bank0 vs bank1

You likely need some internal documentation on what the meaning of the
banks are, how the update process works, and to have unit tests of it to
ensure correctness.

> +static int secboot_p9_write_bank(struct list_head *bank, int section)
> +{
> +	switch(section) {
> +		case SECVAR_VARIABLE_BANK:
> +			return secboot_write_to_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
> +		case SECVAR_UPDATE_BANK:
> +			return secboot_write_to_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
> +	}
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +static int secboot_p9_load_bank(struct list_head *bank, int section)
> +{
> +	switch(section) {
> +		case SECVAR_VARIABLE_BANK:
> +			return secboot_load_from_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
> +		case SECVAR_UPDATE_BANK:
> +			return secboot_load_from_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
> +	}
> +
> +	return OPAL_HARDWARE;
> +}
> +
> +
> +
> +static int secboot_p9_store_init(void)

is p9_store the right thing here? We'd probably have a much different
approach on an FSP system, so perhaps this is more of a pnor backing?

Why would we not also just reuse the serialisation if we did in fact
have lockable flash on a future chip?
Eric Richter June 14, 2019, 8:10 p.m. UTC | #2
On 6/14/19 1:05 AM, Stewart Smith wrote:
> Eric Richter <erichte@linux.ibm.com> writes:
>> This patch implements the platform specific logic for persisting the
>> secure variable storage banks across reboots via the SECBOOT PNOR
>> partition. This is a base implementation meant to provide the minimal
>> functionality required, and is a work-in-progress.
> 
> This seems to be a little bit more of a serialisation layer? But not
> quite a full one...
> 
>>
>> 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. For this initial implementation, the second variable bank
>> section is ignored, but later patches will alternate writes
>> between them to provide a back up in the event of a failure.
>>
>> Forthcoming patches will extend this implementation to utilize the
>> TPM NV storage space to protect the PNOR storage from external
>> tampering.
>>
>> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
>> ---
>>  include/secvar.h                   |   1 +
>>  libstb/secvar/storage/Makefile.inc |   2 +-
>>  libstb/secvar/storage/secboot_p9.c | 243 +++++++++++++++++++++++++++++
>>  3 files changed, 245 insertions(+), 1 deletion(-)
>>  create mode 100644 libstb/secvar/storage/secboot_p9.c
>>
>> diff --git a/include/secvar.h b/include/secvar.h
>> index 3a64e8ae..beb2097b 100644
>> --- a/include/secvar.h
>> +++ b/include/secvar.h
>> @@ -33,6 +33,7 @@ struct secvar_backend_driver {
>>          int version;                            // Which backend version in use
>>  };
>>  
>> +extern struct secvar_storage_driver secboot_p9_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 c107736e..1ade3ae7 100644
>> --- a/libstb/secvar/storage/Makefile.inc
>> +++ b/libstb/secvar/storage/Makefile.inc
>> @@ -4,7 +4,7 @@ SECVAR_STORAGE_DIR = libstb/secvar/storage
>>  
>>  SUBDIRS += $(SECVAR_STORAGE_DIR)
>>  
>> -SECVAR_STORAGE_SRCS =
>> +SECVAR_STORAGE_SRCS = secboot_p9.c
>>  SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
>>  SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
>>  
>> diff --git a/libstb/secvar/storage/secboot_p9.c b/libstb/secvar/storage/secboot_p9.c
>> new file mode 100644
>> index 00000000..bb4242c4
>> --- /dev/null
>> +++ b/libstb/secvar/storage/secboot_p9.c
>> @@ -0,0 +1,243 @@
>> +/* Copyright 2019 IBM Corp.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + *      http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> + * implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "SECBOOT_P9: " fmt
>> +#endif
>> +
>> +#include <stdlib.h>
>> +#include <skiboot.h>
>> +#include <opal.h>
>> +#include "../secvar.h" // TODO: fix relative imports
>> +
>> +// TODO: Determine reasonable values for these
>> +#define SECBOOT_VARIABLE_BANK_SIZE	32000
>> +#define SECBOOT_UPDATE_BANK_SIZE	32000
> 
> I would think these would be dictated by the size of the underlaying
> PNOR partition.
> 
> I think we should set a *minimum* size that they *must* be that we can
> live with, and then split the pnor partition up on format to be whatever
> sizes it needs to be

Minimum size would fit well if we consider what is the minimum number of
variables a particular authentication schema would require.

> >> +
>> +
>> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
> 
> I almost want to suggest using the rot13 of PSBR just for lols :)
> 
>> +#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 bank0[SECBOOT_VARIABLE_BANK_SIZE];
>> +	char bank1[SECBOOT_VARIABLE_BANK_SIZE];
>> +	char update[SECBOOT_UPDATE_BANK_SIZE];
>> +} __attribute__((packed));
> 
> what format are these banks in?
> 

List of secvar structs. They should probably be
struct secvar bank0[], using some ratio for the max number of variables
that can be stored.

However, this nice struct layout may go away if the size of the partition
affects the offsets of where the banks are.

>> +struct secboot *secboot_image;
>> +
>> +
>> +static int secboot_format(void)
>> +{
>> +	if (!platform.secboot_write)
>> +		return -1;
>> +
>> +	memset(secboot_image, 0x00, sizeof(struct secboot));
>> +
>> +	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
>> +	secboot_image->header.version = SECBOOT_VERSION;
>> +
>> +	return platform.secboot_write(0, secboot_image, sizeof(struct secboot));
>> +}
>> +
>> +
>> +static int secboot_serialize_bank(struct list_head *bank, char *target, size_t target_size)
>> +{
>> +	struct secvar_node *node;
>> +	char *tmp = target;
>> +
>> +	if (!bank)
>> +		return -1;
>> +	if (!target)
>> +		return -1;
>> +
>> +	list_for_each(bank, node, link) {
>> +		// Don't persist volatile variables to flash
>> +		if (node->var->flags & SECVAR_FLAG_VOLATILE)
>> +			continue;
>> +
>> +		// TODO: skip over writing PK, write this to TPM in the future
>> +		if (node->var->flags & SECVAR_FLAG_SECURE_STORAGE)
>> +			continue;
>> +
>> +		// Bail early if we are out of storage space
>> +		if ((tmp - target) + sizeof(struct secvar) > target_size) {
>> +			return -1;
>> +		}
>> +
>> +		memcpy(target, node->var, sizeof(struct secvar));
>> +		((struct secvar *) target)->flags = 0; // TODO: use constant to remove problem flags
>> +
>> +		target += sizeof(struct secvar);
>> +	}
> 
> Okay, so I see you're using 'struct secvar' as something that is
> *directly* serialised and is a firmware data structure.
> 
> This was defined back in patch 2 as:
>  +#define SECVAR_FLAG_VOLATILE	0x1
>  +struct secvar {
>  +	char key[SECVAR_MAX_KEY_LEN];
>  +	char metadata[SECVAR_MAX_METADATA_SIZE];
>  +	char data[SECVAR_MAX_DATA_SIZE];
>  +	uint64_t key_len;
>  +	uint64_t metadata_size;
>  +	uint64_t data_size;
>  +	uint64_t flags;
>  +};
>  +
>  +#define SECVAR_FLAG_VOLATILE		0x1 // Hint for storage driver to ignore variable on writes
>  +#define SECVAR_FLAG_DYN_ALLOC		0x2 // Set when metadata and/or data needs to be freed
>  +#define SECVAR_FLAG_SECURE_STORAGE	0x4 // Hint for storage driver to select storage location
> 
> Which at least has the problem of no endianness specified, nor behaviour
> for unknown flags that may be set (i.e. how do we ensure
> forward/backwards compatibility of firmware... we want people to be able
> to upgrade and possibly downgrade firmware and not lose all their keys)
> 
> Since the sizing is this:
> 
>  +#define SECVAR_MAX_KEY_LEN		1024
>  +#define SECVAR_MAX_METADATA_SIZE	1024
>  +#define SECVAR_MAX_DATA_SIZE		2048
> 
> This would mean we have each var take up 1024+1024+2048+8+8+8+8=4128
> space, as well as the above code *enforces* the 2048 limit on data size.
> 
> For the 32000 bytes for a bank specified before, we can only have 7
> secure variables.
> 
> Say we have PK, KEK, DB, DBX, MOKLIST - we're already nearly out.
> 

Changing the struct to be directly serialized was a conscious decision to
reduce the complexity of the storage serialization logic to a single memcpy.
Ideally, the variables don't even need to be copied out of the secboot_image
buffer, and a variable bank item can point directly to where it is in the
buffer (though, that proved to be too complex for this rfc).

Downside of course is the large amount of memory required per variable. The
numbers for both the max size and the partition sizes are largely place-
holders while we figure out what appropriate sizes are.

However, it is unlikely that we will need more variables than that anyway

>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int secboot_write_to_pnor(struct list_head *bank, char *target, size_t max_size)
>> +{
>> +	int ret = 0;
>> +
>> +	memset(target, 0, max_size);
>> +
>> +	ret = secboot_serialize_bank(bank, target, max_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!platform.secboot_write) {
>> +		prlog(PR_ERR, "Failed to write: platform.secboot_write not set\n");
>> +		return -1;
>> +	}
>> +
>> +	ret = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +static int secboot_load_from_pnor(struct list_head *bank, char
>> *source, size_t max_size)
> 
> This doesn't seem to be actually loading anything. is it instead
> *parsing* the serialised structure?
> 

Parse is more accurate, though this is the function that is ultimately called
in the `load_bank` driver hook. Currently matches that, but can be changed
to be more descriptive.

>> +{
>> +	char *src;
>> +	struct secvar_node *tmp;
>> +	uint64_t empty = 0;
>> +
>> +	src = source;
>> +
>> +	while (src < (source + max_size)) {
>> +		// Peek to see if we are at the end of the bank
>> +		if (!memcmp(src, &empty, sizeof(empty))) {
>> +			break;
>> +		}
>> +
>> +		tmp = malloc(sizeof(struct secvar_node));
>> +		if (!tmp) {
>> +			prlog(PR_ERR, "Could not allocate memory for loading secvar from image\n");
>> +			return -1;
>> +		}
>> +
>> +		// TODO: reuse the memory here instead?
>> +//		tmp->var = (struct secvar *) src;
>> +		tmp->var = zalloc(sizeof(struct secvar));
>> +		tmp->var->flags = SECVAR_FLAG_DYN_ALLOC;
>> +		memcpy(tmp->var, src, sizeof(struct secvar));
> 
> I assume that prior to this  function we've verified the hash of the bank?
> 

Hash validation would occur in the _store_init() function, which is called before
loading the banks.

>> +		list_add_tail(bank, &tmp->link);
>> +		src += sizeof(struct secvar);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +// TODO: support bank0 vs bank1
> 
> You likely need some internal documentation on what the meaning of the
> banks are, how the update process works, and to have unit tests of it to
> ensure correctness.
> 
>> +static int secboot_p9_write_bank(struct list_head *bank, int section)
>> +{
>> +	switch(section) {
>> +		case SECVAR_VARIABLE_BANK:
>> +			return secboot_write_to_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
>> +		case SECVAR_UPDATE_BANK:
>> +			return secboot_write_to_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
>> +	}
>> +
>> +	return OPAL_HARDWARE;
>> +}
>> +
>> +static int secboot_p9_load_bank(struct list_head *bank, int section)
>> +{
>> +	switch(section) {
>> +		case SECVAR_VARIABLE_BANK:
>> +			return secboot_load_from_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
>> +		case SECVAR_UPDATE_BANK:
>> +			return secboot_load_from_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
>> +	}
>> +
>> +	return OPAL_HARDWARE;
>> +}
>> +
>> +
>> +
>> +static int secboot_p9_store_init(void)
> 
> is p9_store the right thing here? We'd probably have a much different
> approach on an FSP system, so perhaps this is more of a pnor backing?
> 

No, I've been toying with secboot_tpm_store_, largely since those are
the two storage locations that would be in use.

> Why would we not also just reuse the serialisation if we did in fact
> have lockable flash on a future chip?
> 

If we had an abundance of lockable flash in the future, only the update
bank would need to be written to PNOR. All variables could instead be
written to the lockable flash, which would be locked after processing
updates. Some of the logic may be reusable, but the location would be
different.
diff mbox series

Patch

diff --git a/include/secvar.h b/include/secvar.h
index 3a64e8ae..beb2097b 100644
--- a/include/secvar.h
+++ b/include/secvar.h
@@ -33,6 +33,7 @@  struct secvar_backend_driver {
         int version;                            // Which backend version in use
 };
 
+extern struct secvar_storage_driver secboot_p9_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 c107736e..1ade3ae7 100644
--- a/libstb/secvar/storage/Makefile.inc
+++ b/libstb/secvar/storage/Makefile.inc
@@ -4,7 +4,7 @@  SECVAR_STORAGE_DIR = libstb/secvar/storage
 
 SUBDIRS += $(SECVAR_STORAGE_DIR)
 
-SECVAR_STORAGE_SRCS =
+SECVAR_STORAGE_SRCS = secboot_p9.c
 SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
 SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
 
diff --git a/libstb/secvar/storage/secboot_p9.c b/libstb/secvar/storage/secboot_p9.c
new file mode 100644
index 00000000..bb4242c4
--- /dev/null
+++ b/libstb/secvar/storage/secboot_p9.c
@@ -0,0 +1,243 @@ 
+/* Copyright 2019 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef pr_fmt
+#define pr_fmt(fmt) "SECBOOT_P9: " fmt
+#endif
+
+#include <stdlib.h>
+#include <skiboot.h>
+#include <opal.h>
+#include "../secvar.h" // TODO: fix relative imports
+
+// TODO: Determine reasonable values for these
+#define SECBOOT_VARIABLE_BANK_SIZE	32000
+#define SECBOOT_UPDATE_BANK_SIZE	32000
+
+
+/* 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 bank0[SECBOOT_VARIABLE_BANK_SIZE];
+	char bank1[SECBOOT_VARIABLE_BANK_SIZE];
+	char update[SECBOOT_UPDATE_BANK_SIZE];
+} __attribute__((packed));
+
+struct secboot *secboot_image;
+
+
+static int secboot_format(void)
+{
+	if (!platform.secboot_write)
+		return -1;
+
+	memset(secboot_image, 0x00, sizeof(struct secboot));
+
+	secboot_image->header.magic_number = SECBOOT_MAGIC_NUMBER;
+	secboot_image->header.version = SECBOOT_VERSION;
+
+	return platform.secboot_write(0, secboot_image, sizeof(struct secboot));
+}
+
+
+static int secboot_serialize_bank(struct list_head *bank, char *target, size_t target_size)
+{
+	struct secvar_node *node;
+	char *tmp = target;
+
+	if (!bank)
+		return -1;
+	if (!target)
+		return -1;
+
+	list_for_each(bank, node, link) {
+		// Don't persist volatile variables to flash
+		if (node->var->flags & SECVAR_FLAG_VOLATILE)
+			continue;
+
+		// TODO: skip over writing PK, write this to TPM in the future
+		if (node->var->flags & SECVAR_FLAG_SECURE_STORAGE)
+			continue;
+
+		// Bail early if we are out of storage space
+		if ((tmp - target) + sizeof(struct secvar) > target_size) {
+			return -1;
+		}
+
+		memcpy(target, node->var, sizeof(struct secvar));
+		((struct secvar *) target)->flags = 0; // TODO: use constant to remove problem flags
+
+		target += sizeof(struct secvar);
+	}
+
+	return 0;
+}
+
+
+static int secboot_write_to_pnor(struct list_head *bank, char *target, size_t max_size)
+{
+	int ret = 0;
+
+	memset(target, 0, max_size);
+
+	ret = secboot_serialize_bank(bank, target, max_size);
+	if (ret)
+		return ret;
+
+	if (!platform.secboot_write) {
+		prlog(PR_ERR, "Failed to write: platform.secboot_write not set\n");
+		return -1;
+	}
+
+	ret = platform.secboot_write(0, secboot_image, sizeof(struct secboot));
+
+	return ret;
+}
+
+
+static int secboot_load_from_pnor(struct list_head *bank, char *source, size_t max_size)
+{
+	char *src;
+	struct secvar_node *tmp;
+	uint64_t empty = 0;
+
+	src = source;
+
+	while (src < (source + max_size)) {
+		// Peek to see if we are at the end of the bank
+		if (!memcmp(src, &empty, sizeof(empty))) {
+			break;
+		}
+
+		tmp = malloc(sizeof(struct secvar_node));
+		if (!tmp) {
+			prlog(PR_ERR, "Could not allocate memory for loading secvar from image\n");
+			return -1;
+		}
+
+		// TODO: reuse the memory here instead?
+//		tmp->var = (struct secvar *) src;
+		tmp->var = zalloc(sizeof(struct secvar));
+		tmp->var->flags = SECVAR_FLAG_DYN_ALLOC;
+		memcpy(tmp->var, src, sizeof(struct secvar));
+
+		list_add_tail(bank, &tmp->link);
+		src += sizeof(struct secvar);
+	}
+
+	return 0;
+}
+
+// TODO: support bank0 vs bank1
+static int secboot_p9_write_bank(struct list_head *bank, int section)
+{
+	switch(section) {
+		case SECVAR_VARIABLE_BANK:
+			return secboot_write_to_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
+		case SECVAR_UPDATE_BANK:
+			return secboot_write_to_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
+	}
+
+	return OPAL_HARDWARE;
+}
+
+static int secboot_p9_load_bank(struct list_head *bank, int section)
+{
+	switch(section) {
+		case SECVAR_VARIABLE_BANK:
+			return secboot_load_from_pnor(bank, secboot_image->bank0, SECBOOT_VARIABLE_BANK_SIZE);
+		case SECVAR_UPDATE_BANK:
+			return secboot_load_from_pnor(bank, secboot_image->update, SECBOOT_UPDATE_BANK_SIZE);
+	}
+
+	return OPAL_HARDWARE;
+}
+
+
+
+static int secboot_p9_store_init(void)
+{
+	int ret;
+	unsigned secboot_size;
+
+	/* Already initialized */
+	if (secboot_image)
+		return 0;
+
+	if (!platform.secboot_info)
+		return -1;
+
+	prlog(PR_DEBUG, "Initializing for pnor-based p9 platform\n");
+
+	ret = platform.secboot_info(&secboot_size);
+	if (ret) {
+		prlog(PR_ERR, "error %d retrieving keystore info\n", ret);
+		return -1;
+	}
+	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 -1;
+	}
+
+	secboot_image = memalign(0x1000, sizeof(struct secboot));
+	if (!secboot_image) {
+		prlog(PR_ERR, "Failed to allocate space for the secboot image\n");
+		return -1;
+	}
+
+	/* Read it in */
+	ret = platform.secboot_read(secboot_image, 0, sizeof(struct secboot));
+	if (ret) {
+		prlog(PR_ERR, "failed to read the secboot partition, rc=%d\n", ret);
+		goto out_free;
+	}
+
+	if (secboot_image->header.magic_number != SECBOOT_MAGIC_NUMBER) {
+		prlog(PR_INFO, "Formatting secboot partition...\n");
+		ret = secboot_format();
+		if (ret) {
+			prlog(PR_ERR, "Failed to format secboot!\n");
+			goto out_free;
+		}
+	}
+
+	return 0;
+
+out_free:
+	if (secboot_image) {
+		free(secboot_image);
+		secboot_image = NULL;
+	}
+
+	return -1;
+}
+
+struct secvar_storage_driver secboot_p9_driver = {
+	.load_bank = secboot_p9_load_bank,
+	.write_bank = secboot_p9_write_bank,
+	.store_init = secboot_p9_store_init,
+};