[Skiboot] [PATCH 2/7] libstb/secvar: add secure variable internal abstraction
Stewart Smith
stewart at linux.ibm.com
Fri Jun 14 14:27:42 AEST 2019
Eric Richter <erichte at 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 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
--
Stewart Smith
OPAL Architect, IBM.
More information about the Skiboot
mailing list