[Skiboot] [RFC PATCH 4/7] secvar/storage: add draft secvar storage driver for pnor-based p9 platforms
Stewart Smith
stewart at linux.ibm.com
Fri Jun 14 16:05:26 AEST 2019
Eric Richter <erichte at 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 at 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?
--
Stewart Smith
OPAL Architect, IBM.
More information about the Skiboot
mailing list