[Skiboot] [RFC PATCH 4/7] secvar/storage: add draft secvar storage driver for pnor-based p9 platforms
Eric Richter
erichte at linux.ibm.com
Sat Jun 15 06:10:08 AEST 2019
On 6/14/19 1:05 AM, Stewart Smith wrote:
> 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
Minimum size would fit well if we consider what is the minimum number of
variables a particular authentication schema would require.
> >> +
>> +
>> +/* 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?
>
List of secvar structs. They should probably be
struct secvar bank0[], using some ratio for the max number of variables
that can be stored.
However, this nice struct layout may go away if the size of the partition
affects the offsets of where the banks are.
>> +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.
>
Changing the struct to be directly serialized was a conscious decision to
reduce the complexity of the storage serialization logic to a single memcpy.
Ideally, the variables don't even need to be copied out of the secboot_image
buffer, and a variable bank item can point directly to where it is in the
buffer (though, that proved to be too complex for this rfc).
Downside of course is the large amount of memory required per variable. The
numbers for both the max size and the partition sizes are largely place-
holders while we figure out what appropriate sizes are.
However, it is unlikely that we will need more variables than that anyway
>> +
>> + 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?
>
Parse is more accurate, though this is the function that is ultimately called
in the `load_bank` driver hook. Currently matches that, but can be changed
to be more descriptive.
>> +{
>> + 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?
>
Hash validation would occur in the _store_init() function, which is called before
loading the banks.
>> + 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?
>
No, I've been toying with secboot_tpm_store_, largely since those are
the two storage locations that would be in use.
> Why would we not also just reuse the serialisation if we did in fact
> have lockable flash on a future chip?
>
If we had an abundance of lockable flash in the future, only the update
bank would need to be written to PNOR. All variables could instead be
written to the lockable flash, which would be locked after processing
updates. Some of the logic may be reusable, but the location would be
different.
More information about the Skiboot
mailing list