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

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.

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.

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.

Signed-off-by: Eric Richter <erichte@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

Eric Richter <erichte@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@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

>> 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?
> 

Yep. Though I would like to add a call to the driver's .validate() hook in
the enqueue API call, which we could use to reject all updates in the case of
using the static driver.

>> 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.
Agreed.

> 
>> 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
> 

Should work perfectly for that. This is probably the least TODO'd code, so
I can always separate this driver out and accelerate upstreaming if strongly
desired :P

>>
>> Signed-off-by: Eric Richter <erichte@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.
> 

Linux expects the edk2-compat-v1 "format" to know how to interpret the data
in the variables (and what variables to expect). I think it would make sense though
to include an extra compatible string though, I believe that's allowed by the
device tree spec?

>>  
>>  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?
> 

Unfortunately driver selection is handled at run-time, since it is set in
platform specific code (see for example, at the bottom of platforms/witherspoon.c).

So yes, this is compiled for all builds. This could be made an immediate runtime failure
though so at least *ideally* this wouldn't get past CI.

> (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?)
> 

Per-platform. I don't think any platform will enable this or the switchable driver by default,
so it will be up to the firmware builder/shipper to enable this for their build.

>> +#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?
> 

The significance of the return codes of the .process() hook is somewhat defined
in the `doc/secvar/driver-api.rst` document. Since .process() always returns
OPAL_EMPTY, it signals that there were no updates processed, so the `secvar_main`
process does not write to the variable storage (or update storage).

HOWEVER, we already don't write to storage in the storage driver -- those hooks are
also no-ops. So basically we double-up with doing exactly nothing on variable update
requests.

So queued updates will kind of... sit there forever if they somehow exist, perhaps on
a mode switch.

> (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?
> 

Good point, I didn't consider this.

It isn't possible to queue updates in SYSTEM_MODE, due to the avoid reasons. Updates
issued at the same time might survive a mode switch? But since it requires a physical
presence/key-clear request to transition, that *should* be clearing the secboot partition
unconditionally when switching to USER_MODE, so I don't think updates will stick no matter
what until you boot into USER_MODE.

I will confirm this is the case though.

>> +
>> +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.
> 

I mentioned this before, but compatible is used to determine the format of the
variable data, and what variables it should reasonably expect. Since we are using
the same ESL/AUTH structures, compatibility is the same.

secvarctl however can perhaps be updated to check for the bonus "static" compatible
string, so it can warn when attempting to submit a futile update.

> 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.
> 

The kernel side of this might need to be tweaked to properly check compatible.
In the event we move away from the ESL format (or a variable named "db"), we should
not blindly attempt to load from there.

> Kind regards,
> Daniel
>