[Skiboot] [RFC 6/8] secvar/drivers: add a edk2-derived static key backend

[Daniel Axtens]

Eric Richter <erichte at linux.ibm.com> writes:

> The current edk2-compat-v1 backend expects the user of the system to
> enroll their own secure boot keys into variables to enable secure boot,
> and there is no behavior to allow default secure boot keys to be
> pre-loaded into variables for the hardware appliance use-case.
>
> This patch introduces a new backend driver that only exposes variables
> provided at compile-time, and will not process any updates. OS secure
> boot will always be enabled when utilizing this driver.
>
> This driver uses the same format as the edk2-compat-v1 driver, so existing
> code (e.g secvar-sysfs in linux) should be able to interact with this in
> the exact same manner.

Except, of course, that it's read-only. What happens to writes via
Linux? It seems they just land in the update queue and get ignored?

> To effectively utilize this driver, it is recommended to at least provide
> PK and db variable data (in ESL form), as according to the defaultvars
> specification.

As I mentioned, I think you should enforce this rather than recommend it.

> RFC NOTE: This is a simple driver not intended to be directly used; the
> next patch will introduce a wrapper driver that will switch between using
> this driver and the original edk2-compat-v1 driver based on the secure
> boot mode. Consider this driver to be the SYSTEM_MODE behavior, and the
> original unmodified edk2-compat-v1 driver to be USER_MODE.
>
> That said, I think it is still useful to provide as a standalone driver,
> rather than baked in to the switchable wrapper driver -- it could be used
> on a system that does not have a protected form of storage (e.g. TPM) to
> still enable secure boot if a user wants to compile their own firmware,
> or on an appliance that does not want/need to be able to disable secure
> boot entirely. In both cases, it should be used with a no-op storage
> driver.

In that case, does it mean we can test static key only boot in qemu now?
please? :P

>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
>  include/secvar.h                           |  1 +
>  libstb/secvar/backend/Makefile.inc         |  2 +-
>  libstb/secvar/backend/edk2-compat-static.c | 80 ++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 libstb/secvar/backend/edk2-compat-static.c
>
> diff --git a/include/secvar.h b/include/secvar.h
> index 259b9b63..3b439eaf 100644
> --- a/include/secvar.h
> +++ b/include/secvar.h
> @@ -40,6 +40,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;
> +extern struct secvar_backend_driver edk2_compatible_v1_static;

Isn't it the [edk2-compatible] [static key v1] driver rather than the
[edk2-compatible v1] [static key] driver?

Or perhaps the [edk2-compatible v1] [static key v1] driver is better?

Just thinking about how and when you might need to bump the version
number.

>  
>  int secvar_main(struct secvar_storage_driver, struct secvar_backend_driver);
>  
> diff --git a/libstb/secvar/backend/Makefile.inc b/libstb/secvar/backend/Makefile.inc
> index 436f9faf..b929769f 100644
> --- a/libstb/secvar/backend/Makefile.inc
> +++ b/libstb/secvar/backend/Makefile.inc
> @@ -5,7 +5,7 @@ SECVAR_BACKEND_DIR = libstb/secvar/backend
>  
>  SUBDIRS += $(SECVAR_BACKEND_DIR)
>  
> -SECVAR_BACKEND_OBJS = edk2-compat.o edk2-compat-process.o edk2-compat-reset.o
> +SECVAR_BACKEND_OBJS = edk2-compat.o edk2-compat-process.o edk2-compat-reset.o edk2-compat-static.o
>  SECVAR_BACKEND = $(SECVAR_BACKEND_DIR)/built-in.a
>  
>  $(SECVAR_BACKEND): $(SECVAR_BACKEND_OBJS:%=$(SECVAR_BACKEND_DIR)/%)
> diff --git a/libstb/secvar/backend/edk2-compat-static.c b/libstb/secvar/backend/edk2-compat-static.c
> new file mode 100644
> index 00000000..8c708a8c
> --- /dev/null
> +++ b/libstb/secvar/backend/edk2-compat-static.c
> @@ -0,0 +1,80 @@
> +#include <opal.h>
> +#include "../secvar_devtree.h"
> +#include "../secvar.h"
> +#include "../defaultvars/secvar_default_vars.h"
> +
> +static int edk2_compat_pre_process_static(struct list_head *variable_bank,
> +					  struct list_head *update_bank __unused)
> +{
> +	struct secvar *var;
> +
> +	/* Avoid unused if no static keys */
> +	(void) var;
> +        (void) variable_bank;
> +
> +#ifdef SECVAR_DEFAULT_PK
> +	var = new_secvar("PK", 3, secvar_default_PK, sizeof(secvar_default_PK), SECVAR_FLAG_VOLATILE);
> +	if (!var)
> +		return OPAL_NO_MEM;
> +
> +	list_add_tail(variable_bank, &var->link);
> +#endif
> +#ifdef SECVAR_DEFAULT_KEK
> +	var = new_secvar("KEK", 3, secvar_default_KEK, sizeof(secvar_default_KEK), SECVAR_FLAG_VOLATILE);
> +	if (!var)
> +		return OPAL_NO_MEM;
> +
> +	list_add_tail(variable_bank, &var->link);
> +#endif
> +#ifdef SECVAR_DEFAULT_DB
> +	var = new_secvar("db", 3, secvar_default_db, sizeof(secvar_default_db), SECVAR_FLAG_VOLATILE);
> +	if (!var)
> +		return OPAL_NO_MEM;
> +
> +	list_add_tail(variable_bank, &var->link);

If you don't have a default db you can't boot, right? Should you
detect this at compile time and #error out? Or does this get built in
even to a 'regular' build of skiboot that doesn't have default keys?

(I guess relatedly, do you need to forbid a transition to static key
mode if there isn't a minimal set of keys? What happens if someone wants
to build without static key support - how do we configure that? Is it
just per-platform?)

> +#endif
> +#ifdef SECVAR_DEFAULT_DBX
> +	var = new_secvar("dbx", 3, secvar_default_dbx, sizeof(secvar_default_dbx), SECVAR_FLAG_VOLATILE);
> +	if (!var)
> +		return OPAL_NO_MEM;
> +
> +	list_add_tail(variable_bank, &var->link);
> +#endif
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +
> +static int edk2_compat_process_static(struct list_head *variable_bank __unused,
> +				      struct list_head *update_bank __unused)
> +{
> +        /* No updates will ever be processed, so return an EMPTY here to signal
> +         *  that no updates were managed, and thus the storage driver doesn't have
> +         *  to do anything either. */
> +	return OPAL_EMPTY;
> +}
> +
> +static int edk2_compat_post_process_static(struct list_head *variable_bank __unused,
> +					   struct list_head *update_bank __unused)
> +{
> +
> +	/* Always set secure mode when using this static driver. */
> +
> +	secvar_set_secure_mode();
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +static int edk2_compat_validate_static(struct secvar *var __unused)
> +{
> +	/* No updates are processed in static key mode. Reject all. */
> +	return OPAL_PERMISSION;
> +};


So humour my ignorance and spell it out for me - what happens to queued
writes in the PNOR? Obviously they don't _do_ anything, but what's
communicated to the user? What happens to the contents of the PNOR
update queue - does it get reset or do we go through this process every
boot if something gets put in PNOR?

(More of a thing for the next patch, but) What happens to queued updates
on a mode switch? Are there ordering constraints we need to be aware of?

> +
> +struct secvar_backend_driver edk2_compatible_v1_static = {
> +	.pre_process = edk2_compat_pre_process_static,
> +	.process = edk2_compat_process_static,
> +	.post_process = edk2_compat_post_process_static,
> +	.validate = edk2_compat_validate_static,
> +	.compatible = "ibm,edk2-compat-v1", // TODO: add an additional static compatible here?

Are you meaningfully compatible with ibm,edk2-compat-v1? Pretending you
are edk2-compat-v1 is going to really mess up anyone who tries to write
an update. secvarctl - for example - will have no way of telling that
its writes are doomed.

You may - however - be able to get away without new code in the
kernel. is_ppc_secureboot_enabled() keys off `os-secureboot-enforcing`,
so as long as you have that inside an `ibm-secureboot{,-v1,v2}`
directory everything should still work.

Kind regards,
Daniel