[Skiboot] [RFC PATCH RESEND 04/10] libstb: initialize and format pnor secboot partition
Stewart Smith
stewart at linux.ibm.com
Mon Mar 4 17:48:05 AEDT 2019
Eric Richter <erichte at linux.ibm.com> writes:
> From: Claudio Carvalho <cclaudio at linux.ibm.com>
>
> The SECBOOT partition layout has three subsections of the same type. The
> first one stores active variables. The second stores staging variables,
> those that will become active in the next full reboot. Lastly, the third
> stores updates for the active variables, which are processed by the
> skiroot kernel at boot time.
These concepts should exist in something in doc/
At least in my database-brain, I read this currently as a relatively
non-standard way to say "data file, doublewrite-buffer, and redo log".
>
> This defines the SECBOOT partition layout and also formats the partition
> with default values if it hasn't been formatted yet.
What are the semantics r.e. a corrupt looking partition? If we're doing
boot-to-an-OS Secure Boot, a corrupt SECBOOT partition effectively
semi-bricks the machine as you won't be able to boot an OS kernel.
Should we revert to a default set of keys in this event?
We should certainly have a well defined mechanism to make the user aware
of this situation, as the symptom is likely to be the machine is now
sitting at Petitboot and not booting their OS.
Thus, unlike the NVRAM partition, I think we should *explicitly* state
that on a fresh flash or re-provision, this partition *must* be
formatted and contain valid data.
>
> NOTE1: In order to determine the subsections size, this assumes that the
> SECBOOT partition has the same size in all platforms that supports OS
> secure boot.
I don't think this is an assumption we should make. We should likely
enforce a *minimum* for platforms, but perhaps this will grow over time
and we should be able to handle it.
> NOTE2: The first two sub-sections are used to store variable blobs
> (a.k.a variable banks). Sometimes one bank acts as the active, sometimes
> it acts as the staging. Currently, the active and staging pointers point
> to the same bank, but in future there should be a selector bit in TPM NV
> to indicate which bank is the active.
Why switch which one is active? I can think of pretty simple semantics
for a datafile/doublewrite-buffer/redo-log based scheme where we always
have "this bank is the active one" and don't need to store that in the
TPM (just the hash of what's valid)
Why do we need it this way that's more complex?
> NOTE3: For simplicity, this loads the SECBOOT partition as a whole at
> boot time, although the staging bank is not consumed.
>
> Signed-off-by: Claudio Carvalho <cclaudio at linux.ibm.com>
> ---
> libstb/Makefile.inc | 2 +-
> libstb/secboot_part.c | 249 ++++++++++++++++++++++++++++++++++++++++++++++++++
> libstb/secboot_part.h | 22 +++++
> 3 files changed, 272 insertions(+), 1 deletion(-)
> create mode 100644 libstb/secboot_part.c
> create mode 100644 libstb/secboot_part.h
>
> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
> index 6d54c5cd..5cd6c2ee 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
> secboot_part.c
Or should it be called keystore?
> LIBSTB_OBJS = $(LIBSTB_SRCS:%.c=%.o)
> LIBSTB = $(LIBSTB_DIR)/built-in.a
>
> diff --git a/libstb/secboot_part.c b/libstb/secboot_part.c
> new file mode 100644
> index 00000000..e6558166
> --- /dev/null
> +++ b/libstb/secboot_part.c
Much like core/test/run-nvram-format.c, I think this needs solid and
extensive unit tests. Since we'll also need to provision a machine with
keys in the keystore, we'll likely need a command line utility for
manipulating the content of the partition.
> @@ -0,0 +1,249 @@
> +/* Copyright 2013-2018 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_PART: " fmt
> +#endif
> +
> +#include <skiboot.h>
> +#include "secboot_part.h"
> +
> +struct secboot_section_item {
> + const char *name; /* null terminated string */
> + uint32_t size;
> + char data[0];
> +} __packed;
How does this structure work as an on-disk format with a pointer to a
string that's somewhere else in memory?
> +
> +struct secboot_section_header {
> + uint8_t version;
> + uint32_t reserved;
> + uint32_t size;
> +} __packed;
> +
> +struct secboot_section {
> + struct secboot_section_header header;
> + struct secboot_section_item item[0]; // section_item place holder
> + // followed by other section items up to section size:
> + // SECBOOT_BANK_SIZE if it is bank section; or
> + // SECBOOT_QUEUE_SIZE if it is an update queue section
> +} __packed;
> +
> +struct secboot_header {
> + uint32_t magic_number;
> + uint8_t version;
> +} __packed;
> +
> +struct secboot_part {
> + struct secboot_header header;
> + struct secboot_section bank[0]; // section_header place holder
> + // followed by bank1 section
> + // followed by update queue section
> +} __packed;
> +
> +//#define SECBOOT_SECTION_SIZE(p) (sizeof(section_header) + p->size)
> +/* 0x5053424b = "PSBK" or Power Secure Boot Keystore */
> +#define SECBOOT_MAGIC_NUMBER 0x5053424b
> +#define SECBOOT_VERSION 1
> +#define SECBOOT_MINIMUM_SIZE (sizeof(struct secboot_header) + SECBOOT_BANK_SIZE*2 + SECBOOT_QUEUE_SIZE)
> +
> +#define SECBOOT_BANK_VERSION 1
> +#define SECBOOT_BANK_SIZE 0xb400 // 45 KB
> +
> +#define SECBOOT_QUEUE_VERSION 1
> +#define SECBOOT_QUEUE_SIZE 0x7800 // 30 KB
Are these erase block aligned? Always?
If not, then we lose crash safety during update, which is probably
explicitly what we do *not* want.
> +static struct secboot_part *secboot = NULL;
> +static uint32_t secboot_size = 0;
> +
> +/* Indicates if the secboot partition data can be consumed */
> +static bool secboot_valid = false;
> +
> +static struct secboot_section *active_bank = NULL;
> +static struct secboot_section *staging_bank = NULL;
> +static struct secboot_section *update_queue = NULL;
> +
> +static int secboot_part_format(void)
> +{
> + struct secboot_section *section;
> + int rc;
> +
> + if (!platform.secboot_write)
> + return -1;
> +
> + memset(secboot, 0, secboot_size);
> +
> + /* INITIALIZE THE HEADERS */
> +
> + /* secboot header */
> + secboot->header.magic_number = SECBOOT_MAGIC_NUMBER;
> + secboot->header.version = SECBOOT_VERSION;
> +
> + /* bank0 section header */
> + section = secboot->bank;
> + section->header.version = SECBOOT_BANK_VERSION;
> + section->header.reserved = 0;
> + section->header.size = SECBOOT_BANK_SIZE;
> +
> + /* bank1 section header */
> + section = (struct secboot_section*)((uint8_t*) section + SECBOOT_BANK_SIZE);
> + section->header.version = SECBOOT_BANK_VERSION;
> + section->header.reserved = 0;
> + section->header.size = SECBOOT_BANK_SIZE;
> +
> + /* update queue section header */
> + section = (struct secboot_section*)((uint8_t*) section + SECBOOT_BANK_SIZE);
> + section->header.version = SECBOOT_QUEUE_VERSION;
> + section->header.reserved = 0;
> + section->header.size = SECBOOT_QUEUE_SIZE;
> +
> + /* write the whole thing back to PNOR */
> + rc = platform.secboot_write(0, secboot, secboot_size);
> + if (rc)
> + return -1;
> +
> + prlog(PR_INFO, "formatted\n");
This likely doesn't need be in the log at PR_INFO level.
> + secboot_valid = true;
> +
> + return 0;
> +}
> +
> +/*
> + * Check that the keystore layout is sane. If not, we re-format the lot of it
> + */
> +static int secboot_part_validate(void)
> +{
> + if (secboot->header.magic_number != SECBOOT_MAGIC_NUMBER) {
> + prlog(PR_ERR, "magic number 0x%x mismatch, expected 0x%x\n",
> + secboot->header.magic_number, SECBOOT_MAGIC_NUMBER);
> + return -1;
> + }
> + if (secboot->header.version != SECBOOT_VERSION) {
> + prlog(PR_ERR, "secboot version %d not supported, expected %d\n",
> + secboot->header.version, SECBOOT_VERSION);
> + return -1;
> + }
is reformatting on incorrect version a good idea? This means that if we
upgrade the format, then you can never go back to an older firmware
version, and if you do, we delete data you've put there.
Again, I think semantics should be clearly documented in doc/
> +
> + /*
> + * No need to check the staging bank version, we may overwrite it at
> + * runtime but we don't consume it
> + */
> +
> + active_bank = secboot->bank;
> + if (active_bank->header.version != SECBOOT_BANK_VERSION) {
> + prlog(PR_ERR, "active bank version %d not supported, expected %d\n",
> + active_bank->header.version, SECBOOT_BANK_VERSION);
> + return -1;
> + }
> +
> + staging_bank = active_bank;
> + update_queue = (struct secboot_section*)((uint8_t*) secboot->bank + secboot->bank->header.size);
> + update_queue = (struct secboot_section*)((uint8_t*) update_queue + update_queue->header.size);
> +
> + if (update_queue->header.version != SECBOOT_QUEUE_VERSION) {
> + prlog(PR_ERR, "update queue version %d not supported, expected %d\n",
> + update_queue->header.version, SECBOOT_QUEUE_VERSION);
> + return -1;
> + }
Is this a reformat everything event or just a "continue if things are valid"?
> +
> + prlog(PR_INFO, "secboot partition valid\n");
I don't think this needs to go to the console.
> + secboot_valid = true;
> + return 0;
> +}
> +
> +
> +int secboot_part_init()
> +{
> + int rc;
> +
> + /* Already initialized */
> + if (secboot)
> + return 0;
r.e. naming, we already have stb_init() (err.. secureboot_init()) so
also having something clalled secboot_part_init() that's separate may
not be too intuitive.
> +
> + if (!platform.secboot_info)
> + return -1;
> + rc = platform.secboot_info(&secboot_size);
> + if (rc) {
> + prlog(PR_ERR, "error %d retrieving keystore info\n", rc);
> + return -1;
> + }
> + if (SECBOOT_MINIMUM_SIZE > secboot_size) {
> + prlog(PR_ERR, "secboot partition %d KB too small. min=%ld\n",
> + secboot_size >> 10, SECBOOT_MINIMUM_SIZE);
> + return -1;
> + }
> +
> + prlog(PR_INFO, "size is %d KB\n", secboot_size >> 10);
> +
> + /*
> + * We allocate the pnor secboot image with 4k alignment to make the
> + * FSP backend job's easier
> + */
> + secboot = memalign(0x1000, secboot_size);
> + if (!secboot) {
> + prlog(PR_ERR, "failed to allocate the secboot partition buffer\n");
> + secboot_size = 0;
> + return -1;
> + }
> +
> + /* Read it in */
> + rc = platform.secboot_read(secboot, 0, secboot_size);
> + if (rc) {
> + prlog(PR_ERR, "failed to read the secboot partition, rc=%d\n", rc);
> + goto out_free;
> + }
> +
> + /*
> + * TODO:
> + * 1. Can we assume that the secboot partition is zeroed out in
> MFG?
Well, it'd be interesting to look at what SECBOOT is initialised to
today, and even then, the strategy of having it always initialised to
something sane does have benefits.
> + */
> + if (secboot->header.magic_number == 0xffffffff ||
> + secboot->header.version == 0xff ||
> + secboot->header.magic_number == 0x00000000) {
> + rc = secboot_part_format();
> + if (rc)
> + goto out_free;
> + }
> +
> + rc = secboot_part_validate();
> + if (rc)
> + goto out_free;
> +
> + /* secboot header */
> + prlog(PR_INFO, "secboot->magic_number 0x%x\n", secboot->header.magic_number);
> + prlog(PR_INFO, "secboot->version %d\n", secboot->header.version);
> + /* active bank header */
> + prlog(PR_INFO, "active->version %d\n", active_bank->header.version);
> + prlog(PR_INFO, "active->reserved 0x%x\n", active_bank->header.reserved);
> + prlog(PR_INFO, "active->size %d KB\n", active_bank->header.size >> 10);
> + /* staging bank header */
> + prlog(PR_INFO, "staging->version %d\n", staging_bank->header.version);
> + prlog(PR_INFO, "staging->reserved 0x%x\n", staging_bank->header.reserved);
> + prlog(PR_INFO, "staging->size %d KB\n", staging_bank->header.size >> 10);
> + /* update queue header */
> + prlog(PR_INFO, "queue->version %d\n", update_queue->header.version);
> + prlog(PR_INFO, "queue->reserved 0x%x\n", update_queue->header.reserved);
> + prlog(PR_INFO, "queue->size %d KB\n", update_queue->header.size
> >> 10);
None of that needs to go to the console during boot. PR_TRACE maybe? IF
at all needed.
> +
> + return 0;
> +
> +out_free:
> + if (secboot) {
> + free(secboot);
> + secboot = NULL;
> + secboot_size = 0;
> + }
> + return -1;
> +}
> diff --git a/libstb/secboot_part.h b/libstb/secboot_part.h
> new file mode 100644
> index 00000000..7f29183b
> --- /dev/null
> +++ b/libstb/secboot_part.h
> @@ -0,0 +1,22 @@
> +/* Copyright 2013-2014 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 __SECBOOT_PART_H
> +#define __SECBOOT_PART_H
> +
> +int secboot_part_init(void);
> +
> +#endif /* __SECBOOT_PART_H */
> --
> 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