[Skiboot] [RFC 3/8] secvar/drivers: add mode-switchable storage driver for secboot_tpm
Daniel Axtens
dja at axtens.net
Wed Oct 6 18:19:02 AEDT 2021
> This driver utilizes a previously unused TPM NV index reserved for opal
> secure boot use (0x01c10192) to store which mode it is operating in.
>
> The driver will initialize itself into a mode based on the value stored in
> the MODE index, and if undefined, on the status of the indices used by the
> secboot_tpm driver. The following describes the possible states:
> - MODE undefined, VARS/CONTROL undefined -> SYSTEM_MODE
> - MODE undefined, VARS/CONTROL defined -> USER_MODE
> - MODE index defined, value = SYSTEM_MODE -> SYSTEM_MODE
> - MODE index defined, value = USER_MODE -> USER_MODE
>
> The first option details the first-boot scenario, the system (if built
> with default variables) will boot directly into SYSTEM_MODE. The second
> option covers a scenario where the machine may have initialized secvar
> prior (e.g. firmware upgrade occurred), and so it initializes in USER_MODE
> instead, to preserve the machine's previous state.
So this is the part of this series that I'm most interested in because
this sits at the core of the security properties of your design.
At first glance it seems sane.
* You're storing MODE in the TPM, which means (assuming we lock the TPM
during boot correctly, etc) it can't be changed by the OS.
* Your defaults make sense: an undefined MODE implies SYSTEM_MODE.
Some thoughts:
* Some of the error paths seem to still be TODOs. This is something to
which I will want to pay close attention.
* I find the names of things confusing. What are "prov" names? (and how
are names derived generally? sorry I'm not really up on how TPMs
operate in this respect!)
I have some related sorts of questions in the code, but I struggle to
follow the body of the function - I think because I don't grok the names
properly - so I'll have to come back and look at things in more detail later.
> Physical presence is used to switch between modes. Asserting with the
> clear-os-keys option will set to USER_MODE, and effectively disable
> secure boot. Asserting with reset-default-keys will set to SYSTEM_MODE,
> and secure boot will be enabled with the default keys provided by the
> backend.
>
> RFC NOTE: This is probably the messiest part of this patch set. I am trying to
> keep the secboot_tpm driver relatively untouched, so that these new modes
> are purely an extension on the existing driver(s). However, as the TPM NV
> space is needed to securely preserve the state, this new driver needs to
> re-implement a lot of the same TPM NV initialization logic.
>
> I am looking to refactor the existing secboot_tpm driver to be more
> modular, and also better reactive to some of its components being
> pre-initialized. At the moment, it should most "work" as is, it is just
> very inelegant as it repeats a lot of the same initialization steps when
> the init function is called in USER_MODE.
>
> I have left a lot of TODO comments in where I would be copy-pasting even
> more bits from secboot_tpm -- all candidates for a better refactor and
> reuse of code from secboot_tpm.
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> include/secvar.h | 2 +
> libstb/secvar/storage/Makefile.inc | 2 +-
> .../secvar/storage/secboot_tpm_switchable.c | 251 ++++++++++++++++++
> .../secvar/storage/secboot_tpm_switchable.h | 15 ++
> 4 files changed, 269 insertions(+), 1 deletion(-)
> create mode 100644 libstb/secvar/storage/secboot_tpm_switchable.c
> create mode 100644 libstb/secvar/storage/secboot_tpm_switchable.h
>
> diff --git a/include/secvar.h b/include/secvar.h
> index 413d7997..259b9b63 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -5,6 +5,7 @@
> #define _SECVAR_DRIVER_
>
> #include <stdint.h>
> +#include <ccan/list/list.h>
>
> struct secvar;
>
> @@ -37,6 +38,7 @@ struct secvar_backend_driver {
> };
>
> extern struct secvar_storage_driver secboot_tpm_driver;
> +extern struct secvar_storage_driver secboot_tpm_switchable_driver;
> extern struct secvar_backend_driver edk2_compatible_v1;
>
> 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 50156fe2..dbddd6e5 100644
> --- a/libstb/secvar/storage/Makefile.inc
> +++ b/libstb/secvar/storage/Makefile.inc
> @@ -7,7 +7,7 @@ SUBDIRS += $(SECVAR_STORAGE_DIR)
>
> # Swap the comment on these two lines to use the fake TPM NV
> # implementation hardware without a TPM
> -SECVAR_STORAGE_OBJS = secboot_tpm.o tpmnv_ops.o
> +SECVAR_STORAGE_OBJS = secboot_tpm.o tpmnv_ops.o secboot_tpm_switchable.o
> #SECVAR_STORAGE_OBJS = secboot_tpm.o fakenv_ops.o
>
> SECVAR_STORAGE = $(SECVAR_STORAGE_DIR)/built-in.a
> diff --git a/libstb/secvar/storage/secboot_tpm_switchable.c b/libstb/secvar/storage/secboot_tpm_switchable.c
> new file mode 100644
> index 00000000..0790ed68
> --- /dev/null
> +++ b/libstb/secvar/storage/secboot_tpm_switchable.c
> @@ -0,0 +1,251 @@
> +#include <stdlib.h>
> +#include <skiboot.h>
> +#include <opal.h>
> +#include "../secvar.h"
> +#include "../secvar_devtree.h"
> +#include "secboot_tpm.h"
> +#include <tssskiboot.h>
> +#include <ibmtss/TPM_Types.h>
> +#include "secboot_tpm_switchable.h"
> +
> +/* POTENTIAL ITEMS TO MOVE TO secboot_tpm.h */
> +#define SECBOOT_TPM_MAX_VAR_SIZE 8192
> +
> +extern const uint8_t tpmnv_vars_name[34];
> +extern const uint8_t tpmnv_vars_prov_name[34];
> +extern const uint8_t tpmnv_control_name[34];
> +extern const uint8_t tpmnv_control_prov_name[34];
> +
> +/* END POTENTIALLY HEADER STUFF */
> +
> +// TODO: actually get the correct mode name hash here.
> +const uint8_t tpmnv_mode_name[] = {
> + 0x00, 0x0b,
> + 0x7a, 0xe8, 0x16, 0xe0, 0x15, 0xab, 0xdc, 0xd8, 0xc6, 0xe6, 0xa1, 0x6f, 0x26, 0x71, 0x65, 0x96,
> + 0x69, 0x15, 0x04, 0xd1, 0x05, 0x14, 0xfc, 0xf0, 0x82, 0x17, 0x99, 0x22, 0x4e, 0x06, 0xe5, 0x76,
> +};
> +
> +const uint8_t tpmnv_mode_prov_name[] = {
> + 0x00, 0x0b,
> + 0x7a, 0xe8, 0x16, 0xe0, 0x15, 0xab, 0xdc, 0xd8, 0xc6, 0xe6, 0xa1, 0x6f, 0x26, 0x71, 0x65, 0x96,
> + 0x69, 0x15, 0x04, 0xd1, 0x05, 0x14, 0xfc, 0xf0, 0x82, 0x17, 0x99, 0x22, 0x4e, 0x06, 0xe5, 0x76,
> +};
> +
> +uint64_t mode_value = 0;
> +
> +
> +// TODO: break down into reusable functions w/ the functions in secboot_tpm_store_init
> +static int pre_switchable_init(void)
> +{
> + TPMI_RH_NV_INDEX *indices = NULL;
> + char nv_vars_name[sizeof(tpmnv_vars_name)];
> + char nv_control_name[sizeof(tpmnv_control_name)];
> + char nv_mode_name[sizeof(tpmnv_mode_name)];
> + size_t count = 0;
> + bool control_defined = false;
> + bool vars_defined = false;
> + bool mode_defined = false;
> + int i, rc;
> +
> + /* Check if the NV indices have been defined already */
> + rc = tpmnv_ops.getindices(&indices, &count);
> + if (rc) {
> + prlog(PR_ERR, "Could not load defined indicies from TPM, rc=%d\n", rc);
> + goto error;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (indices[i] == SECBOOT_TPMNV_VARS_INDEX)
> + vars_defined = true;
> + else if (indices[i] == SECBOOT_TPMNV_CONTROL_INDEX)
> + control_defined = true;
> + else if (indices[i] == SECBOOT_TPMNV_MODE_INDEX)
> + mode_defined = true;
> + }
> + free(indices);
> +
> + if (secvar_check_clear_keys() || secvar_check_set_default_keys()) {
> + /* TODO: undefine *ALL THREE* indices */
> + mode_defined = control_defined = vars_defined = false;
> + }
> +
> + if (mode_defined) {
> + // TODO: Probably break a lot of the following up into a reusable function between
> + // this driver and secboot_tpm
> + TPMS_NV_PUBLIC nv_public;
> + TPM2B_NAME vars_tmp;
> +
> + rc = tpmnv_ops.readpublic(SECBOOT_TPMNV_MODE_INDEX,
> + &nv_public,
> + &vars_tmp);
> + if (rc) {
> + prlog(PR_ERR, "Failed to readpublic from the MODE index, rc=%d\n", rc);
> + return rc;
> + }
> +
> + memcpy(nv_mode_name, vars_tmp.t.name, MIN(sizeof(tpmnv_vars_name), vars_tmp.t.size));
I assume nul-termination is handled correctly here?
> +
> + // TODO: perhaps rework this function too, or just drop that extra function
> + if (!memcmp(tpmnv_mode_prov_name, nv_mode_name, sizeof(tpmnv_mode_prov_name))) {
> + /* Detected a provisioned mode index, claim it here and set the mode to system mode */
> + // TODO: perhaps not hardcode the space
> + rc = tpmnv_ops.definespace(SECBOOT_TPMNV_MODE_INDEX, sizeof(uint64_t));
> + if (rc) {
> + prlog(PR_ERR, "Failed to define the MODE index, rc=%d\n", rc);
> + return rc;
> + }
> +
> + /* Write TPMNV variables to actual NV */
> + mode_value = 1;
> + rc = tpmnv_ops.write(SECBOOT_TPMNV_MODE_INDEX, &mode_value, sizeof(mode_value), 0);
> + if (rc)
> + return rc;
> +
> + return OPAL_SUCCESS;
> + }
> +
> + if (!memcmp(tpmnv_mode_name, nv_mode_name, sizeof(tpmnv_mode_name))) {
> + /* Detected an improperly defined mode index, give up? */
> + abort(); // TODO: don't actually abort, this should be in an init function
> + }
What do we mean by improperly defined mode name? we have nv_mode_name !=
{tpmnv_mode_prov_name, tpmnv_mode_name} - do mode_prov_name and
mode_name map to SYSTEM and USER?
> +
> +
> + rc = tpmnv_ops.read(SECBOOT_TPMNV_VARS_INDEX,
> + &mode_value,
> + sizeof(mode_value), 0);
> + if (rc) {
> + prlog(PR_ERR, "Failed to read from the VARS index\n");
> + goto error;
> + }
> +
> + return OPAL_SUCCESS;
> + }
> +
> + /* Mode index was not defined, so we need to check the other indices first */
> +
> + /* Determine if we need to define the indices. These should BOTH be false or true */
> + if (!vars_defined && !control_defined) {
> + // TODO: Define *ALL THREE* indices here, perhaps let the other init handle the formatting?
> + // or do we format here as well in secboot_tpm format?
> +
> + if (secvar_check_clear_keys()) {
> + /* clear-os-keys was issued, continue in user mode (OS secure boot disabled) */
> + mode_value = USER_MODE;
> + }
> +
> + /* reset-default-keys likely undefined the indices, either way default to system mode */
> + mode_value = SYSTEM_MODE;
> +
> + goto define;
> + }
> + if (vars_defined ^ control_defined) {
> + /* This should never happen. Both indices should be defined at the same
> + * time. Otherwise something seriously went wrong. */
> + prlog(PR_ERR, "NV indices defined with unexpected attributes. Assert physical presence to clear\n");
> + goto error;
> + }
> +
> + rc = secboot_tpm_get_tpmnv_names(nv_vars_name, nv_control_name);
> + if (rc)
> + goto error;
> +
> + if (secboot_tpm_check_provisioned_indices(nv_vars_name, nv_control_name)) {
> + // TODO: Reclaim the NV indices here, we default to system mode (noop),
> + // so that driver will not handle it.
> +
> + mode_value = SYSTEM_MODE;
> + goto define;
> + }
> +
> + /* Otherwise, ensure the NV indices were defined with the correct set of attributes */
> + rc = secboot_tpm_check_tpmnv_attrs(nv_vars_name, nv_control_name);
> + if (rc)
> + goto error;
> +
> + /* Reaching here, mode index was not defined, and the other NV indices were.
> + * This is (likely) a firmware update case, where we should preserve the previous
> + * secure boot state. */
> + mode_value = SYSTEM_MODE;
Wasn't the previous state USER_MODE?
> +
> +define:
> + rc = tpmnv_ops.definespace(SECBOOT_TPMNV_MODE_INDEX, sizeof(mode_value));
> + if (rc) {
> + prlog(PR_ERR, "Failed to define the MODE index, rc=%d\n", rc);
> + return rc;
> + }
So what happens if we return an error from this function for some
reason? Do we fail the boot?
Can a host fill up the TPM such that we cannot define a space? If so,
can they force a system to stay in a particular state?
Kind regards,
Daniel
More information about the Skiboot
mailing list