[Skiboot] [RFC 3/6] libstb: add secure variable internal abstraction
Eric Richter
erichte at linux.vnet.ibm.com
Tue Apr 2 05:37:49 AEDT 2019
On 3/28/19 8:51 PM, Oliver wrote:
> On Fri, Mar 29, 2019 at 9:18 AM Eric Richter <erichte at linux.ibm.com> wrote:
>>
>> 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.
>>
>> This abstraction utilizes two linked lists ("banks") for storing the
>> variables in memory for other components (such as the runtime service API)
>> to access.
>>
>> Platform specific implementations must implement five functions, all of
>> which operate on, or enumerate a bank linked list:
>>
>> - a platform-specific init() function (see next patch)
>> - secvar_load_variable_bank()
>> - secvar_load_update_bank()
>> - secvar_write_variable_bank()
>> - secvar_write_update_bank()
>>
>> The secvar_load functions operate as the name implies -- they should
>> fill a supplied and initialized bank linked list head with secure variables
>> as loaded from that platform's storage method. Likewise, the secvar_write
>> functions should perform the inverse -- take a supplied bank list head,
>> and write that to the platform's storage method. On P9, this would be
>> the SECBOOT partition in PNOR.
>>
>> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
>> ---
>> libstb/Makefile.inc | 2 +-
>> libstb/secvar.c | 150 ++++++++++++++++++++++++++++++++++++++++++++
>> libstb/secvar.h | 85 +++++++++++++++++++++++++
>> 3 files changed, 236 insertions(+), 1 deletion(-)
>> create mode 100644 libstb/secvar.c
>> create mode 100644 libstb/secvar.h
>>
>> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
>> index 6d54c5cd..2bc9e91b 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
>> +LIBSTB_SRCS = container.c tpm_chip.c cvc.c secureboot.c trustedboot.c secvar.c
>> LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
>> LIBSTB = $(LIBSTB_DIR)/built-in.a
>>
>> diff --git a/libstb/secvar.c b/libstb/secvar.c
>> new file mode 100644
>> index 00000000..e1bc463f
>> --- /dev/null
>> +++ b/libstb/secvar.c
>> @@ -0,0 +1,150 @@
>> +/* 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 "secvar.h"
>> +
>> +struct list_head variable_bank;
>> +struct list_head update_bank;
>> +int secvar_enabled = 0;
>> +
>> +// Calculate length of a char16_t, with a max possible bytes
>> +// if end of string is not found before max bytes, returns -1
>> +// otherwise, returns length
>> +int str16nlen(char16_t *str, size_t maxbytes)
>> +{
>> + int ret = 0;
>> + char16_t *c = str;
>> +
>> + while ((c - str) < maxbytes) {
>> + if (*c == 0)
>> + return ret;
>> + ret++;
>> + c++;
>> + }
>> +
>> + return -1;
>> +}
>> +
>> +
>> +void clear_bank_list(struct list_head *bank)
>> +{
>> + struct secvar *node, *next;
>> +
>> + if (!bank)
>> + return;
>> +
>> + list_for_each_safe(bank, node, next, link) {
>> + if (node->data)
>> + free(node->data);
>> + list_del(&node->link);
>> + free(node);
>> + }
>> +}
>> +
>> +struct secvar *find_secvar_by_name_vendor(char16_t *name, guid_t *vendor, struct list_head *bank)
>> +{
>> + struct secvar *var = NULL;
>> + int len;
>> +
>> + len = str16nlen(name, SECVAR_MAX_NAME_SIZE);
>> + if (len <= 0) {
>> + return NULL;
>> + }
>> +
>> + list_for_each(bank, var, link) {
>> + if (!memcmp(name, var->name, len*2) &&
>> + !memcmp(vendor, var->vendor, sizeof(guid_t))) {
>> + return var;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +
>> +// Returns the packed physical size of a variable
>> +size_t sizeof_secvar(struct secvar *var)
>> +{
>> + return sizeof(var->name) +
>> + sizeof(var->vendor) +
>> + sizeof(var->attributes) +
>> + sizeof(var->data_size) +
>> + var->data_size;
>> +
>> +}
>> +
>> +
>> +// To be set by the platform init function
>> +// candidate for moving to platform struct
>> +int (*secvar_write_variable_bank)(struct list_head *variable_bank);
>> +int (*secvar_write_update_bank)(struct list_head *update_bank);
>> +int (*secvar_load_variable_bank)(struct list_head *variable_bank);
>> +int (*secvar_load_update_bank)(struct list_head *update_bank);
>> +
>> +int secvar_init(void)
>> +{
>> + int ret;
>> +
>
>> + secvar_enabled = 0;
>
> Isn't this already initialised to zero?
>
>
>> + prlog(PR_DEBUG, "Initializing secure variables\n");
>> + // Initialize platform storage, etc first
>> + switch (proc_gen) {
>> + default:
>> + prlog(PR_INFO, "Platform does not support secure boot, skipping...\n");
>> + return 1;
>> + }
>
> Tying this to the processor generation doesn't make a whole lot of
> sense to me. You've already defined platform specific functions to
> access the secure boot storage area so why not use the presence of
> those to determine if secure variables are supported?
>
I agree, especially if the platform specific functions are moved to the
platform structure. This was an artifact from an older version that
predated the platform specific logic.
>> + if (ret) {
>> + return ret;
>> + }
> you're using an un-initialised variable.
>
>> + list_head_init(&variable_bank);
>> + list_head_init(&update_bank);
>
> You can use LIST_HEAD() to staticly initialise these. For example in
> core/i2c.c we initialise the global bus list with:
>
> static LIST_HEAD(i2c_bus_list);
>
>> +
>> + // This should be set by the platform init function
>> + if ((!secvar_load_variable_bank) || (!secvar_load_update_bank)) {
>> + prlog(PR_ERR, "Failed to load from storage, secvar load functions not set!\n");
>> + return -1;
>
> Return a useful OPAL_<blah> code or make the function void.
>
>> + }
>> +
>> + ret = secvar_load_variable_bank(&variable_bank);
>> + if (ret)
>> + return ret;
>> + prlog(PR_DEBUG, "Loaded variable bank\n");
>> +
>> + ret = secvar_load_update_bank(&update_bank);
>> + if (ret)
>> + return ret;
>> + prlog(PR_DEBUG, "Loaded updates, ");
>> + if (list_empty(&update_bank))
>> + prlog(PR_DEBUG, "none to apply.\n");
>> + else
>> + prlog(PR_DEBUG, "found updates to apply\n");
>> +
>> + // TODO: Process update queue here
>> + // TODO: Derive SecureMode, etc variables here
>> +
>> + secvar_enabled = 1;
>> +
>> + return 0;
>> +}
>> diff --git a/libstb/secvar.h b/libstb/secvar.h
>> new file mode 100644
>> index 00000000..29d3cf06
>> --- /dev/null
>> +++ b/libstb/secvar.h
>> @@ -0,0 +1,85 @@
>> +/* 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>
>> +
>> +typedef uint16_t char16_t;
>> +
>> +#define SECVAR_MAX_NAME_SIZE 1024
>> +#define SECVAR_MAX_NAME_LEN (SECVAR_MAX_NAME_SIZE/sizeof(char16_t))
>> +#define SECVAR_MAX_DATA_SIZE 2048
>> +#define SECVAR_VENDOR_SIZE 16
>> +
>> +
>> +enum {
>> + SECVAR_VARIABLE_BANK,
>> + SECVAR_UPDATE_BANK,
>> +};
>> +
>> +
>> +typedef unsigned char guid_t[SECVAR_VENDOR_SIZE];
>> +
>> +struct secvar {
>> + struct list_node link;
>> + char16_t name[SECVAR_MAX_NAME_LEN];
>> + guid_t vendor;
>> + uint32_t attributes;
>> + uint32_t data_size;
>> + char *data;
>> +};
>> +
>> +extern struct list_head variable_bank;
>> +extern struct list_head update_bank;
>> +extern int secvar_enabled;
>> +
>> +// Helper functions
>> +int str16nlen(char16_t *str, size_t max);
>> +void clear_bank_list(struct list_head *bank);
>> +struct secvar *find_secvar_by_name_vendor(char16_t *name, guid_t *vendor, struct list_head *bank);
>> +size_t sizeof_secvar(struct secvar *var);
>>
>> +extern int (*secvar_write_variable_bank)(struct list_head *variable_bank);
>> +extern int (*secvar_write_update_bank)(struct list_head *update_bank);
>> +extern int (*secvar_load_variable_bank)(struct list_head *variable_bank);
>> +extern int (*secvar_load_update_bank)(struct list_head *update_bank);
>
> Wierd API. Is the implementation of the update and variable cases
> going to be all that different? Seems like it would be cleaner to have
> one read/write function and pass in the bank type as an argument.
>
Unfortunately, yes that's a quirk that isn't included in this set, but
should have been mentioned. On P9 for example, we need to "write lock"
the variable bank during this call by storing a hash of the variable
bank in the TPM NV space. Writing to the update bank does not require
this, however.
Similarly, loading from the variable bank will have the extra step of
validating the loaded bank data against the hash stored in the TPM.
That said, I agree there is room for improvement here. I've considered
either removing the arguments entirely (as they are accessible from the
platform code anyway), or merging the functions into read/write and
using a flag to control which bank to read/write from/to.
> Also is there a good reason not to put this directly in the platform
> structure? Mystery callbacks that get set at runtime are mostly just
> annoying in my experience.
>
I wasn't sure if they had belonged there, however it does seem like it
is a better place for them.
>> +
>> +int secvar_init(void);
>> +
>
>> +// From upstream ccan
>> +#define list_typeof(var) typeof(var)
>> +
>> +static inline void *list_entry_or_null(const struct list_head *h,
>> + const struct list_node *n,
>> + size_t off)
>> +{
>> + if (n == &h->n)
>> + return NULL;
>> + return (char *)n - off;
>> +}
>> +
>> +#define list_next(h, i, member) \
>> + ((list_typeof(i))list_entry_or_null(list_debug(h), \
>> + (i)->member.next, \
>> + list_off_var_((i), member)))
>> +
>> +
>> +
>
> Put these in ccan/list/list.h in a seperate patch and include the
> documentation from upstream ccan. There's no reason to have random
> bits of generic list code peppered around the tree.
>
Adding these to ccan/list/list.h will deviate from the actual upstream
ccan list.h, is that acceptable? I attempted to update list.h to
upstream's version, however that caused breakages elsewhere in skiboot,
hence why I only extracted what I needed.
If that is fine, then I will move these to list.h.
>> +#endif
>> --
>> 2.17.2
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>
More information about the Skiboot
mailing list