| Message ID | 20200511213152.24952-5-erichte@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add initial secure variable storage and backend drivers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (0f1937ef40fca0c3212a9dff1010b832a24fb063) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | fail | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote: > This patch adjusts the behavior of secvar_main to actually halt the boot > in some form if there is an issue initializing secure variables. > We > halt in skiboot if the secvar storage driver fails to initialize, as this > should only happen due to unresolvable hardware issues, and contains our > secure boot state. doesn't parse > For all other cases we enforce secure boot in the > bootloader (secure mode flag is set, but there will be no keys). > > Previously, the storage driver was expected to handle any locking > procedures implicitly as part of the write operation. This patch makes > the locking behavior now explicit, and part of the secvar_main flow. > > The storage driver is now locked unconditionally when exiting > secvar_main, and the lock() call should halt the boot if it encounters > any sign of struggle. Call it .seal(), or .lockdown(), or similar. Anything called lock() I'd expect to be balanced with an unlock(). > Signed-off-by: Eric Richter <erichte@linux.ibm.com> > --- > V4: > - adjusted lines to 80 columns > - calls secureboot_enforce() instead of abort() > > libstb/secvar/secvar_main.c | 38 +++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c > index 3b842d16..f4b98925 100644 > --- a/libstb/secvar/secvar_main.c > +++ b/libstb/secvar/secvar_main.c > @@ -8,6 +8,7 @@ > #include <stdlib.h> > #include <skiboot.h> > #include <opal.h> > +#include <libstb/secureboot.h> > #include "secvar.h" > #include "secvar_devtree.h" > > @@ -39,11 +40,15 @@ int secvar_main(struct secvar_storage_driver storage_driver, > list_head_init(&variable_bank); > list_head_init(&update_bank); > > + /* Failures here should indicate some kind of hardware problem, > + * therefore we don't even attempt to continue */ > rc = secvar_storage.store_init(); > if (rc) > - goto fail; > + secureboot_enforce(); > - /* Failures here should indicate some kind of hardware problem */ > + /* Failures here may be recoverable, > + * (PNOR was corrupted, restore from backup) > + * so we want to boot up to skiroot to give the user a chance to fix */ Can you elaborate about what is actually happening here. I think you're saying it'll boot to petitboot with an empty key ring so you can potentially restore it, but the verbiage here assumes you already know what it does. > rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK); > if (rc) > goto fail; > @@ -61,6 +66,8 @@ int secvar_main(struct secvar_storage_driver storage_driver, > rc = secvar_backend.pre_process(); > if (rc) { > prlog(PR_ERR, "Error in backend pre_process = %d\n", rc); > + /* Early failure state, lock the storage */ > + secvar_storage.lock(); > goto out; > } > } > @@ -83,11 +90,20 @@ int secvar_main(struct secvar_storage_driver storage_driver, > SECVAR_VARIABLE_BANK); > if (rc) > goto out; > - > - rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK); > + } > + /* Write (and probably clear) the update bank if .process() actually > + * detected and handled updates in the update bank. Unlike above, this > + * includes error cases, where the backend should probably be clearing > + * the bank. > + */ > + if (rc != OPAL_EMPTY) { > + rc = secvar_storage.write_bank(&update_bank, > + SECVAR_UPDATE_BANK); > if (rc) > goto out; > } > + /* Unconditionally lock the storage at this point */ > + secvar_storage.lock(); > > if (secvar_backend.post_process) { > rc = secvar_backend.post_process(); > @@ -100,9 +116,23 @@ int secvar_main(struct secvar_storage_driver storage_driver, > prlog(PR_INFO, "secvar initialized successfully\n"); > > return OPAL_SUCCESS; > + > fail: > + /* Early failure, base secvar support failed to initialize */ > secvar_set_status("fail"); > + secvar_storage.lock(); > + secvar_set_secure_mode(); > + > + prerror("secvar failed to initialize, rc = %04x\n", rc); > + return rc; > + > out: rename out to soft_fail or something. > + /* Soft-failure, enforce secure boot in bootloader for debug/recovery */ > + clear_bank_list(&variable_bank); > + clear_bank_list(&update_bank); > + secvar_storage.lock(); > + secvar_set_secure_mode(); > + > prerror("secvar failed to initialize, rc = %04x\n", rc); > return rc; > }
diff --git a/libstb/secvar/secvar_main.c b/libstb/secvar/secvar_main.c index 3b842d16..f4b98925 100644 --- a/libstb/secvar/secvar_main.c +++ b/libstb/secvar/secvar_main.c @@ -8,6 +8,7 @@ #include <stdlib.h> #include <skiboot.h> #include <opal.h> +#include <libstb/secureboot.h> #include "secvar.h" #include "secvar_devtree.h" @@ -39,11 +40,15 @@ int secvar_main(struct secvar_storage_driver storage_driver, list_head_init(&variable_bank); list_head_init(&update_bank); + /* Failures here should indicate some kind of hardware problem, + * therefore we don't even attempt to continue */ rc = secvar_storage.store_init(); if (rc) - goto fail; + secureboot_enforce(); - /* Failures here should indicate some kind of hardware problem */ + /* Failures here may be recoverable, + * (PNOR was corrupted, restore from backup) + * so we want to boot up to skiroot to give the user a chance to fix */ rc = secvar_storage.load_bank(&variable_bank, SECVAR_VARIABLE_BANK); if (rc) goto fail; @@ -61,6 +66,8 @@ int secvar_main(struct secvar_storage_driver storage_driver, rc = secvar_backend.pre_process(); if (rc) { prlog(PR_ERR, "Error in backend pre_process = %d\n", rc); + /* Early failure state, lock the storage */ + secvar_storage.lock(); goto out; } } @@ -83,11 +90,20 @@ int secvar_main(struct secvar_storage_driver storage_driver, SECVAR_VARIABLE_BANK); if (rc) goto out; - - rc = secvar_storage.write_bank(&update_bank, SECVAR_UPDATE_BANK); + } + /* Write (and probably clear) the update bank if .process() actually + * detected and handled updates in the update bank. Unlike above, this + * includes error cases, where the backend should probably be clearing + * the bank. + */ + if (rc != OPAL_EMPTY) { + rc = secvar_storage.write_bank(&update_bank, + SECVAR_UPDATE_BANK); if (rc) goto out; } + /* Unconditionally lock the storage at this point */ + secvar_storage.lock(); if (secvar_backend.post_process) { rc = secvar_backend.post_process(); @@ -100,9 +116,23 @@ int secvar_main(struct secvar_storage_driver storage_driver, prlog(PR_INFO, "secvar initialized successfully\n"); return OPAL_SUCCESS; + fail: + /* Early failure, base secvar support failed to initialize */ secvar_set_status("fail"); + secvar_storage.lock(); + secvar_set_secure_mode(); + + prerror("secvar failed to initialize, rc = %04x\n", rc); + return rc; + out: + /* Soft-failure, enforce secure boot in bootloader for debug/recovery */ + clear_bank_list(&variable_bank); + clear_bank_list(&update_bank); + secvar_storage.lock(); + secvar_set_secure_mode(); + prerror("secvar failed to initialize, rc = %04x\n", rc); return rc; }
This patch adjusts the behavior of secvar_main to actually halt the boot in some form if there is an issue initializing secure variables. We halt in skiboot if the secvar storage driver fails to initialize, as this should only happen due to unresolvable hardware issues, and contains our secure boot state. For all other cases we enforce secure boot in the bootloader (secure mode flag is set, but there will be no keys). Previously, the storage driver was expected to handle any locking procedures implicitly as part of the write operation. This patch makes the locking behavior now explicit, and part of the secvar_main flow. The storage driver is now locked unconditionally when exiting secvar_main, and the lock() call should halt the boot if it encounters any sign of struggle. Signed-off-by: Eric Richter <erichte@linux.ibm.com> --- V4: - adjusted lines to 80 columns - calls secureboot_enforce() instead of abort() libstb/secvar/secvar_main.c | 38 +++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)