diff mbox series

[2/7] libstb/secvar: add secure variable internal abstraction

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

Checks

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

Commit Message

Eric Richter June 10, 2019, 12:26 p.m. UTC 
This patch implements a platform-independent abstraction for storing and
retrieving secure variables, as required for host OS secure boot. This
serves as the main entry point for initializing the in-memory cache of the
secure variables, which also kicks off any platform-specific logic that may
be needed. This patch also provides core functions for the subsequent
patches in this series to utilize.

The base secure variable implementation makes use of two types of
drivers, to be selected by the platform: "storage" drivers, and
"backend" drivers. The storage driver implements the hooks required to
write the secure variables to some form of non-volatile memory, and load
the variables on boot. The backend driver defines how the variables
should be interpreted, and processed.

Secure variables are stored in two types of banks, the "variable" bank
and the "update" bank. Variables that have been validated and processed
are stored in the variable bank. This bank is effectively read-only
after the base secvar initialization. Any proposed variable updates are
instead stored in the update bank. During secvar initialization, the
backend driver processes variables from the update bank, and if valid,
adds the new variable to the variable bank.

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 include/platform.h                 |  1 +
 include/secvar.h                   | 39 +++++++++++++
 libstb/Makefile.inc                |  3 +-
 libstb/secureboot.c                |  2 +
 libstb/secvar/Makefile.inc         | 14 +++++
 libstb/secvar/backend/Makefile.inc | 11 ++++
 libstb/secvar/secvar.h             | 71 ++++++++++++++++++++++++
 libstb/secvar/secvar_main.c        | 89 ++++++++++++++++++++++++++++++
 libstb/secvar/secvar_util.c        | 88 +++++++++++++++++++++++++++++
 libstb/secvar/storage/Makefile.inc | 11 ++++
 10 files changed, 328 insertions(+), 1 deletion(-)
 create mode 100644 include/secvar.h
 create mode 100644 libstb/secvar/Makefile.inc
 create mode 100644 libstb/secvar/backend/Makefile.inc
 create mode 100644 libstb/secvar/secvar.h
 create mode 100644 libstb/secvar/secvar_main.c
 create mode 100644 libstb/secvar/secvar_util.c
 create mode 100644 libstb/secvar/storage/Makefile.inc

Comments

Stewart Smith June 14, 2019, 4:27 a.m. UTC | #1 
Eric Richter <erichte@linux.ibm.com> writes:
> This patch implements a platform-independent abstraction for storing and
> retrieving secure variables, as required for host OS secure boot. This
> serves as the main entry point for initializing the in-memory cache of the
> secure variables, which also kicks off any platform-specific logic that may
> be needed. This patch also provides core functions for the subsequent
> patches in this series to utilize.
>
> The base secure variable implementation makes use of two types of
> drivers, to be selected by the platform: "storage" drivers, and
> "backend" drivers. The storage driver implements the hooks required to
> write the secure variables to some form of non-volatile memory, and load
> the variables on boot. The backend driver defines how the variables
> should be interpreted, and processed.

having a "backend driver" at this stage seems a bit like premature
abstraction, as we may never change that serialisation format.

> Secure variables are stored in two types of banks, the "variable" bank
> and the "update" bank. Variables that have been validated and processed
> are stored in the variable bank. This bank is effectively read-only
> after the base secvar initialization. Any proposed variable updates are
> instead stored in the update bank. During secvar initialization, the
> backend driver processes variables from the update bank, and if valid,
> adds the new variable to the variable bank.

"effectively" or "are"?
>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>  include/platform.h                 |  1 +
>  include/secvar.h                   | 39 +++++++++++++
>  libstb/Makefile.inc                |  3 +-
>  libstb/secureboot.c                |  2 +
>  libstb/secvar/Makefile.inc         | 14 +++++
>  libstb/secvar/backend/Makefile.inc | 11 ++++
>  libstb/secvar/secvar.h             | 71 ++++++++++++++++++++++++
>  libstb/secvar/secvar_main.c        | 89 ++++++++++++++++++++++++++++++
>  libstb/secvar/secvar_util.c        | 88 +++++++++++++++++++++++++++++
>  libstb/secvar/storage/Makefile.inc | 11 ++++
>  10 files changed, 328 insertions(+), 1 deletion(-)
>  create mode 100644 include/secvar.h
>  create mode 100644 libstb/secvar/Makefile.inc
>  create mode 100644 libstb/secvar/backend/Makefile.inc
>  create mode 100644 libstb/secvar/secvar.h
>  create mode 100644 libstb/secvar/secvar_main.c
>  create mode 100644 libstb/secvar/secvar_util.c
>  create mode 100644 libstb/secvar/storage/Makefile.inc
>
> diff --git a/include/platform.h b/include/platform.h
> index b29966f9..97e93c2a 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -186,6 +186,7 @@ struct platform {
>  	int (*secboot_read)(void *dst, uint32_t src, uint32_t len);
>  	int (*secboot_write)(uint32_t dst, void *src, uint32_t len);
>  
> +	int (*secvar_init)(void);
>  	/*
>  	 * OCC timeout. This return how long we should wait for the OCC
>  	 * before timing out. This lets us use a high value on larger FSP
> diff --git a/include/secvar.h b/include/secvar.h
> new file mode 100644
> index 00000000..3a64e8ae
> --- /dev/null
> +++ b/include/secvar.h
> @@ -0,0 +1,39 @@
> +/* 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 _SECVAR_DRIVER_
> +#define _SECVAR_DRIVER_
> +
> +struct secvar;
> +
> +struct secvar_storage_driver {
> +        int (*load_bank)(struct list_head *bank, int section);
> +        int (*write_bank)(struct list_head *bank, int section);
> +        int (*store_init)(void);
> +};
> +
> +struct secvar_backend_driver {
> +        int (*pre_process)(void);               // Perform any pre-processing stuff (e.g. determine secure boot state)
> +        int (*process)(void);                   // Process all updates
> +        int (*post_process)(void);              // Perform any post-processing stuff (e.g. derive/update variables)
> +        int (*validate)(struct secvar *var);    // Validate a single variable, return boolean
> +        int version;                            // Which backend version in use
> +};
> +
> +
> +int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
> +
> +#endif
> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
> index 6d54c5cd..d3f68496 100644
> --- a/libstb/Makefile.inc
> +++ b/libstb/Makefile.inc
> @@ -8,11 +8,12 @@ LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c
>  LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
>  LIBSTB = $(LIBSTB_DIR)/built-in.a
>  
> +include $(SRC)/$(LIBSTB_DIR)/secvar/Makefile.inc
>  include $(SRC)/$(LIBSTB_DIR)/mbedtls/Makefile.inc
>  include $(SRC)/$(LIBSTB_DIR)/drivers/Makefile.inc
>  include $(SRC)/$(LIBSTB_DIR)/tss/Makefile.inc
>  
> -$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(MBEDTLS)
> +$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(SECVAR) $(MBEDTLS)
>  
>  libstb/create-container: libstb/create-container.c libstb/container-utils.c
>  	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) \
> diff --git a/libstb/secureboot.c b/libstb/secureboot.c
> index 1578f52e..68c81ebf 100644
> --- a/libstb/secureboot.c
> +++ b/libstb/secureboot.c
> @@ -170,6 +170,8 @@ void secureboot_init(void)
>  	if (cvc_init())
>  		secureboot_enforce();
>  
> +	if (platform.secvar_init)
> +		platform.secvar_init();
>  	secure_init = true;
>  }
>  
> diff --git a/libstb/secvar/Makefile.inc b/libstb/secvar/Makefile.inc
> new file mode 100644
> index 00000000..75870910
> --- /dev/null
> +++ b/libstb/secvar/Makefile.inc
> @@ -0,0 +1,14 @@
> +# -*-Makefile-*-
> +
> +SECVAR_DIR = libstb/secvar
> +
> +SUBDIRS += $(SECVAR_DIR)
> +
> +include $(SECVAR_DIR)/storage/Makefile.inc
> +include $(SECVAR_DIR)/backend/Makefile.inc
> +
> +SECVAR_SRCS = secvar_main.c secvar_util.c
> +SECVAR_OBJS = $(SECVAR_SRCS:%.c=%.o)
> +SECVAR = $(SECVAR_DIR)/built-in.a
> +
> +$(SECVAR): $(SECVAR_OBJS:%=$(SECVAR_DIR)/%) $(SECVAR_STORAGE) $(SECVAR_BACKEND)
> diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
> new file mode 100644
> index 00000000..7a7ca1f7
> --- /dev/null
> +++ b/libstb/secvar/backend/Makefile.inc
> @@ -0,0 +1,11 @@
> +# -*-Makefile-*-
> +
> +SECVAR_BACKEND_DIR = libstb/secvar/backend
> +
> +SUBDIRS += $(SECVAR_BACKEND_DIR)
> +
> +SECVAR_BACKEND_SRCS =
> +SECVAR_BACKEND_OBJS = $(SECVAR_BACKEND_SRCS:%.c=%.o)
> +SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
> +
> +$(SECVAR_BACKEND): $(SECVAR_BACKEND_OBJS:%=$(SECVAR_BACKEND_DIR)/%)
> diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
> new file mode 100644
> index 00000000..d02a0cef
> --- /dev/null
> +++ b/libstb/secvar/secvar.h
> @@ -0,0 +1,71 @@
> +/* 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 _SECVAR_H_
> +#define _SECVAR_H_
> +
> +#include <ccan/list/list.h>
> +#include <stdint.h>
> +#include <secvar.h>
> +
> +#define SECVAR_MAX_KEY_LEN		1024
> +#define SECVAR_MAX_METADATA_SIZE	1024
> +#define SECVAR_MAX_DATA_SIZE		2048

I feel these should be exposed in the device tree, as well as clearly
documented in doc/ about what's the minimum that a platform is allowed
to support.

i.e. could I make a box that only allows 128 byte keys?

> +
> +enum {
> +	SECVAR_VARIABLE_BANK,
> +	SECVAR_UPDATE_BANK,
> +};
> +
> +
> +struct secvar_node {
> +	struct list_node link;
> +	struct secvar *var;
> +};
> +
> +#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

This is already defined above. Is it really a hint? Can a storage
backend persist these or not?

> +#define SECVAR_FLAG_DYN_ALLOC		0x2 // Set when metadata and/or data needs to be freed

I'm not sure why this exists, the max size is specified and allowed for

> +#define SECVAR_FLAG_SECURE_STORAGE	0x4 // Hint for storage driver to select storage location

Hint or requirement?

> +
> +enum {
> +	BACKEND_NONE = 0,
> +	BACKEND_TC_COMPAT_V1,
> +};
> +
> +extern struct list_head variable_bank;
> +extern struct list_head update_bank;
> +extern int secvar_enabled;
> +extern struct secvar_storage_driver secvar_storage;
> +extern struct secvar_backend_driver secvar_backend;
> +
> +// Helper functions
> +void clear_bank_list(struct list_head *bank);
> +struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank);
> +int is_key_empty(char *key, uint64_t key_len);
> +int list_length(struct list_head *bank);
> +
> +#endif
> diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
> new file mode 100644
> index 00000000..4a3c97a8
> --- /dev/null
> +++ b/libstb/secvar/secvar_main.c
> @@ -0,0 +1,89 @@
> +/* 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) "SECVAR: " fmt
> +#endif
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <skiboot.h>
> +#include <opal.h>
> +#include "secvar.h"
> +
> +struct list_head variable_bank;
> +struct list_head update_bank;
> +int secvar_enabled = 0;
> +
> +// To be filled in by platform.secvar_init
> +struct secvar_storage_driver secvar_storage = {0};
> +struct secvar_backend_driver secvar_backend = {0};
> +
> +int secvar_main(struct secvar_storage_driver storage_driver,
> +               struct secvar_backend_driver backend_driver)
> +{
> +	int rc = OPAL_UNSUPPORTED;
> +
> +	secvar_storage = storage_driver;
> +	secvar_backend = backend_driver;
> +
> +	list_head_init(&variable_bank);
> +	list_head_init(&update_bank);
> +
> +	rc = secvar_storage.store_init();
> +	if (rc)
> +		goto out;
> +
> +	if (secvar_backend.pre_process)
> +		rc = secvar_backend.pre_process();
> +	if (rc)
> +		goto out;
> +
> +	rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	if (rc)
> +		goto out;
> +
> +	rc = secvar_storage.load_bank(&update_bank, SECVAR_UPDATE_BANK);
> +	if (rc)
> +		goto out;
> +
> +	if (secvar_backend.process)
> +		rc = secvar_backend.process();
> +	if (rc)
> +		goto out;
> +
> +	rc = secvar_storage.write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
> +	if (rc)
> +		goto out;
> +
> +	rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK);
> +	if (rc)
> +		goto out;
> +
> +	// TODO: maybe this should just be part of .process()?
> +	if (secvar_backend.post_process)
> +		rc = secvar_backend.post_process();
> +	if (rc)
> +		goto out;
> +
> +	secvar_enabled = 1;
> +
> +	return OPAL_SUCCESS;
> +
> +out:
> +	printf("Secure Variables Status %04x\n", rc);
> +	return rc;
> +}
> diff --git a/libstb/secvar/secvar_util.c b/libstb/secvar/secvar_util.c
> new file mode 100644
> index 00000000..5d79c991
> --- /dev/null
> +++ b/libstb/secvar/secvar_util.c
> @@ -0,0 +1,88 @@
> +/* 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) "SECVAR: " fmt
> +#endif
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <skiboot.h>
> +#include <opal.h>
> +#include "secvar.h"
> +
> +void clear_bank_list(struct list_head *bank)
> +{
> +	struct secvar_node *node, *next;
> +
> +	if (!bank)
> +		return;
> +
> +	list_for_each_safe(bank, node, next, link) {
> +		list_del(&node->link);
> +
> +		if (!node->var) {
> +			free(node);
> +			continue;
> +		}
> +
> +		if (node->var->flags & SECVAR_FLAG_DYN_ALLOC) {
> +			free(node->var);
> +		}
> +
> +		free(node);
> +	}
> +}
> +
> +struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank)
> +{
> +	struct secvar_node *node = NULL;
> +
> +	list_for_each(bank, node, link) {
> +		// Prevent matching shorter key subsets / bail early
> +		if (key_len != node->var->key_len)
> +			continue;
> +		if (!memcmp(key, node->var->key, key_len)) {
> +			return node;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +
> +int is_key_empty(char *key, uint64_t key_len)
> +{
> +	int i;
> +	for (i = 0; i < key_len; i++) {
> +		if (key[i] != 0)
> +			return 0;
> +	}
> +
> +	return 1;
> +}

so, i see this is used later on, where does the "all nulls but not zero
length means empty" rule come from?

> +
> +int list_length(struct list_head *bank)

this looks unused, at least for anything that isn't debug?

It also sits in the "namespace" of ccan/list/list.h, and IIRC has
generally not been a function provided so that anyone looking to use it
reconsiders their actions as there's no cached count or anything. Is
this actually needed anywhere?

> +{
> +	int ret = 0;
> +	struct secvar_node *node;
> +
> +	list_for_each(bank, node, link) {
> +		ret++;
> +	}
> +
> +	return ret;
> +}
> diff --git a/libstb/secvar/storage/Makefile.inc b/libstb/secvar/storage/Makefile.inc
> new file mode 100644
> index 00000000..c107736e
> --- /dev/null
> +++ b/libstb/secvar/storage/Makefile.inc
> @@ -0,0 +1,11 @@
> +# -*-Makefile-*-
> +
> +SECVAR_STORAGE_DIR = libstb/secvar/storage
> +
> +SUBDIRS += $(SECVAR_STORAGE_DIR)
> +
> +SECVAR_STORAGE_SRCS =
> +SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
> +SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
> +
> +$(SECVAR_STORAGE): $(SECVAR_STORAGE_OBJS:%=$(SECVAR_STORAGE_DIR)/%)
> -- 
> 2.20.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
Eric Richter June 14, 2019, 8:10 p.m. UTC | #2 
On 6/13/19 11:27 PM, Stewart Smith wrote:
> Eric Richter <erichte@linux.ibm.com> writes:
>> This patch implements a platform-independent abstraction for storing and
>> retrieving secure variables, as required for host OS secure boot. This
>> serves as the main entry point for initializing the in-memory cache of the
>> secure variables, which also kicks off any platform-specific logic that may
>> be needed. This patch also provides core functions for the subsequent
>> patches in this series to utilize.
>>
>> The base secure variable implementation makes use of two types of
>> drivers, to be selected by the platform: "storage" drivers, and
>> "backend" drivers. The storage driver implements the hooks required to
>> write the secure variables to some form of non-volatile memory, and load
>> the variables on boot. The backend driver defines how the variables
>> should be interpreted, and processed.
> 
> having a "backend driver" at this stage seems a bit like premature
> abstraction, as we may never change that serialisation format.
> 
>> Secure variables are stored in two types of banks, the "variable" bank
>> and the "update" bank. Variables that have been validated and processed
>> are stored in the variable bank. This bank is effectively read-only
>> after the base secvar initialization. Any proposed variable updates are
>> instead stored in the update bank. During secvar initialization, the
>> backend driver processes variables from the update bank, and if valid,
>> adds the new variable to the variable bank.
> 
> "effectively" or "are"?

Are as read only as possible. No code beyond the initialization step
writes to the variable bank list, and the API can only add variables to
the update bank. This assumes skiboot memory is relatively tamper
resistant.

>>
>> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
>> ---
>>  include/platform.h                 |  1 +
>>  include/secvar.h                   | 39 +++++++++++++
>>  libstb/Makefile.inc                |  3 +-
>>  libstb/secureboot.c                |  2 +
>>  libstb/secvar/Makefile.inc         | 14 +++++
>>  libstb/secvar/backend/Makefile.inc | 11 ++++
>>  libstb/secvar/secvar.h             | 71 ++++++++++++++++++++++++
>>  libstb/secvar/secvar_main.c        | 89 ++++++++++++++++++++++++++++++
>>  libstb/secvar/secvar_util.c        | 88 +++++++++++++++++++++++++++++
>>  libstb/secvar/storage/Makefile.inc | 11 ++++
>>  10 files changed, 328 insertions(+), 1 deletion(-)
>>  create mode 100644 include/secvar.h
>>  create mode 100644 libstb/secvar/Makefile.inc
>>  create mode 100644 libstb/secvar/backend/Makefile.inc
>>  create mode 100644 libstb/secvar/secvar.h
>>  create mode 100644 libstb/secvar/secvar_main.c
>>  create mode 100644 libstb/secvar/secvar_util.c
>>  create mode 100644 libstb/secvar/storage/Makefile.inc
>>
>> diff --git a/include/platform.h b/include/platform.h
>> index b29966f9..97e93c2a 100644
>> --- a/include/platform.h
>> +++ b/include/platform.h
>> @@ -186,6 +186,7 @@ struct platform {
>>  	int (*secboot_read)(void *dst, uint32_t src, uint32_t len);
>>  	int (*secboot_write)(uint32_t dst, void *src, uint32_t len);
>>  
>> +	int (*secvar_init)(void);
>>  	/*
>>  	 * OCC timeout. This return how long we should wait for the OCC
>>  	 * before timing out. This lets us use a high value on larger FSP
>> diff --git a/include/secvar.h b/include/secvar.h
>> new file mode 100644
>> index 00000000..3a64e8ae
>> --- /dev/null
>> +++ b/include/secvar.h
>> @@ -0,0 +1,39 @@
>> +/* 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 _SECVAR_DRIVER_
>> +#define _SECVAR_DRIVER_
>> +
>> +struct secvar;
>> +
>> +struct secvar_storage_driver {
>> +        int (*load_bank)(struct list_head *bank, int section);
>> +        int (*write_bank)(struct list_head *bank, int section);
>> +        int (*store_init)(void);
>> +};
>> +
>> +struct secvar_backend_driver {
>> +        int (*pre_process)(void);               // Perform any pre-processing stuff (e.g. determine secure boot state)
>> +        int (*process)(void);                   // Process all updates
>> +        int (*post_process)(void);              // Perform any post-processing stuff (e.g. derive/update variables)
>> +        int (*validate)(struct secvar *var);    // Validate a single variable, return boolean
>> +        int version;                            // Which backend version in use
>> +};
>> +
>> +
>> +int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
>> +
>> +#endif
>> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
>> index 6d54c5cd..d3f68496 100644
>> --- a/libstb/Makefile.inc
>> +++ b/libstb/Makefile.inc
>> @@ -8,11 +8,12 @@ LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c
>>  LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
>>  LIBSTB = $(LIBSTB_DIR)/built-in.a
>>  
>> +include $(SRC)/$(LIBSTB_DIR)/secvar/Makefile.inc
>>  include $(SRC)/$(LIBSTB_DIR)/mbedtls/Makefile.inc
>>  include $(SRC)/$(LIBSTB_DIR)/drivers/Makefile.inc
>>  include $(SRC)/$(LIBSTB_DIR)/tss/Makefile.inc
>>  
>> -$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(MBEDTLS)
>> +$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(SECVAR) $(MBEDTLS)
>>  
>>  libstb/create-container: libstb/create-container.c libstb/container-utils.c
>>  	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) \
>> diff --git a/libstb/secureboot.c b/libstb/secureboot.c
>> index 1578f52e..68c81ebf 100644
>> --- a/libstb/secureboot.c
>> +++ b/libstb/secureboot.c
>> @@ -170,6 +170,8 @@ void secureboot_init(void)
>>  	if (cvc_init())
>>  		secureboot_enforce();
>>  
>> +	if (platform.secvar_init)
>> +		platform.secvar_init();
>>  	secure_init = true;
>>  }
>>  
>> diff --git a/libstb/secvar/Makefile.inc b/libstb/secvar/Makefile.inc
>> new file mode 100644
>> index 00000000..75870910
>> --- /dev/null
>> +++ b/libstb/secvar/Makefile.inc
>> @@ -0,0 +1,14 @@
>> +# -*-Makefile-*-
>> +
>> +SECVAR_DIR = libstb/secvar
>> +
>> +SUBDIRS += $(SECVAR_DIR)
>> +
>> +include $(SECVAR_DIR)/storage/Makefile.inc
>> +include $(SECVAR_DIR)/backend/Makefile.inc
>> +
>> +SECVAR_SRCS = secvar_main.c secvar_util.c
>> +SECVAR_OBJS = $(SECVAR_SRCS:%.c=%.o)
>> +SECVAR = $(SECVAR_DIR)/built-in.a
>> +
>> +$(SECVAR): $(SECVAR_OBJS:%=$(SECVAR_DIR)/%) $(SECVAR_STORAGE) $(SECVAR_BACKEND)
>> diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
>> new file mode 100644
>> index 00000000..7a7ca1f7
>> --- /dev/null
>> +++ b/libstb/secvar/backend/Makefile.inc
>> @@ -0,0 +1,11 @@
>> +# -*-Makefile-*-
>> +
>> +SECVAR_BACKEND_DIR = libstb/secvar/backend
>> +
>> +SUBDIRS += $(SECVAR_BACKEND_DIR)
>> +
>> +SECVAR_BACKEND_SRCS =
>> +SECVAR_BACKEND_OBJS = $(SECVAR_BACKEND_SRCS:%.c=%.o)
>> +SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
>> +
>> +$(SECVAR_BACKEND): $(SECVAR_BACKEND_OBJS:%=$(SECVAR_BACKEND_DIR)/%)
>> diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
>> new file mode 100644
>> index 00000000..d02a0cef
>> --- /dev/null
>> +++ b/libstb/secvar/secvar.h
>> @@ -0,0 +1,71 @@
>> +/* 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 _SECVAR_H_
>> +#define _SECVAR_H_
>> +
>> +#include <ccan/list/list.h>
>> +#include <stdint.h>
>> +#include <secvar.h>
>> +
>> +#define SECVAR_MAX_KEY_LEN		1024
>> +#define SECVAR_MAX_METADATA_SIZE	1024
>> +#define SECVAR_MAX_DATA_SIZE		2048
> 
> I feel these should be exposed in the device tree, as well as clearly
> documented in doc/ about what's the minimum that a platform is allowed
> to support.
> 
> i.e. could I make a box that only allows 128 byte keys?
> 

If these are to be changed by the platform, then yes these should be
exposed via device tree nodes. Changing the size hadn't been originally
intended, but that might be something a platform decides to change,
particularly if it wants to conserve space/memory.

>> +
>> +enum {
>> +	SECVAR_VARIABLE_BANK,
>> +	SECVAR_UPDATE_BANK,
>> +};
>> +
>> +
>> +struct secvar_node {
>> +	struct list_node link;
>> +	struct secvar *var;
>> +};
>> +
>> +#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
> 
> This is already defined above. Is it really a hint? Can a storage
> backend persist these or not?

Whoops, bad squash/rebase.

Hint probably isn't the correct term as a storage driver *should not*
persist a variable with this flag to any storage location.

> 
>> +#define SECVAR_FLAG_DYN_ALLOC		0x2 // Set when metadata and/or data needs to be freed
> 
> I'm not sure why this exists, the max size is specified and allowed for

This is a hint for garbage collection. Ideally, this shouldn't need to
exist, and either everything is dynamically allocated, or everything is
part of a pre-allocated buffer.

This was intended to separate variables that are pointers into a larger
buffer (e.g. secboot image loaded into memory), and variables that are
allocated at runtime such as volatile variables or enqueued variable
updates.

> 
>> +#define SECVAR_FLAG_SECURE_STORAGE	0x4 // Hint for storage driver to select storage location
> 
> Hint or requirement?

Hint. This should be used sparingly by the authentication schema for
variables like a platform key, that should be stored in write-proof
storage. If in the future we have a larger amount of write-lockable
flash, this hint can be ignored by that platform's storage driver.

> 
>> +
>> +enum {
>> +	BACKEND_NONE = 0,
>> +	BACKEND_TC_COMPAT_V1,
>> +};
>> +
>> +extern struct list_head variable_bank;
>> +extern struct list_head update_bank;
>> +extern int secvar_enabled;
>> +extern struct secvar_storage_driver secvar_storage;
>> +extern struct secvar_backend_driver secvar_backend;
>> +
>> +// Helper functions
>> +void clear_bank_list(struct list_head *bank);
>> +struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank);
>> +int is_key_empty(char *key, uint64_t key_len);
>> +int list_length(struct list_head *bank);
>> +
>> +#endif
>> diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
>> new file mode 100644
>> index 00000000..4a3c97a8
>> --- /dev/null
>> +++ b/libstb/secvar/secvar_main.c
>> @@ -0,0 +1,89 @@
>> +/* 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) "SECVAR: " fmt
>> +#endif
>> +
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <skiboot.h>
>> +#include <opal.h>
>> +#include "secvar.h"
>> +
>> +struct list_head variable_bank;
>> +struct list_head update_bank;
>> +int secvar_enabled = 0;
>> +
>> +// To be filled in by platform.secvar_init
>> +struct secvar_storage_driver secvar_storage = {0};
>> +struct secvar_backend_driver secvar_backend = {0};
>> +
>> +int secvar_main(struct secvar_storage_driver storage_driver,
>> +               struct secvar_backend_driver backend_driver)
>> +{
>> +	int rc = OPAL_UNSUPPORTED;
>> +
>> +	secvar_storage = storage_driver;
>> +	secvar_backend = backend_driver;
>> +
>> +	list_head_init(&variable_bank);
>> +	list_head_init(&update_bank);
>> +
>> +	rc = secvar_storage.store_init();
>> +	if (rc)
>> +		goto out;
>> +
>> +	if (secvar_backend.pre_process)
>> +		rc = secvar_backend.pre_process();
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = secvar_storage.load_bank(&update_bank, SECVAR_UPDATE_BANK);
>> +	if (rc)
>> +		goto out;
>> +
>> +	if (secvar_backend.process)
>> +		rc = secvar_backend.process();
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = secvar_storage.write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
>> +	if (rc)
>> +		goto out;
>> +
>> +	rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK);
>> +	if (rc)
>> +		goto out;
>> +
>> +	// TODO: maybe this should just be part of .process()?
>> +	if (secvar_backend.post_process)
>> +		rc = secvar_backend.post_process();
>> +	if (rc)
>> +		goto out;
>> +
>> +	secvar_enabled = 1;
>> +
>> +	return OPAL_SUCCESS;
>> +
>> +out:
>> +	printf("Secure Variables Status %04x\n", rc);
>> +	return rc;
>> +}
>> diff --git a/libstb/secvar/secvar_util.c b/libstb/secvar/secvar_util.c
>> new file mode 100644
>> index 00000000..5d79c991
>> --- /dev/null
>> +++ b/libstb/secvar/secvar_util.c
>> @@ -0,0 +1,88 @@
>> +/* 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) "SECVAR: " fmt
>> +#endif
>> +
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <skiboot.h>
>> +#include <opal.h>
>> +#include "secvar.h"
>> +
>> +void clear_bank_list(struct list_head *bank)
>> +{
>> +	struct secvar_node *node, *next;
>> +
>> +	if (!bank)
>> +		return;
>> +
>> +	list_for_each_safe(bank, node, next, link) {
>> +		list_del(&node->link);
>> +
>> +		if (!node->var) {
>> +			free(node);
>> +			continue;
>> +		}
>> +
>> +		if (node->var->flags & SECVAR_FLAG_DYN_ALLOC) {
>> +			free(node->var);
>> +		}
>> +
>> +		free(node);
>> +	}
>> +}
>> +
>> +struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank)
>> +{
>> +	struct secvar_node *node = NULL;
>> +
>> +	list_for_each(bank, node, link) {
>> +		// Prevent matching shorter key subsets / bail early
>> +		if (key_len != node->var->key_len)
>> +			continue;
>> +		if (!memcmp(key, node->var->key, key_len)) {
>> +			return node;
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +
>> +int is_key_empty(char *key, uint64_t key_len)
>> +{
>> +	int i;
>> +	for (i = 0; i < key_len; i++) {
>> +		if (key[i] != 0)
>> +			return 0;
>> +	}
>> +
>> +	return 1;
>> +}
> 
> so, i see this is used later on, where does the "all nulls but not zero
> length means empty" rule come from?
> 

Alternative option to request the first item in a list in _get_next. I
don't see a valid reason for a string full of nulls to be a key,
particularly since that permits different keys with different amount of
nulls to be valid.

>> +
>> +int list_length(struct list_head *bank)
> 
> this looks unused, at least for anything that isn't debug?
> 
> It also sits in the "namespace" of ccan/list/list.h, and IIRC has
> generally not been a function provided so that anyone looking to use it
> reconsiders their actions as there's no cached count or anything. Is
> this actually needed anywhere?
> 

Hmmm, it was used in processing at one point, but it seems that code has
also been removed. I agree, this is fair game to remove.

>> +{
>> +	int ret = 0;
>> +	struct secvar_node *node;
>> +
>> +	list_for_each(bank, node, link) {
>> +		ret++;
>> +	}
>> +
>> +	return ret;
>> +}
>> diff --git a/libstb/secvar/storage/Makefile.inc b/libstb/secvar/storage/Makefile.inc
>> new file mode 100644
>> index 00000000..c107736e
>> --- /dev/null
>> +++ b/libstb/secvar/storage/Makefile.inc
>> @@ -0,0 +1,11 @@
>> +# -*-Makefile-*-
>> +
>> +SECVAR_STORAGE_DIR = libstb/secvar/storage
>> +
>> +SUBDIRS += $(SECVAR_STORAGE_DIR)
>> +
>> +SECVAR_STORAGE_SRCS =
>> +SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
>> +SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
>> +
>> +$(SECVAR_STORAGE): $(SECVAR_STORAGE_OBJS:%=$(SECVAR_STORAGE_DIR)/%)
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>>
>
diff mbox series

Patch

diff --git a/include/platform.h b/include/platform.h
index b29966f9..97e93c2a 100644
--- a/include/platform.h
+++ b/include/platform.h
@@ -186,6 +186,7 @@  struct platform {
 	int (*secboot_read)(void *dst, uint32_t src, uint32_t len);
 	int (*secboot_write)(uint32_t dst, void *src, uint32_t len);
 
+	int (*secvar_init)(void);
 	/*
 	 * OCC timeout. This return how long we should wait for the OCC
 	 * before timing out. This lets us use a high value on larger FSP
diff --git a/include/secvar.h b/include/secvar.h
new file mode 100644
index 00000000..3a64e8ae
--- /dev/null
+++ b/include/secvar.h
@@ -0,0 +1,39 @@ 
+/* 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 _SECVAR_DRIVER_
+#define _SECVAR_DRIVER_
+
+struct secvar;
+
+struct secvar_storage_driver {
+        int (*load_bank)(struct list_head *bank, int section);
+        int (*write_bank)(struct list_head *bank, int section);
+        int (*store_init)(void);
+};
+
+struct secvar_backend_driver {
+        int (*pre_process)(void);               // Perform any pre-processing stuff (e.g. determine secure boot state)
+        int (*process)(void);                   // Process all updates
+        int (*post_process)(void);              // Perform any post-processing stuff (e.g. derive/update variables)
+        int (*validate)(struct secvar *var);    // Validate a single variable, return boolean
+        int version;                            // Which backend version in use
+};
+
+
+int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
+
+#endif
diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
index 6d54c5cd..d3f68496 100644
--- a/libstb/Makefile.inc
+++ b/libstb/Makefile.inc
@@ -8,11 +8,12 @@  LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c
 LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
 LIBSTB = $(LIBSTB_DIR)/built-in.a
 
+include $(SRC)/$(LIBSTB_DIR)/secvar/Makefile.inc
 include $(SRC)/$(LIBSTB_DIR)/mbedtls/Makefile.inc
 include $(SRC)/$(LIBSTB_DIR)/drivers/Makefile.inc
 include $(SRC)/$(LIBSTB_DIR)/tss/Makefile.inc
 
-$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(MBEDTLS)
+$(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(SECVAR) $(MBEDTLS)
 
 libstb/create-container: libstb/create-container.c libstb/container-utils.c
 	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) \
diff --git a/libstb/secureboot.c b/libstb/secureboot.c
index 1578f52e..68c81ebf 100644
--- a/libstb/secureboot.c
+++ b/libstb/secureboot.c
@@ -170,6 +170,8 @@  void secureboot_init(void)
 	if (cvc_init())
 		secureboot_enforce();
 
+	if (platform.secvar_init)
+		platform.secvar_init();
 	secure_init = true;
 }
 
diff --git a/libstb/secvar/Makefile.inc b/libstb/secvar/Makefile.inc
new file mode 100644
index 00000000..75870910
--- /dev/null
+++ b/libstb/secvar/Makefile.inc
@@ -0,0 +1,14 @@ 
+# -*-Makefile-*-
+
+SECVAR_DIR = libstb/secvar
+
+SUBDIRS += $(SECVAR_DIR)
+
+include $(SECVAR_DIR)/storage/Makefile.inc
+include $(SECVAR_DIR)/backend/Makefile.inc
+
+SECVAR_SRCS = secvar_main.c secvar_util.c
+SECVAR_OBJS = $(SECVAR_SRCS:%.c=%.o)
+SECVAR = $(SECVAR_DIR)/built-in.a
+
+$(SECVAR): $(SECVAR_OBJS:%=$(SECVAR_DIR)/%) $(SECVAR_STORAGE) $(SECVAR_BACKEND)
diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
new file mode 100644
index 00000000..7a7ca1f7
--- /dev/null
+++ b/libstb/secvar/backend/Makefile.inc
@@ -0,0 +1,11 @@ 
+# -*-Makefile-*-
+
+SECVAR_BACKEND_DIR = libstb/secvar/backend
+
+SUBDIRS += $(SECVAR_BACKEND_DIR)
+
+SECVAR_BACKEND_SRCS =
+SECVAR_BACKEND_OBJS = $(SECVAR_BACKEND_SRCS:%.c=%.o)
+SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
+
+$(SECVAR_BACKEND): $(SECVAR_BACKEND_OBJS:%=$(SECVAR_BACKEND_DIR)/%)
diff --git a/libstb/secvar/secvar.h b/libstb/secvar/secvar.h
new file mode 100644
index 00000000..d02a0cef
--- /dev/null
+++ b/libstb/secvar/secvar.h
@@ -0,0 +1,71 @@ 
+/* 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 _SECVAR_H_
+#define _SECVAR_H_
+
+#include <ccan/list/list.h>
+#include <stdint.h>
+#include <secvar.h>
+
+#define SECVAR_MAX_KEY_LEN		1024
+#define SECVAR_MAX_METADATA_SIZE	1024
+#define SECVAR_MAX_DATA_SIZE		2048
+
+enum {
+	SECVAR_VARIABLE_BANK,
+	SECVAR_UPDATE_BANK,
+};
+
+
+struct secvar_node {
+	struct list_node link;
+	struct secvar *var;
+};
+
+#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
+
+enum {
+	BACKEND_NONE = 0,
+	BACKEND_TC_COMPAT_V1,
+};
+
+extern struct list_head variable_bank;
+extern struct list_head update_bank;
+extern int secvar_enabled;
+extern struct secvar_storage_driver secvar_storage;
+extern struct secvar_backend_driver secvar_backend;
+
+// Helper functions
+void clear_bank_list(struct list_head *bank);
+struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank);
+int is_key_empty(char *key, uint64_t key_len);
+int list_length(struct list_head *bank);
+
+#endif
diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c
new file mode 100644
index 00000000..4a3c97a8
--- /dev/null
+++ b/libstb/secvar/secvar_main.c
@@ -0,0 +1,89 @@ 
+/* 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) "SECVAR: " fmt
+#endif
+
+#include <stdlib.h>
+#include <string.h>
+#include <skiboot.h>
+#include <opal.h>
+#include "secvar.h"
+
+struct list_head variable_bank;
+struct list_head update_bank;
+int secvar_enabled = 0;
+
+// To be filled in by platform.secvar_init
+struct secvar_storage_driver secvar_storage = {0};
+struct secvar_backend_driver secvar_backend = {0};
+
+int secvar_main(struct secvar_storage_driver storage_driver,
+               struct secvar_backend_driver backend_driver)
+{
+	int rc = OPAL_UNSUPPORTED;
+
+	secvar_storage = storage_driver;
+	secvar_backend = backend_driver;
+
+	list_head_init(&variable_bank);
+	list_head_init(&update_bank);
+
+	rc = secvar_storage.store_init();
+	if (rc)
+		goto out;
+
+	if (secvar_backend.pre_process)
+		rc = secvar_backend.pre_process();
+	if (rc)
+		goto out;
+
+	rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	if (rc)
+		goto out;
+
+	rc = secvar_storage.load_bank(&update_bank, SECVAR_UPDATE_BANK);
+	if (rc)
+		goto out;
+
+	if (secvar_backend.process)
+		rc = secvar_backend.process();
+	if (rc)
+		goto out;
+
+	rc = secvar_storage.write_bank(&variable_bank, SECVAR_VARIABLE_BANK);
+	if (rc)
+		goto out;
+
+	rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK);
+	if (rc)
+		goto out;
+
+	// TODO: maybe this should just be part of .process()?
+	if (secvar_backend.post_process)
+		rc = secvar_backend.post_process();
+	if (rc)
+		goto out;
+
+	secvar_enabled = 1;
+
+	return OPAL_SUCCESS;
+
+out:
+	printf("Secure Variables Status %04x\n", rc);
+	return rc;
+}
diff --git a/libstb/secvar/secvar_util.c b/libstb/secvar/secvar_util.c
new file mode 100644
index 00000000..5d79c991
--- /dev/null
+++ b/libstb/secvar/secvar_util.c
@@ -0,0 +1,88 @@ 
+/* 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) "SECVAR: " fmt
+#endif
+
+#include <stdlib.h>
+#include <string.h>
+#include <skiboot.h>
+#include <opal.h>
+#include "secvar.h"
+
+void clear_bank_list(struct list_head *bank)
+{
+	struct secvar_node *node, *next;
+
+	if (!bank)
+		return;
+
+	list_for_each_safe(bank, node, next, link) {
+		list_del(&node->link);
+
+		if (!node->var) {
+			free(node);
+			continue;
+		}
+
+		if (node->var->flags & SECVAR_FLAG_DYN_ALLOC) {
+			free(node->var);
+		}
+
+		free(node);
+	}
+}
+
+struct secvar_node *find_secvar(char *key, uint64_t key_len, struct list_head *bank)
+{
+	struct secvar_node *node = NULL;
+
+	list_for_each(bank, node, link) {
+		// Prevent matching shorter key subsets / bail early
+		if (key_len != node->var->key_len)
+			continue;
+		if (!memcmp(key, node->var->key, key_len)) {
+			return node;
+		}
+	}
+
+	return NULL;
+}
+
+
+int is_key_empty(char *key, uint64_t key_len)
+{
+	int i;
+	for (i = 0; i < key_len; i++) {
+		if (key[i] != 0)
+			return 0;
+	}
+
+	return 1;
+}
+
+int list_length(struct list_head *bank)
+{
+	int ret = 0;
+	struct secvar_node *node;
+
+	list_for_each(bank, node, link) {
+		ret++;
+	}
+
+	return ret;
+}
diff --git a/libstb/secvar/storage/Makefile.inc b/libstb/secvar/storage/Makefile.inc
new file mode 100644
index 00000000..c107736e
--- /dev/null
+++ b/libstb/secvar/storage/Makefile.inc
@@ -0,0 +1,11 @@ 
+# -*-Makefile-*-
+
+SECVAR_STORAGE_DIR = libstb/secvar/storage
+
+SUBDIRS += $(SECVAR_STORAGE_DIR)
+
+SECVAR_STORAGE_SRCS =
+SECVAR_STORAGE_OBJS = $(SECVAR_STORAGE_SRCS:%.c=%.o)
+SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
+
+$(SECVAR_STORAGE): $(SECVAR_STORAGE_OBJS:%=$(SECVAR_STORAGE_DIR)/%)