[Skiboot] [RFC 3/8] secvar/drivers: add mode-switchable storage driver for secboot_tpm
Eric Richter
erichte at linux.ibm.com
Thu Oct 7 08:54:26 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.
>
Most of the error paths are TODOs because if we go this route, I'd really
prefer rewriting the current storage driver's _init() function. It's kind of
a mess of branches that sometimes repeat the same logic.
Furthermore, I would like to avoid re-initializing the TPM data structures
if possible, so that _init() function would need to better handle partial
inits.
> * 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!)
>
prov = "manufacturing provisioned".
This isn't something that is super clear in the code, apologies. When we
started this eons ago, we had a different set of sizes + attributes to define
the TPM NV indices which was sent to manufacturing to provision the TPMs.
So, if we detect "manufacturing provisioned" TPM NV indices, we treat it as
a first boot scenario, similarly if they were not defined in the first place.
To clarify in case, the TPM_ReadPublic command returns a "name", which is a hash
of the bits used to define the NV index, most notably the size and attributes.
These hashes should always be the same and should never change -- hence these
checks.
> 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?
>
mode_prov_name is the manufacturing provisioned TPM NV name, and
mode_name is the expected *reprovisioned* hash name.
Right now, they are the same because I skipped generating the new name hash
until the actual TPM NV index definition (size, attributes) are determined.
This is used to check if the NV index has been defined yet (or redefined, in the case
of the manu-provisioned NV index).
>> +
>> +
>> + 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?
This is somewhat documented in doc/secvar/driver-api.rst
"If this hook returns an error (non-zero) code, secvar will immediately halt the boot."
Errors to initialize the storage driver means we have zero ability to determine secure
boot state, and thus we panic and give up.
>
> 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?
>
Yes and no.
The possible ways this could be hit are:
1. A user defines a very large NV index prior to flashing on firmware that supports secvar
2. A user flashes on a version of skiboot that does not set platform auth & lock the nv indices,
and then decides to undefine them, etc
In both cases, the storage driver will panic because it either cannot define the NV indices due to
insufficient space, or the storage driver will panic because the expected NV indices are not defined
with the expected size + attributes (or the manu-provisioned attributes, which will immediately be
reclaimed by the driver anyway).
So the short answer is, not really. Not unless the user was intentionally trying to compromise their
system, as they'd have to circumvent FW secure boot to do so.
> Kind regards,
> Daniel
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
More information about the Skiboot
mailing list