[Skiboot] [RFC PATCH RESEND 05/10] keystore: initialize the base keystore structure
Stewart Smith
stewart at linux.ibm.com
Mon Mar 4 18:06:53 AEDT 2019
Eric Richter <erichte at linux.ibm.com> writes:
> This patch introduces and initializes the structures for representing
> the keystore in memory. Also included is the logic for parsing the
> secboot partition data into the in-memory keystore.
>
> The keystore is kept in-memory for a few reasons. First, we don't want
> to be reloading from PNOR whenever we want to retrieve a key, as the
> underlying PNOR section may have been overwritten without validation.
> The second point is a matter of performance -- by keeping things in
> memory it reduces the amount of slow I/O operations on the chip, as well
> as reducing the amount of wear. Finally, it is much simpler from a
> programming perspective to manipulate a list in-memory than to be
> constantly operating on a blob of binary data that would need to be
> reparsed each time, although this does consume more memory.
>
> There are two parts to the in-memory keystore: the Active Bank, and the
> Update Queue. The Active Bank is the list of keys to be operated on. As
> the name implies, the Update Queue is a queue of updates to be applied
> to the Active Bank (though the actual processing of this is left to the
> kernel[1]). For simplicity and sake of code reuse, the Active Bank and
> the Update Queue use the exact same format, and therefore can be loaded
> in the exact same manner -- the only difference is the offset that they
> are loaded from in the secboot partition.
>
> [1] Implementations of the update queue processing code in the kernel
> are forthcoming, we have an internal version that is not quite ready for
> upstream.
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> libstb/Makefile.inc | 2 +-
> libstb/keystore.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++
> libstb/keystore.h | 38 +++++++++++++++++++++++++++
> libstb/secboot_part.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
> libstb/secboot_part.h | 1 +
> libstb/secureboot.c | 4 +++
> 6 files changed, 182 insertions(+), 1 deletion(-)
> create mode 100644 libstb/keystore.c
> create mode 100644 libstb/keystore.h
>
> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
> index 5cd6c2ee..6cc30134 100644
> --- a/libstb/Makefile.inc
> +++ b/libstb/Makefile.inc
> @@ -4,7 +4,7 @@ LIBSTB_DIR = libstb
>
> SUBDIRS += $(LIBSTB_DIR)
>
> -LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c secboot_part.c
> +LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c secboot_part.c keystore.c
> LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
> LIBSTB = $(LIBSTB_DIR)/built-in.a
>
> diff --git a/libstb/keystore.c b/libstb/keystore.c
> new file mode 100644
> index 00000000..7d6027ef
> --- /dev/null
> +++ b/libstb/keystore.c
> @@ -0,0 +1,71 @@
> +/* Copyright 2013-2017 IBM Corp.
Include current year.
> + *
> + * 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) "KEYSTORE: " fmt
> +#endif
> +
> +#include <skiboot.h>
> +#include <opal.h>
> +#include <device.h>
> +#include "container.h"
> +#include "cvc.h"
> +#include "keystore.h"
> +#include "secboot_part.h"
> +
> +static struct list_head active_bank_list = LIST_HEAD_INIT(active_bank_list);
> +static struct list_head update_queue_list = LIST_HEAD_INIT(update_queue_list);
> +
> +static bool keystore_ready = false; /* has the keystore been loaded? */
> +
> +// TODO: OPAL_UNSUPPORTED?
> +#define CHECK_KEYSTORE_READY if(!keystore_ready) {prlog(PR_ERR,
> "Ignoring call, keystore not ready\n"); return OPAL_RESOURCE; }
Skip the prlog() and just open code if(!keystore_ready) return
OPAL_RESOURCE at the start of each OPAL API.
also be sure to document each
> +
> +
> +
> +int keystore_init(void)
> +{
> + int rc;
> +
> + /* software secure boot supported only on p9 and above */
> + if (proc_gen != proc_gen_p9) {
> + prlog(PR_INFO, "Secureboot is not supported on this platform\n");
> + return 0;
> + }
Why are we restricting it to P9 and above? Is there any reason why
somebody couldn't ship this on P8 if they wanted to?
> +
> + /* API init */
> + list_head_init(&active_bank_list);
> + list_head_init(&update_queue_list);
> +
> + rc = secboot_part_init();
> + if (rc) {
> + prlog(PR_ERR, "Failed to initialize the secboot partition\n");
> + return -1;
> + }
> +
> + /* add variables we have in pnor secboot */
> + rc = secboot_part_build_keystore(&active_bank_list, &update_queue_list);
> + if (rc) {
> + prlog(PR_ERR, "Failed to build the keystore\n");
> + return -1;
> + }
> +
> + keystore_ready = true;
> +
> + prlog(PR_INFO, "Keystore initialized successfully\n");
PR_DEBUG.
> +
> + return 0;
> +}
> diff --git a/libstb/keystore.h b/libstb/keystore.h
> new file mode 100644
> index 00000000..e2eb296c
> --- /dev/null
> +++ b/libstb/keystore.h
> @@ -0,0 +1,38 @@
> +/* Copyright 2013-2014 IBM Corp.
I'm guessing this came from somewhere else and has in fact been edited
in a year after 2014. If so, update year.
> + *
> + * 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 __KEYSTORE_H
> +#define __KEYSTORE_H
> +
> +// TODO: Assign these constants more intelligently
> +#define ACTIVE_BANK 0x1
> +#define UPDATE_QUEUE 0x2
> +#define CLEAR_QUEUE 0x4
> +#define START_OVER 0x8
> +
> +int keystore_init(void);
> +
> +// TODO: figure out how this differs from secboot_section_item
> +// Struct for bank lists
> +struct keystore_variable {
> + struct list_node link;
> + uint64_t name_size;
> + uint64_t data_size;
> + char *name;
> + char *data;
> +};
> +
> +#endif /* __KEYSTORE_H */
> diff --git a/libstb/secboot_part.c b/libstb/secboot_part.c
> index e6558166..9a2c8ac0 100644
> --- a/libstb/secboot_part.c
> +++ b/libstb/secboot_part.c
> @@ -20,6 +20,7 @@
>
> #include <skiboot.h>
> #include "secboot_part.h"
> +#include "keystore.h"
>
> struct secboot_section_item {
> const char *name; /* null terminated string */
> @@ -164,6 +165,72 @@ static int secboot_part_validate(void)
> }
>
>
> +static int read_section(struct list_head *bank, char *cur, char *end)
> +{
> + struct keystore_variable *var;
> +
> + while (cur < end) {
> + // Bail if variable name size is empty -- end of list
> + if (*((uint64_t*) cur) == 0llu) {
> + prlog(PR_INFO, "variable name size is empty, bailing\n");
> + break;
> + }
> +
> + var = (struct keystore_variable*) malloc(sizeof(struct keystore_variable));
> + if (!var) {
> + return -1;
> + }
> +
> + var->name_size = *((uint64_t*) cur); cur += sizeof(uint64_t);
> + var->data_size = *((uint64_t*) cur); cur += sizeof(uint64_t);
> + var->name = malloc(var->name_size);
This smells like "DANGER WILL ROBINSON" to me.
> + if (!var->name) {
> + free(var);
> + return -2;
> + }
> + var->data = malloc(var->data_size);
> + if (!var->data) {
> + free(var->name);
> + free(var);
> + return -3;
> + }
> +
> + memcpy(var->name, cur, var->name_size);
> + cur += var->name_size;
> + memcpy(var->data, cur, var->data_size);
> + cur += var->data_size;
> +
> + prlog(PR_INFO, "loaded: '%s'\n", var->name);
Don't need to display this on console
> +
> + list_add_tail(bank, &var->link);
> + }
> +
> + return 0;
> +}
All of this will need solid unit tests and the ability to be fuzzed with
afl-lop.
What is the format of what's going in these sections? It should, if at
all possible, borrow from somewhere else rather than invent its own
thing, and it should also be clearly documented in doc/
> +int secboot_part_build_keystore(struct list_head *active, struct list_head *uqueue)
> +{
> + int ret = -1;
> +
> + if (!secboot_valid)
> + return -1;
> +
> + ret = read_section(active, (char*) active_bank->item, ((char*) active_bank) + SECBOOT_BANK_SIZE);
> + if (ret) {
> + prlog(PR_ERR, "Error loading Active Bank: %d\n", ret);
> + }
> +
> + ret = read_section(uqueue, (char*) update_queue->item, ((char*) update_queue) + SECBOOT_QUEUE_SIZE);
> + if (ret) {
> + prlog(PR_ERR, "Error loading update queue: %d\n", ret);
> + }
> +
> + return ret;
> +
> +}
> +
> +
> int secboot_part_init()
> {
> int rc;
> diff --git a/libstb/secboot_part.h b/libstb/secboot_part.h
> index 7f29183b..66e71561 100644
> --- a/libstb/secboot_part.h
> +++ b/libstb/secboot_part.h
> @@ -18,5 +18,6 @@
> #define __SECBOOT_PART_H
>
> int secboot_part_init(void);
> +int secboot_part_build_keystore(struct list_head* active, struct list_head* uqueue);
>
> #endif /* __SECBOOT_PART_H */
> diff --git a/libstb/secureboot.c b/libstb/secureboot.c
> index 4f6a301d..c6b72b5d 100644
> --- a/libstb/secureboot.c
> +++ b/libstb/secureboot.c
> @@ -24,6 +24,7 @@
> #include <opal-api.h>
> #include <inttypes.h>
> #include "secureboot.h"
> +#include "keystore.h"
>
> static const void* hw_key_hash = NULL;
> static size_t hw_key_hash_size;
> @@ -171,6 +172,9 @@ void secureboot_init(void)
> secureboot_enforce();
>
> secure_init = true;
> +
> + keystore_init();
> +
> }
>
> int secureboot_verify(enum resource_id id, void *buf, size_t len)
> --
> 2.14.4
>
> _______________________________________________
> 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