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

[Eric Richter]

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

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
>